Closed Bug 908570 Opened 11 years ago Closed 8 years ago

no longer able to check spelling on etherpad - only "add dictionary" is present

Categories

(Firefox :: Menus, defect)

Product:
Firefox
Component:
Menus
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: glob, Assigned: ehsan.akhgari)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Screen Shot 2013-08-23 at 1.12.20 PM.png
33.09 KB, image/png
Details
Don't show the 'Add Dictionaries' menu item for contenteditable elements with spellcheck=false
3.92 KB, patch
dao
: review+
Details | Diff | Splinter Review 
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0

i'm no longer able to check spelling on any pad on etherpad.mozilla.org (perhaps due to the #innerdocbody having spellcheck="false").

in fx23, this results in "check spelling" being unchecked by default in the context menu, and checking it enables spellcheck.

in fx26, "check spelling" isn't present, instead i get "add dictionary".  spell checking works on every other field (including the chat input field on the same etherpad page).
It looks like this was an intentional change by bug 905176.  Ehsan, can you verify?

Some inspecting shows that the document you edit in Etherpad is a <body spellcheck="false"> (id="innerdocbody", nested two iframes deep).  isContentEditable on that body and its children is false, though, so I'm not sure how it's actually editable.
Status: UNCONFIRMED → NEW
Component: Untriaged → Menus
Ever confirmed: true
Flags: needinfo?(ehsan)
even if it were intentional, "add dictionary" is an incorrect replacement, as it implies that it's possible to check spelling, but you first have to add a dictionary.

if this behaviour is intention (sadface.jpg), i think it would be better to display and disable the menuitem.
(In reply to Byron Jones ‹:glob› from comment #2)
> even if it were intentional, "add dictionary" is an incorrect replacement,
> as it implies that it's possible to check spelling, but you first have to
> add a dictionary.

Hmm, yeah, it seems like onMisspelling should always be false if canSpell is false: <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#354>.

> if this behaviour is intention (sadface.jpg), i think it would be better to
> display and disable the menuitem.

We usually hide the menu items which will never be enabled because of a user's action.

Note that we've just changed our behavior to respect the website's wishes about whether something should be spell checked or not.  Etherpad having spellcheck=false is really what's broken here.  Any idea how we can report that bug somewhere that it can get fixed?
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> Note that we've just changed our behavior to respect the website's wishes
> about whether something should be spell checked or not.  Etherpad having
> spellcheck=false is really what's broken here.  Any idea how we can report
> that bug somewhere that it can get fixed?

Filed bug 909352.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> (In reply to Byron Jones ‹:glob› from comment #2)
> > even if it were intentional, "add dictionary" is an incorrect replacement,
> > as it implies that it's possible to check spelling, but you first have to
> > add a dictionary.
> 
> Hmm, yeah, it seems like onMisspelling should always be false if canSpell is
> false:
> <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> nsContextMenu.js#354>.

Hold on, I misread this as "Add to Dictionary".  The "Add Dictionaries" menu item is shown unconditionally and I don't think there is anything wrong with doing that.

Is there anything to be done in this bug?
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> Hold on, I misread this as "Add to Dictionary".  The "Add Dictionaries" menu
> item is shown unconditionally and I don't think there is anything wrong with
> doing that.

ah.

my confusion stems from it being displayed differently when compared to a context menu on an item with spell checking enabled.  on such items there's a "languages" submenu which lists my installed dictionaries, as well as the "add dictionary" menuitem.

if you're happy with that situation, then i guess this is a wontfix or invalid.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> Hold on, I misread this as "Add to Dictionary".  The "Add Dictionaries" menu
> item is shown unconditionally and I don't think there is anything wrong with
> doing that.

It's not shown unconditionally but only for text input fields. Now, why should we show it for text input fields without spell checking but not for other areas on the page? This seems like an arbitrary distinction. It seems to me that this should just depend on whether spell checking is enabled.
(In reply to Byron Jones ‹:glob› from comment #6)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> > Hold on, I misread this as "Add to Dictionary".  The "Add Dictionaries" menu
> > item is shown unconditionally and I don't think there is anything wrong with
> > doing that.
> 
> ah.
> 
> my confusion stems from it being displayed differently when compared to a
> context menu on an item with spell checking enabled.  on such items there's
> a "languages" submenu which lists my installed dictionaries, as well as the
> "add dictionary" menuitem.

Yes, that's right.

> if you're happy with that situation, then i guess this is a wontfix or
> invalid.

I'm ambivalent to be honest, this is just how our menu used to work, and in a way, I didn't really change the behavior here.

(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> > Hold on, I misread this as "Add to Dictionary".  The "Add Dictionaries" menu
> > item is shown unconditionally and I don't think there is anything wrong with
> > doing that.
> 
> It's not shown unconditionally but only for text input fields.

Nope, it is shown unconditionally for all editable fields <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#386>.  It's just that we sometimes hide the item in the main menu and show the one in the Languages submenu.

> Now, why
> should we show it for text input fields without spell checking but not for
> other areas on the page? This seems like an arbitrary distinction. It seems
> to me that this should just depend on whether spell checking is enabled.

You mean showing it even for non-editable content?  I doubt that is a good idea.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #8)
> > It's not shown unconditionally but only for text input fields.
> 
> Nope, it is shown unconditionally for all editable fields
> <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> nsContextMenu.js#386>.

That's what I meant. The menu item is context sensitive, "is this thing an editable field?" is its condition... The question is, does this condition make sense?

> > Now, why
> > should we show it for text input fields without spell checking but not for
> > other areas on the page? This seems like an arbitrary distinction. It seems
> > to me that this should just depend on whether spell checking is enabled.
> 
> You mean showing it even for non-editable content?  I doubt that is a good
> idea.

So do I, but it would be consistent with showing it for fields that don't deal with spell checking. Why are we showing the menu item for those?
(In reply to comment #9)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #8)
> > > It's not shown unconditionally but only for text input fields.
> > 
> > Nope, it is shown unconditionally for all editable fields
> > <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> > nsContextMenu.js#386>.
> 
> That's what I meant. The menu item is context sensitive, "is this thing an
> editable field?" is its condition... The question is, does this condition make
> sense?

Oh, I see.  I thought you're talking about only inputs and textareas.  Otherwise, I agree.

And I don't personally think that this menu item makes much sense.

> > > Now, why
> > > should we show it for text input fields without spell checking but not for
> > > other areas on the page? This seems like an arbitrary distinction. It seems
> > > to me that this should just depend on whether spell checking is enabled.
> > 
> > You mean showing it even for non-editable content?  I doubt that is a good
> > idea.
> 
> So do I, but it would be consistent with showing it for fields that don't deal
> with spell checking. Why are we showing the menu item for those?

What do you mean by fields that don't deal with spell checking?
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #10)
> > > You mean showing it even for non-editable content?  I doubt that is a good
> > > idea.
> > 
> > So do I, but it would be consistent with showing it for fields that don't deal
> > with spell checking. Why are we showing the menu item for those?
> 
> What do you mean by fields that don't deal with spell checking?

Fields with spellcheck="false" like on etherpad. It's unclear why users should add a dictionary this way, as they won't be able to use it there.
Attached patch WIP (obsolete) — — Splinter Review 
Like this?  (Note that this patch as it is is not good enough, there is an extra separator item showing, and I don't have time to figure out where it's coming from.)
Attachment #796649 - Flags: feedback?(dao)
Attachment #796649 - Flags: feedback?(dao+bmo) → feedback+
Depends on: 1246296
Assignee: nobody → ehsan
Attachment #796649 - Attachment is obsolete: true
Attachment #8802771 - Flags: review?(dao+bmo) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b3dc1102513
Don't show the 'Add Dictionaries…' menu item for contenteditable elements with spellcheck=false; r=dao
Backed bug 908570 and bug 1246296 for failing the modified test browser_contextmenu.js on OSX and Windows (in non-e10s mode):

https://hg.mozilla.org/integration/mozilla-inbound/rev/a7281f6c576a8980975037278f51b11f5eaba15a
https://hg.mozilla.org/integration/mozilla-inbound/rev/69974770e8a63a412561f0490622c6717f749194

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6b3dc11025136983d14bff43dc3b483fc25832d9
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=37980591&repo=mozilla-inbound

08:49:14     INFO -  107 INFO TEST-PASS | browser/base/content/test/general/browser_contextmenu.js | checking item #6 (---) name -
08:49:14     INFO -  108 INFO TEST-PASS | browser/base/content/test/general/browser_contextmenu.js | checking item #7 (context-selectall) name -
08:49:14     INFO -  109 INFO TEST-PASS | browser/base/content/test/general/browser_contextmenu.js | checking item #7 (context-selectall) enabled state -
08:49:14     INFO -  110 INFO TEST-PASS | browser/base/content/test/general/browser_contextmenu.js | checking item #8 (---) name -
08:49:14     INFO -  111 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_contextmenu.js | checking item #9 (spell-check-enabled) name - Got context-inspect, expected spell-check-enabled
08:49:14     INFO -  Stack trace:
08:49:14     INFO -      chrome://mochikit/content/browser-test.js:test_is:909
08:49:14     INFO -      chrome://mochitests/content/browser/browser/base/content/test/general/contextmenu_common.js:checkMenuItem:138
08:49:14     INFO -      chrome://mochitests/content/browser/browser/base/content/test/general/contextmenu_common.js:checkMenu:215
08:49:14     INFO -      chrome://mochitests/content/browser/browser/base/content/test/general/contextmenu_common.js:checkContextMenu:134
08:49:14     INFO -      chrome://mochitests/content/browser/browser/base/content/test/general/contextmenu_common.js:test_contextmenu:312
08:49:14     INFO -      Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:937:23
Flags: needinfo?(ehsan)
This one is innocent.  Investigating in the other bug.
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e02e34d291ca
Don't show the 'Add Dictionaries…' menu item for contenteditable elements with spellcheck=false; r=dao
https://hg.mozilla.org/mozilla-central/rev/e02e34d291ca
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
No longer depends on: 1349745
Regressions: 1349745
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: