Ticket #2685 (closed New Feature: fixed)

Opened 16 months ago

Last modified 14 months ago

Integrate the SpellChecker.net "Web Spell Checker"

Reported by: fredck Owned by: fredck
Priority: Normal Milestone: FCKeditor 2.6.4
Component: UI : Spell Checker Version:
Keywords: Confirmed Review+ Cc: SpellChecker.net

Description

We are partnering with  SpellChecker.net to bring a nice new feature to the editor. It consists on a powerful spell checker (and not only) that requires no installation neither in the browser nor in the server.

This spell checker is called "Web Spell Checker". It's an online service provided by SpellChecker.net, which is closely integrated with the editor interface. It is free, but the free version displays a banner space while the spell check dialog is open.

Because of the "zero installation" nature of this solution, it is supposed to become the default spell checker in the editor. We'll still support SpellerPages and ieSpell just like before.

This ticket should handle not only the code necessary to introduce this feature in the editor, but also all documentation changes and additions to properly inform our users about the possible settings, and an explanation for the banner thing.

Attachments

2685.patch Download (12.4 KB) - added by fredck 16 months ago.
icon.gif Download (0.9 KB) - added by fredck 16 months ago.
Proposed icon change.
2685_2.patch Download (13.9 KB) - added by fredck 14 months ago.
2685_3.patch Download (13.9 KB) - added by fredck 14 months ago.

Change History

Changed 16 months ago by fredck

Changed 16 months ago by fredck

Proposed icon change.

  Changed 16 months ago by fredck

  • status changed from new to assigned

A first patch has been attached. It should be already work, using the SpellChecker.net web services.

SpellChecker.net is also proposing a new Spell Check icon, to replace the current one. It's also attached.

I'm not asking for review yet, as we are still controlling the code with SpellChecker.net.

  Changed 15 months ago by fredck

  • keywords Review? added

follow-up: ↓ 4   Changed 15 months ago by alfonsoml

  • keywords Review- added; Review? removed

The dialog is reading the data with GetData() but then it sets it back with innerHTML, so any code protection won't be executed.

The coding style doesn't follow the guidelines, and there are several spelling mistakes in the comments :-)
There are several debugging statements commented out, I don't see any reason to keep them in the production code.

I'm not sure if the code should use the "default" case in FCKSpellCheckCommand.prototype.Execute or if it should work only when the value is "WSC" (and also the IsEnabled property)

The code uses frames with src="", doesn't that generate problems when using https? (I'm not sure). Anyway, the user will get a warning due to the load of  http://loader.spellchecker.net from a secure page.

Changed 14 months ago by fredck

in reply to: ↑ 3   Changed 14 months ago by fredck

  • keywords Review? added; Review- removed

Ok... a new patch has been added with several modifications.

Replying to alfonsoml:

The dialog is reading the data with GetData() but then it sets it back with innerHTML, so any code protection won't be executed.

I've changed it to use SetData instead.

The coding style doesn't follow the guidelines, and there are several spelling mistakes in the comments :-)

I've fixed most of the code style. (this code has been provided by SpellChecker.net)

There are several debugging statements commented out, I don't see any reason to keep them in the production code.

Cleaned up.

I'm not sure if the code should use the "default" case in FCKSpellCheckCommand.prototype.Execute or if it should work only when the value is "WSC" (and also the IsEnabled property)

True... it's not anymore a default case. No changes for IsEnabled instead, as our currently implementation means that FF and similars are not compatible with ieSpell, not that they are compatible with a set of option.

The code uses frames with src="", doesn't that generate problems when using https? (I'm not sure). Anyway, the user will get a warning due to the load of  http://loader.spellchecker.net from a secure page.

I've tested it over https. It works well, but it gives the "mixed content" warning. Not a big issue for the beta, but we'll try to fix it for the stable. I'll file a ticket for it, as soon as this ticket get closed.

  Changed 14 months ago by martinkou

  • keywords Review- added; Review? removed

w.html, line 146: If initAndSpell() has been called, then this check shouldn't be done at all. w.html, line 109, 119, 121, 171, 199, 216: Still need to correct bracket styles. tmpFrameset.html, line 33, 36, 43, 50, 51: Bracket styles. ciframe.html, line 48, 55: Bracket styles. fckspellcheckcommand_gecko.js, line 28: Recommend it to be changed to

this.IsEnabled = FCKConfig.SpellChecker.Equals( [ 'WSC', 'SpellerPages' ] );

fckspellcheckcommand_ie.js, line 28: Recommend it to be changed to

this.IsEnabled = FCKConfig.SpellChecker.Equals( [ 'WSC', 'SpellerPages', 'ieSpell' ] );

Changed 14 months ago by fredck

  Changed 14 months ago by fredck

Attached a new patch with the code style changes.

As said in my previous comments, the patch implements isEnable in a way that it specifies that ieSpell is not compatible with Firefox. Any other value is accepted as compatible with all browsers. This is intentional. The code is shorter... and we can't apply error check around all the code in JavaScript, unfortunately.

  Changed 14 months ago by martinkou

  • keywords Review+ added; Review- removed

  Changed 14 months ago by fredck

Ok... awaiting for the SpellChecker.net review to commit.

  Changed 14 months ago by SpellChecker.net

Review+ by SpellChecker.net

We have reviewed the patch 2685_3.patch and did not found any problem.

  Changed 14 months ago by fredck

  • cc SpellChecker.net added
  • status changed from assigned to closed
  • resolution set to fixed

Fixed with [2907].

  Changed 14 months ago by fredck

The new icon has been introduced with [2909].

Note: See TracTickets for help on using tickets.