Last Comment Bug 338318 - UI for spellcheck in browser window (form fields)
: UI for spellcheck in browser window (form fields)
Status: VERIFIED FIXED
[verified-seamonkey1.1b]
: fixed-seamonkey1.1b, fixed1.8.1
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: ---
Assigned To: Ian Neal
:
Mentors:
Depends on: 302050 370436
Blocks: 354289
  Show dependency treegraph
 
Reported: 2006-05-17 09:46 PDT by Jason Bassford
Modified: 2008-07-31 04:23 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
First draft of patch v0.1 (28.23 KB, patch)
2006-09-17 12:01 PDT, Ian Neal
neil: review+
Details | Diff | Splinter Review
Return early patch v0.1a (27.77 KB, patch)
2006-09-21 16:30 PDT, Ian Neal
neil: review+
Details | Diff | Splinter Review
xpfe as well as suite patch v0.1b (Checked into trunk) (31.34 KB, patch)
2006-09-22 16:13 PDT, Ian Neal
iann_bugzilla: review+
jag-mozilla: superreview+
kairo: approval‑seamonkey1.1b+
Details | Diff | Splinter Review
Branch patch v0.1b (Checked into 1.8 branch) (28.47 KB, patch)
2006-09-25 16:26 PDT, Ian Neal
no flags Details | Diff | Splinter Review
Regression fix for branch TB v0.1c (Checked into 1.8 branch) (1.44 KB, patch)
2006-09-26 13:57 PDT, Ian Neal
mscott: review+
mscott: superreview+
Details | Diff | Splinter Review

Description Jason Bassford 2006-05-17 09:46:17 PDT
This is a follow-up from bug 16409, filed against core, that was just marked FIXED.  This isn't fixed in SeaMonkey yet - but, apparently, it's just a matter of getting the GUI for it as the backend is now in place.  It may be dependent on the switch to Toolkit.
Comment 1 Robert Kaiser 2006-08-19 12:31:28 PDT
SeaMonkey build from trunk and 1.8 branch actually show the red underlines for mistyped words but there's no UI for correcting words or changing dictionaries, so changing summary to reflect what's really missing. I guess the backend is there from bug 302050
Comment 2 Robert Kaiser 2006-08-19 12:35:08 PDT
That Firefox frontend patch for this functionality is attachment 203956 [details] [diff] [review] - we probably want that ported to SeaMonkey browser at least
Comment 3 Robert Kaiser 2006-08-19 12:53:51 PDT
IanN already has some experience with spellcheck stuff e.g. from bug 337155 - could/would you help with this? Or anyone else who can look into that issue? It would really be nice to not only show there's something wrong but also to be able to correct it (or change to a more sensible language when I'm posting on German pages).
Comment 4 Ian Neal 2006-09-17 12:01:16 PDT
Created attachment 238915 [details] [diff] [review]
First draft of patch v0.1

This patch:
* Adds UI to spell checking for pages with editable web content
* Adds pref into Navigator -> Languages pref pane
* Updates help for Navigator -> Languages pref pane
Comment 5 neil@parkwaycc.co.uk 2006-09-18 06:35:15 PDT
Comment on attachment 238915 [details] [diff] [review]
First draft of patch v0.1

>+menuitem.spell-suggestion {
>+  font-weight: bold;
>+}
This belongs in the themes' communicator.css, so we can reuse it in editor and message compose. You don't need the tag name, the class will do.

>+                if (this.onTextInput && ! this.target.readOnly && this.target.type != "password") {
Use this.target.type == "text" instead, and you could check for disabled inputs in case someone fixes the can't right-click on disabled inputs bug.

>+        // if the document is editable, show context menu like in text inputs
I think you should have this test first, just after setting autoDownload; maybe move the uninit stuff there too. Better still, return early thus avoiding having to change the selectAll test.

>+        url = pref.getComplexValue("editor.spellcheckers.url",
>+                                   Components.interfaces.nsIPrefLocalizedString).data;
>+        window.open(url);
I've since discovered that we shouldn't be doing this, because it sets the opener to the chrome window :-(

r=me with these fixed.
Comment 6 Ian Neal 2006-09-21 16:30:51 PDT
Created attachment 239574 [details] [diff] [review]
Return early patch v0.1a

Changes since v0.1:
* Moved editable document earlier so could return early.
* Use window.openDialog instead of window.open
* css changes now in themes
Comment 7 neil@parkwaycc.co.uk 2006-09-22 03:31:42 PDT
Comment on attachment 239574 [details] [diff] [review]
Return early patch v0.1a

>+                if (this.onTextInput && ! this.target.readOnly && ! this.target.disabled && this.target.type == "text") {
You don't need to test this.onTextInput, this.target.type == "text" ensures the target is a text box. Also I don't like that space after ! style.

>+                if (! this.target.readOnly) {
Check for disabled too please.

>+      var url;
>+      try {
>+        url = pref.getComplexValue("editor.spellcheckers.url",
Ditch the try/catch? At least write var url = ...;
 
>+*       content/global/inlineSpellCheckUI.js        (/toolkit/content/inlineSpellCheckUI.js)
Why oh why oh why is toolkit so worried about the size of licences? :-(
Comment 8 neil@parkwaycc.co.uk 2006-09-22 06:19:54 PDT
Comment on attachment 239574 [details] [diff] [review]
Return early patch v0.1a

>Index: suite/browser/browser-prefs.js
Isn't this version suiterunner only, so you need to patch xpfe too?
Comment 9 Robert Kaiser 2006-09-22 08:06:08 PDT
(In reply to comment #8)
> (From update of attachment 239574 [details] [diff] [review] [edit])
> >Index: suite/browser/browser-prefs.js
> Isn't this version suiterunner only, so you need to patch xpfe too?
 
Neil's right with this, you need to patch both in this case on trunk.
BTW, same is true for the help document, as in our current intermediate state of those, we're not actually using the one in suite/ yet.
Comment 10 Ian Neal 2006-09-22 16:13:40 PDT
Created attachment 239730 [details] [diff] [review]
xpfe as well as suite patch v0.1b (Checked into trunk)

Changes since v0.1a:
* xpfe versions of help file and browser-prefs.js included.
* move var within try.

Carrying forward r=
Comment 11 jag (Peter Annema) 2006-09-24 15:06:15 PDT
Comment on attachment 239730 [details] [diff] [review]
xpfe as well as suite patch v0.1b (Checked into trunk)

>@@ -231,17 +267,17 @@ nsContextMenu.prototype = {

>-        this.showItem( "context-selectall", true );
>+        this.showItem( "context-selectall", !( this.onLink || this.onImage ) );

Is this part of this bug?
Comment 12 Ian Neal 2006-09-24 16:25:59 PDT
(In reply to comment #11)
> (From update of attachment 239730 [details] [diff] [review] [edit])
> >@@ -231,17 +267,17 @@ nsContextMenu.prototype = {
> 
> >-        this.showItem( "context-selectall", true );
> >+        this.showItem( "context-selectall", !( this.onLink || this.onImage ) );
> 
> Is this part of this bug?
> 
Yes, well it was intended to be there.
Comment 13 Ian Neal 2006-09-24 16:40:13 PDT
Comment on attachment 239730 [details] [diff] [review]
xpfe as well as suite patch v0.1b (Checked into trunk)

Checking in (trunk)
suite/browser/browser-prefs.js;
new revision: 1.51; previous revision: 1.50
suite/common/contentAreaContextOverlay.xul;
new revision: 1.61; previous revision: 1.60
suite/common/nsContextMenu.js;
new revision: 1.116; previous revision: 1.115
suite/common/pref/pref-languages.xul;
new revision: 1.51; previous revision: 1.50
suite/locales/en-US/chrome/common/contentAreaCommands.dtd;
new revision: 1.32; previous revision: 1.31
suite/locales/en-US/chrome/common/help/cs_nav_prefs_navigator.xhtml;
new revision: 1.38; previous revision: 1.37
suite/locales/en-US/chrome/common/pref/pref-languages.dtd;
new revision: 1.19; previous revision: 1.18
xpfe/bootstrap/browser-prefs.js;
new revision: 1.51; previous revision: 1.50
xpfe/global/jar.mn;
new revision: 1.99; previous revision: 1.98
themes/classic/communicator/communicator.css;
new revision: 1.18; previous revision: 1.17
themes/modern/communicator/communicator.css;
new revision: 1.27; previous revision: 1.26
extensions/help/resources/locale/en-US/cs_nav_prefs_navigator.xhtml;
new revision: 1.37; previous revision: 1.36
done
Comment 14 Ian Neal 2006-09-24 16:40:50 PDT
Comment on attachment 239730 [details] [diff] [review]
xpfe as well as suite patch v0.1b (Checked into trunk)

Requesting approval for SM1.1b
Comment 15 Robert Kaiser 2006-09-25 04:39:05 PDT
Comment on attachment 239730 [details] [diff] [review]
xpfe as well as suite patch v0.1b (Checked into trunk)

a=me given it works well on trunk.
Comment 16 Ian Neal 2006-09-25 16:26:06 PDT
Created attachment 240064 [details] [diff] [review]
Branch patch v0.1b (Checked into 1.8 branch)

Branch version of patch v0.1b
Comment 17 Ian Neal 2006-09-25 16:30:10 PDT
Comment on attachment 240064 [details] [diff] [review]
Branch patch v0.1b (Checked into 1.8 branch)

Checking in (1.8 branch)
xpfe/bootstrap/browser-prefs.js;
new revision: 1.28.2.13; previous revision: 1.28.2.12
xpfe/communicator/resources/content/contentAreaContextOverlay.xul;
new revision: 1.57.4.3; previous revision: 1.57.4.2
xpfe/communicator/resources/content/nsContextMenu.js;
new revision: 1.108.2.7; previous revision: 1.108.2.6
xpfe/communicator/resources/locale/en-US/contentAreaCommands.dtd;
new revision: 1.29.12.2; previous revision: 1.29.12.1
xpfe/components/prefwindow/resources/content/pref-languages.xul;
new revision: 1.50.24.1; previous revision: 1.50
xpfe/components/prefwindow/resources/locale/en-US/pref-languages.dtd;
new revision: 1.18.44.1; previous revision: 1.18
xpfe/global/jar.mn;
new revision: 1.85.2.3; previous revision: 1.85.2.2
themes/classic/communicator/communicator.css;
new revision: 1.12.28.3; previous revision: 1.12.28.2
themes/modern/communicator/communicator.css;
new revision: 1.23.28.3; previous revision: 1.23.28.2
extensions/help/resources/locale/en-US/cs_nav_prefs_navigator.xhtml;
new revision: 1.29.6.7; previous revision: 1.29.6.6
done
Comment 18 Scott MacGregor 2006-09-26 08:13:03 PDT
Hey Iann,

This patch seemed to have caused some entity errors in today's branch Thunderbird build. See Bug 302050, we probably need some of the entities that were added.
Comment 19 Robert Kaiser 2006-09-26 09:51:24 PDT
Hah, on branch we still use xpfe/communicator (on trunk we don't package any UI from there any more) which is also used by Thunderbird (on both trunk and branch), so it's entirely possible that we conflict there in some way.
Either we add the locale parts in mail/ as well on branch, or we wrap the contentAreaContextOverlay.xul changes (maybe also the nsContextMenu.js ones) in chrome preprocessor |#ifdef MOZ_SUITE| blocks.
Comment 20 Ian Neal 2006-09-26 13:57:27 PDT
Created attachment 240202 [details] [diff] [review]
Regression fix for branch TB v0.1c (Checked into 1.8 branch)

This patch:
* Adds missing locale changes
Comment 21 Scott MacGregor 2006-09-26 14:00:49 PDT
Comment on attachment 240202 [details] [diff] [review]
Regression fix for branch TB v0.1c (Checked into 1.8 branch)

Thanks Ian. Feel free to check this into the 1.8.1 branch. a=mscott
Comment 22 Ian Neal 2006-09-26 14:17:14 PDT
Comment on attachment 240202 [details] [diff] [review]
Regression fix for branch TB v0.1c (Checked into 1.8 branch)

Checking in (1.8 branch)
mail/locales/en-US/chrome/communicator/contentAreaCommands.dtd;
new revision: 1.3.2.1; previous revision: 1.3
done
Comment 23 Serge Gautherie (:sgautherie) 2006-10-01 21:39:41 PDT
[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1) Gecko/20060930 SeaMonkey/1.1b] (nightly) (W98SE)

V.Fixed on MOZILLA_1_8_BRANCH.
Comment 24 Stephen Donner [:stephend] 2006-10-15 09:32:00 PDT
Verified FIXED using trunk build 2006-10-15-06 of SeaMonkey under Windows XP; I've been enjoying this for a while.  Thanks for porting it!

Note You need to log in before you can comment on or make changes to this bug.