Closed Bug 338318 Opened 14 years ago Closed 13 years ago

UI for spellcheck in browser window (form fields)

Categories

(SeaMonkey :: UI Design, enhancement)

enhancement
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: jasonb, Assigned: iann_bugzilla)

References

Details

(Keywords: fixed-seamonkey1.1b, fixed1.8.1, Whiteboard: [verified-seamonkey1.1b])

Attachments

(3 files, 2 obsolete files)

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.
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
Summary: invoke spell check in browser window (multiple form fields) → UI for spell check in browser window (form fields)
That Firefox frontend patch for this functionality is attachment 203956 [details] [diff] [review] - we probably want that ported to SeaMonkey browser at least
Depends on: 302050
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).
Keywords: helpwanted
Summary: UI for spell check in browser window (form fields) → UI for spellcheck in browser window (form fields)
Flags: blocking-seamonkey1.1b?
Attached patch First draft of patch v0.1 (obsolete) — Splinter Review
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
Assignee: guifeatures → iann_bugzilla
Status: NEW → ASSIGNED
Attachment #238915 - Flags: review?(neil)
Keywords: helpwanted
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.
Attachment #238915 - Flags: review?(neil) → review+
Attached patch Return early patch v0.1a (obsolete) — Splinter Review
Changes since v0.1:
* Moved editable document earlier so could return early.
* Use window.openDialog instead of window.open
* css changes now in themes
Attachment #238915 - Attachment is obsolete: true
Attachment #239574 - Flags: review?(neil)
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? :-(
Attachment #239574 - Flags: review?(neil) → review+
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?
(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.
Changes since v0.1a:
* xpfe versions of help file and browser-prefs.js included.
* move var within try.

Carrying forward r=
Attachment #239574 - Attachment is obsolete: true
Attachment #239730 - Flags: superreview?(jag)
Attachment #239730 - Flags: review+
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?
Attachment #239730 - Flags: superreview?(jag) → superreview+
(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 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
Attachment #239730 - Attachment description: xpfe as well as suite patch v0.1b → xpfe as well as suite patch v0.1b (Checked into trunk)
Comment on attachment 239730 [details] [diff] [review]
xpfe as well as suite patch v0.1b (Checked into trunk)

Requesting approval for SM1.1b
Attachment #239730 - Flags: approval-seamonkey1.1b?
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.
Attachment #239730 - Flags: approval-seamonkey1.1b? → approval-seamonkey1.1b+
Hardware: PC → All
Branch version of patch v0.1b
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
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.
Blocks: 354289
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.
This patch:
* Adds missing locale changes
Attachment #240202 - Flags: superreview?(bienvenu)
Attachment #240202 - Flags: review?(mscott)
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
Attachment #240202 - Flags: superreview?(bienvenu)
Attachment #240202 - Flags: superreview+
Attachment #240202 - Flags: review?(mscott)
Attachment #240202 - Flags: review+
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
Attachment #240202 - Attachment description: Regression fix for branch TB v0.1c → Regression fix for branch TB v0.1c (Checked into 1.8 branch)
Attachment #240064 - Attachment description: Branch patch v0.1b → Branch patch v0.1b (Checked into 1.8 branch)
[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.
Flags: blocking-seamonkey1.1b?
Whiteboard: [verified-seamonkey1.1b]
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!
Status: RESOLVED → VERIFIED
Depends on: 370436
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.