Last Comment Bug 433860 - No spelling suggestions for text inputs when contenteditable node in document
: No spelling suggestions for text inputs when contenteditable node in document
Status: VERIFIED FIXED
: testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: P3 normal with 2 votes (vote)
: mozilla1.9.3a5
Assigned To: :Ehsan Akhgari
:
Mentors:
: 562576 (view as bug list)
Depends on: 569504 570321
Blocks: contenteditable 593575
  Show dependency treegraph
 
Reported: 2008-05-15 04:57 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2010-09-04 04:42 PDT (History)
16 users (show)
roc: blocking1.9.2-
roc: wanted1.9.2+
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.5+
.5-fixed
.11+
.11-fixed


Attachments
testcase (276 bytes, text/html)
2008-05-15 04:57 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
v1 (894 bytes, patch)
2008-05-17 04:20 PDT, Peter Van der Beken [:peterv] - away till Aug 1st
no flags Details | Diff | Splinter Review
Patch (v1) (12.95 KB, patch)
2010-06-01 11:03 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v1) (14.40 KB, patch)
2010-06-01 11:09 PDT, :Ehsan Akhgari
gavin.sharp: review+
mbeltzner: approval1.9.2.5+
mbeltzner: approval1.9.1.11+
Details | Diff | Splinter Review
post-fix screenshot (44.83 KB, image/png)
2010-07-01 12:47 PDT, krupa raj[:krupa]
no flags Details

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2008-05-15 04:57:26 PDT
Created attachment 321056 [details]
testcase

See testcase, when right-clicking on the text in the input or the textarea, you don't get spelling suggestions for the misspelled word.
Also, the "Check Spelling" option is checked by default. It seems to work for the contenteditable node only.

As a sidenote (this should probably be a new bug), the text input and the textarea can get a selected look, when you select it. This should not happen.
It doesn't happen anymore when you've focused the text input or the textarea and select afterwards.
Comment 1 Peter Van der Beken [:peterv] - away till Aug 1st 2008-05-17 04:20:07 PDT
Created attachment 321372 [details] [diff] [review]
v1
Comment 2 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2009-07-30 12:48:22 PDT
Peter, is this patch still good?  Any plans to get it in the tree?
Comment 3 Henrik Skupin (:whimboo) 2009-11-16 03:22:54 PST
*** Bug 484181 has been marked as a duplicate of this bug. ***
Comment 4 Henrik Skupin (:whimboo) 2009-11-16 03:26:03 PST
Looks like that patch has been forgotten. I have send it to the tryserver to check if it still applies and fixes this particular issue.
Comment 5 Peter Van der Beken [:peterv] - away till Aug 1st 2009-11-16 03:46:43 PST
Not so much forgotten, I just haven't had the time to check whether it is the right fix. (Without an automated testcase sending it to try isn't really useful either)
Comment 6 u88484 2009-11-16 05:01:11 PST
Requesting blocking as this bug happens on facebook in the status update input field.  I believe they switched to a contenteditable field in the last couple of months.  We confirmed that this isn't a regression as it appears in the build the patch for bug 237964 was included in the 2007-06-28 nightly builds.

Should we add top50 as a keyword since facebook.com is #1 ranked sitein many countries according to alexa.com?  I'm not sure if that is reserved for only specific site bugs or not.
Comment 7 Henrik Skupin (:whimboo) 2009-11-16 05:20:39 PST
(In reply to comment #5)
> Not so much forgotten, I just haven't had the time to check whether it is the
> right fix. (Without an automated testcase sending it to try isn't really useful
> either)

Why it's not really useful either? We can at least test manually and check it against existing testcases like we have on bug 484181.

Try server builds are available here: http://build.mozilla.org/tryserver-builds/hskupin@mozilla.com-bug433860-spellchecker

Running a quick check with attachment 411174 [details] on Windows shows me that we still suffer from the problem even with the proposed patch applied. Given that I would assume that it's not the right fix.
Comment 8 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2009-11-17 14:14:45 PST
Maybe somebody could write an automated testcase?
Comment 9 Henrik Skupin (:whimboo) 2010-01-18 06:28:45 PST
(In reply to comment #8)
> Maybe somebody could write an automated testcase?

Cristian Klein was working recently on a fix and test for bug 370436. CC'ing him and ask if he has an interest for this bug.
Comment 10 Cristian KLEIN 2010-01-20 15:09:54 PST
(In reply to comment #9)
> (In reply to comment #8)
> > Maybe somebody could write an automated testcase?
> 
> Cristian Klein was working recently on a fix and test for bug 370436. CC'ing
> him and ask if he has an interest for this bug.

Hello,

At first glance, the bug seems more difficult than the one I fixed. I won't have time until the end of next week to look more into it.

OTOH, I found the following comment in extensions/spellcheck/src/mozInlineSpellWordUtil.cpp @ 113:

// Find the root node for the editor. For contenteditable we'll need something
// cleverer here.

Might this be the key to solving this bug?
Comment 11 Andrew Hurle [:ahurle] 2010-05-02 10:57:25 PDT
*** Bug 562576 has been marked as a duplicate of this bug. ***
Comment 12 :Ehsan Akhgari 2010-06-01 11:03:36 PDT
Created attachment 448562 [details] [diff] [review]
Patch (v1)

Peterv's approach was correct.  I basically took his patch and added a unit test for input[spellcheck="true"], textareas and contenteditables.  I had to tweak the test_contextmenu.html checks a bit to make things work for testing spell check related items in the menu.
Comment 13 :Ehsan Akhgari 2010-06-01 11:09:47 PDT
Created attachment 448563 [details] [diff] [review]
Patch (v1)

I wish there were a way for bugzilla to check whether I've qrefreshed before attaching a patch.  :-)
Comment 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-06-01 17:05:41 PDT
Comment on attachment 448563 [details] [diff] [review]
Patch (v1)

- let's rename possibleSpellChecking to onEditableArea
- file a followup to remove the canSpellCheck and showItem lines in this contentEditable block (they are redundant - we later end up calling initSpellingItems() which does the same work)
- file a followup to make sure that we're resetting all the flags we need to in that block, or to fix things so that we don't have to.
Comment 15 :Ehsan Akhgari 2010-06-02 11:29:36 PDT
(In reply to comment #14)
> (From update of attachment 448563 [details] [diff] [review])
> - let's rename possibleSpellChecking to onEditableArea

Done.

> - file a followup to remove the canSpellCheck and showItem lines in this
> contentEditable block (they are redundant - we later end up calling
> initSpellingItems() which does the same work)

Bug 569658.

> - file a followup to make sure that we're resetting all the flags we need to in
> that block, or to fix things so that we don't have to.

Bug 569659.
Comment 17 Mike Beltzner [:beltzner, not reading bugmail] 2010-06-03 12:12:19 PDT
We can haz backport? Does this affect 1.9.1 as well?
Comment 18 :Ehsan Akhgari 2010-06-03 16:57:31 PDT
Comment on attachment 448563 [details] [diff] [review]
Patch (v1)

(In reply to comment #17)
> We can haz backport?

Sure.  The test needs bug 569504 though (which I think we should take on branch regardless of this bug).

> Does this affect 1.9.1 as well?

Yes.
Comment 19 Mike Beltzner [:beltzner, not reading bugmail] 2010-06-04 06:32:34 PDT
Comment on attachment 448563 [details] [diff] [review]
Patch (v1)

a=beltzner for mozilla-1.9.1 default and mozilla-1.9.2 default, fixes a problem that affects Facebook due to a change in their code. Needs to land with bug 569504
Comment 20 :Ehsan Akhgari 2010-06-04 10:18:02 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7aa0256a2b80
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d584cd712136

For some reason I thought we're at 1.9.1.15 while checking in.  :/
Comment 21 :Ehsan Akhgari 2010-06-04 11:36:09 PDT
Backed out from 1.9.1 due to mochitest failures:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d1168b311bae

s: win32-slave45
1457 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #0 (*chubbiness) name - got "context-undo", expected "*chubbiness"
Bug 513558 - mochitest-plain: intermittent "test_contextmenu.html | checking item #10 (context-selectall) enabled state - got false, expected true" Bug 508472 - intermittent failures in test_contextmenu.html | menuitem has an access key 1458 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #0 (*chubbiness) enabled state - got false, expected true
1459 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #1 (spell-add-to-dictionary) name - got "---", expected "spell-add-to-dictionary"
1460 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #1 (spell-add-to-dictionary) enabled state - got null, expected true
1461 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #2 (---) name - got "context-cut", expected "---"
1462 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #3 (context-undo) name - got "context-copy", expected "context-undo"
1464 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #4 (---) name - got "context-paste", expected "---"
1465 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #5 (context-cut) name - got "context-delete", expected "context-cut"
1467 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #6 (context-copy) name - got "---", expected "context-copy"
1468 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #6 (context-copy) enabled state - got null, expected false
1469 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #7 (context-paste) name - got "context-selectall", expected "context-paste"
1470 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #8 (context-delete) name - got "---", expected "context-delete"
1471 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #8 (context-delete) enabled state - got null, expected false
1472 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #9 (---) name - got "spell-check-enabled", expected "---"
1473 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #10 (context-selectall) name - got "spell-dictionaries", expected "context-selectall"
1475 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #11 (---) name - got [], expected "---"
1476 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #12 (spell-check-enabled) name - got undefined, expected "spell-check-enabled"
1477 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #12 (spell-check-enabled) enabled state - got undefined, expected true
1478 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #13 (spell-dictionaries) name - got undefined, expected "spell-dictionaries"
1479 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #13 (spell-dictionaries) enabled state - got undefined, expected true
1492 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking expected number of menu entries - got 24, expected 30
1515 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #0 (spell-no-suggestions) name - got "context-undo", expected "spell-no-suggestions"
1517 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #1 (spell-add-to-dictionary) name - got "---", expected "spell-add-to-dictionary"
1518 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #1 (spell-add-to-dictionary) enabled state - got null, expected true
1519 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #2 (---) name - got "context-cut", expected "---"
1520 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #3 (context-undo) name - got "context-copy", expected "context-undo"
1522 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #4 (---) name - got "context-paste", expected "---"
1523 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #5 (context-cut) name - got "context-delete", expected "context-cut"
1525 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #6 (context-copy) name - got "---", expected "context-copy"
1526 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #6 (context-copy) enabled state - got null, expected false
1527 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #7 (context-paste) name - got "context-selectall", expected "context-paste"
1528 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #8 (context-delete) name - got "---", expected "context-delete"
1529 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #8 (context-delete) enabled state - got null, expected false
1530 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #9 (---) name - got "spell-check-enabled", expected "---"
1531 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #10 (context-selectall) name - got "spell-dictionaries", expected "context-selectall"
1533 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #11 (---) name - got [], expected "---"
1534 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #12 (spell-check-enabled) name - got undefined, expected "spell-check-enabled"
1535 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #12 (spell-check-enabled) enabled state - got undefined, expected true
1536 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #13 (spell-dictionaries) name - got undefined, expected "spell-dictionaries"
1537 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #13 (spell-dictionaries) enabled state - got undefined, expected true
1550 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking expected number of menu entries - got 24, expected 30
1573 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #0 (*prodigality) name - got "context-undo", expected "*prodigality"
1574 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #0 (*prodigality) enabled state - got false, expected true
1575 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #1 (spell-add-to-dictionary) name - got "---", expected "spell-add-to-dictionary"
1576 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #1 (spell-add-to-dictionary) enabled state - got null, expected true
1577 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #2 (---) name - got "context-cut", expected "---"
1578 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #3 (context-undo) name - got "context-copy", expected "context-undo"
1580 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #4 (---) name - got "context-paste", expected "---"
1581 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #5 (context-cut) name - got "context-delete", expected "context-cut"
1583 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #6 (context-copy) name - got "---", expected "context-copy"
1584 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #6 (context-copy) enabled state - got null, expected false
1585 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #7 (context-paste) name - got "context-selectall", expected "context-paste"
1586 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #8 (context-delete) name - got "---", expected "context-delete"
1587 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #8 (context-delete) enabled state - got null, expected false
1588 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #9 (---) name - got "spell-check-enabled", expected "---"
1589 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #10 (context-selectall) name - got "spell-dictionaries", expected "context-selectall"
1591 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #11 (---) name - got [], expected "---"
1592 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #12 (spell-check-enabled) name - got undefined, expected "spell-check-enabled"
1593 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #12 (spell-check-enabled) enabled state - got undefined, expected true
1594 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #13 (spell-dictionaries) name - got undefined, expected "spell-dictionaries"
1595 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking item #13 (spell-dictionaries) enabled state - got undefined, expected true
1608 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | checking expected number of menu entries - got 24, expected 30
Comment 22 neil@parkwaycc.co.uk 2010-06-07 01:54:14 PDT
Comment on attachment 448563 [details] [diff] [review]
Patch (v1)

>       var item = menu.ownerDocument.createElement("menuitem");
>-      item.setAttribute("label", displayName);
>+      item.setAttribute("id", "spell-check-dictionary-" + list[i]);
>+      item.label = displayName;
The label attribute/property change is wrong since there's no XBL yet.
[Ironically you could have used the id property, since that's in IDL.]
Comment 23 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-06-07 04:07:07 PDT
That's bug 570321.
Comment 24 :Ehsan Akhgari 2010-06-13 14:07:43 PDT
On 1.9.1, the test doesn't correctly fire the context menu event, so the popup parent range and offset are not set, therefore the spelling suggestions are not hooked to the context menu.  Therefore, testing this on 1.9.1 is not possible in the test suite.

I verified that the patch fixes the bug on 1.9.1 manually, and landed it without the test:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9141113ce7c1
Comment 25 krupa raj[:krupa] 2010-07-01 12:47:13 PDT
verified fixed in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.7pre) Gecko/20100701 Namoroka/3.6.7pre
Comment 26 krupa raj[:krupa] 2010-07-01 12:47:52 PDT
Created attachment 455522 [details]
post-fix screenshot

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