338318 - UI for spellcheck in browser window (form fields)

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, enhancement)

Description

[Jason Bassford]

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]

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]

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]

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]

Attachment: 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

[[e-mail]]

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]

Attachment: 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

[[e-mail]]

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

[[e-mail]]

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]

(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]

Attachment: 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)]

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]

(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]

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]

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]

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]

Attachment: Branch patch v0.1b (Checked into 1.8 branch)

Branch version of patch v0.1b

Comment 17

[Ian Neal]

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]

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]

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]

Attachment: Regression fix for branch TB v0.1c (Checked into 1.8 branch)

This patch:
* Adds missing locale changes

Comment 21

[Scott MacGregor]

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]

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)]

[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] Not actively reading bugmail]

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!