Closed
Bug 338318
Opened 19 years ago
Closed 18 years ago
UI for spellcheck in browser window (form fields)
Categories
(SeaMonkey :: UI Design, enhancement)
SeaMonkey
UI Design
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: jasonb, Assigned: iannbugzilla)
References
Details
(Keywords: fixed-seamonkey1.1b, fixed1.8.1, Whiteboard: [verified-seamonkey1.1b])
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
iannbugzilla
:
review+
jag+mozilla
:
superreview+
kairo
:
approval-seamonkey1.1b+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
mscott
:
review+
mscott
:
superreview+
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
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)
Comment 2•18 years ago
|
||
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•18 years ago
|
||
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
Updated•18 years ago
|
Summary: UI for spell check in browser window (form fields) → UI for spellcheck in browser window (form fields)
Updated•18 years ago
|
Flags: blocking-seamonkey1.1b?
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 5•18 years ago
|
||
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+
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 7•18 years ago
|
||
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 8•18 years ago
|
||
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•18 years ago
|
||
(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.
Assignee | ||
Comment 10•18 years ago
|
||
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 11•18 years ago
|
||
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+
Assignee | ||
Comment 12•18 years ago
|
||
(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.
Assignee | ||
Comment 13•18 years ago
|
||
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)
Assignee | ||
Comment 14•18 years ago
|
||
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 15•18 years ago
|
||
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+
Updated•18 years ago
|
Hardware: PC → All
Assignee | ||
Comment 16•18 years ago
|
||
Branch version of patch v0.1b
Assignee | ||
Comment 17•18 years ago
|
||
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: 18 years ago
Keywords: fixed-seamonkey1.1b,
fixed1.8.1
Resolution: --- → FIXED
Comment 18•18 years ago
|
||
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
Comment 19•18 years ago
|
||
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.
Assignee | ||
Comment 20•18 years ago
|
||
This patch:
* Adds missing locale changes
Attachment #240202 -
Flags: superreview?(bienvenu)
Attachment #240202 -
Flags: review?(mscott)
Comment 21•18 years ago
|
||
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+
Assignee | ||
Comment 22•18 years ago
|
||
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)
Comment 23•18 years ago
|
||
[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
You need to log in
before you can comment on or make changes to this bug.
Description
•