Last Comment Bug 484181 - spellcheck broken in contenteditable DIV with preceding empty DIV within a DIV
: spellcheck broken in contenteditable DIV with preceding empty DIV within a DIV
Status: VERIFIED FIXED
: verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Spelling checker (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: mozilla1.9.3a5
Assigned To: :Ehsan Akhgari
:
Mentors:
http://brantaero.com/lack/spellcheck....
: 536461 541555 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-19 05:58 PDT by Kevin Field
Modified: 2010-07-16 16:17 PDT (History)
23 users (show)
mbeltzner: blocking1.9.2-
mbeltzner: wanted1.9.2+
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
alpha5+
needed
.5-fixed
.11-fixed


Attachments
testcase from url (443 bytes, text/html)
2009-03-19 06:21 PDT, Kevin Brosnan
no flags Details
Spellcheck broken with <a> and <p> but fine with <span> (1.26 KB, text/html)
2009-11-09 05:12 PST, Douglas
no flags Details
Patch (v1) (4.87 KB, patch)
2010-05-31 14:42 PDT, :Ehsan Akhgari
roc: review+
dveditz: approval1.9.2.5+
dveditz: approval1.9.1.11+
Details | Diff | Splinter Review

Description Kevin Field 2009-03-19 05:58:34 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7

See the source of the example page for details.

By "spellcheck broken" I mean that no spell checking occurs, even if you have that layout setting in about:config set to 1 or 2, and despite the fact that if you right click there's a check mark beside "Check Spelling."  To get it to work, you have to uncheck that context menu entry and recheck it.  Hardly convenient.

This may seem like a "who cares?" kind of thing but it broke my web app.  In the original context, that empty DIV was actually nested in another DIV, holding a bunch of control buttons (bold, italic, etc.), and was there for AJAX info to be loaded into, and also, I was using a transitional doctype.  But the sample URL is the most isolated version of this bug that I could show you.

Reproducible: Always

Steps to Reproduce:
1. Type a misspelled word into both boxes.
2. Right-click in the box that's not highlighting it, and uncheck "Check Spelling".
3. Right-click there again, and recheck "Check Spelling".
4. Notice that this is unnecessary in the other box, all because the DIV before the editable div has a space in it rather than being empty.
Actual Results:  
The two boxes differ in behaviour.

Expected Results:  
The two boxes' spell-checking ability should not be affected by the content of surrounding DIVs, and should not look in the context menu as if they're working when they aren't.
Comment 1 Kevin Brosnan 2009-03-19 06:21:24 PDT
Created attachment 368244 [details]
testcase from url
Comment 2 Kevin Field 2009-03-19 06:35:18 PDT
Ah, another detail.  Thanks.  I didn't notice that, since in our context the editor always starts empty (for comment-adding).
Comment 3 Douglas 2009-11-02 06:33:04 PST
I also see this bug in

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.0.15) Gecko/2009101601 Firefox/3.0.15 (.NET CLR 3.5.30729)
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.9.1.4) Gecko/20091016 Firefox/3.5.4 (.NET CLR 3.5.30729)

This also happens in my xulrunner 1.9.1 based application.

Further details: spell check in the first box in the testcase is only broken on the first line. If you press return and continue to type a second paragraph, spell check is fine.
Comment 4 u88484 2009-11-05 15:36:32 PST
Requesting blocking do to this occurring on facebook with the status update input field.
Comment 5 Douglas 2009-11-09 05:12:41 PST
Created attachment 411174 [details]
Spellcheck broken with <a> and <p> but fine with <span>

After further investigation, this is not just a problem with preceding divs. Spelling will also break with empty preceding anchor or paragraph tags.

It is fine if there is an empty span preceding the contenteditable div though.
Comment 6 u88484 2009-11-09 05:58:35 PST
Does anybody know if this is a regression from Firefox 3.1 or 3.5?
Comment 7 u88484 2009-11-11 14:44:46 PST
CC'ing Mike Beltzner.  I believe he is one of the 3.6 driver and this is potentially a huge issue on a lot of sites.
Comment 8 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-13 10:23:58 PST
Comment 4 is a little concerning, though I just went and tested Firefox 3 and Firefox 3.5 and they both seem to fail on Facebook now, too, so I don't think this is a regression, just a case we're not handling.

cc'ing some folks on the Firefox team to see if we can figure out who's best to fix this.
Comment 9 Henrik Skupin (:whimboo) 2009-11-13 10:57:55 PST
So the problem is that the spellchecker only checks the given default values but do not perform any action for user entered content. Is that right? Someone has to disable and enable the spellchecker to get the newly added content checked.

If it is a regression then it's really old. Kurt, would you have time to check with Firefox 2 or even 1.5 or 1.0?
Comment 10 u88484 2009-11-14 08:56:17 PST
(In reply to comment #9)
> Someone has to disable and enable the spellchecker to get the newly added
> content checked.
>
Correct.  On facebook, I can type asdf and the squiggly lines do not show and neither do suggestions when I right click on the misspelled word.  If I toggle 'Check Spelling' in the context menu to off and then on again, spell checking works but for only for whatever was already typed.

> If it is a regression then it's really old. Kurt, would you have time to check
> with Firefox 2 or even 1.5 or 1.0?

Spelling checking WFM on facebook using 2.0.0.20 and 3.5.3.  I just updated the 3.5.3 build to 3.5.5 and then spelling checking doesn't work.
Comment 11 u88484 2009-11-14 09:11:00 PST
Hmm, just tried again with all new profiles and 2.0.0.20 is the only build that worked for me.  3.0.15 and 3.5.5 do not work.  So this looks to be a regression from the 2.0 but I don't think content editable was even in 2.0.  Looks to be in 1.9 - bug 237964
Comment 12 Henrik Skupin (:whimboo) 2009-11-15 14:20:32 PST
The checkin on 1.9.0 happened on 2007-06-27 19:48. Could someone please try nightly builds from 2007-06-27 and 2007-06-28 if it's in this window?
Comment 13 Henrik Skupin (:whimboo) 2009-11-15 14:23:45 PST
Dupe of bug 433860?
Comment 14 u88484 2009-11-15 17:08:16 PST
Using the 2007062711 build, when clicking inside the facebook status update field, the caret does not appear and no text can be entered. Same thing with the testcases here.  I spoofed my UA to 3.0.5 and 3.5.5 just to also see if facebook serve different content but they didn't.

Using the 2009062805 build, the testcases and facebook do the same thing today's trunk build do.

First time facebook worked correctly so I tested and then had the same result as reported earlier in this bug.  I also came across that during testing the other day.
Comment 15 Henrik Skupin (:whimboo) 2009-11-16 03:22:54 PST
Thanks Kurt. I strongly believe we can mark this as a dupe of bug 433860. There is a patch I will send to the tryserver so we can check if it fixes the problem.

*** This bug has been marked as a duplicate of bug 433860 ***
Comment 16 Douglas 2009-11-29 14:00:50 PST
This is not a duplicate of bug 433860. That bug concerns missing suggestions in the context menu for input boxes. This bug concerns spelling mistakes not being detected at all (not even a red underline) inside content editable divs.
Comment 17 Henrik Skupin (:whimboo) 2009-12-01 03:51:02 PST
Martijn, could you cross-check please? I believe it's the same underlying issue.
Comment 18 Howie 2010-01-17 19:56:23 PST
Still not fixed within 3.6 RC.  I find this to plague the "Status" box within Facebook.  When one updates his status, the auto spellcheck does not work.  

Workaround: If one unchecks the "Check Spelling" (right click menu) option and then immediately "rechecks" the same option- with the cursor within the "Status" box- the spellcheck feature is activated.  This workaround probably works best after one has completely composed his updated FB status.

Is this a significant thing to fix?  It doesn't impact one's ability to type within the FB Status Update box, but becomes annoying to constantly have to manually invoke the spellcheck function for this one context box.
Comment 19 Kevin Field 2010-01-18 06:14:17 PST
Re: significance, I would say so.  It affects many web apps, but not all, so you develop a trust of the spell-checker being enabled, and then that trust is broken in certain contexts (but you have no idea beforehand which contexts those are.)  I was on Etherpad.com the other day and noticed a spelling error and that's when I noticed this bug affects it too.  (I think it's because they load the DIV contents via AJAX and do some post-processing, so as of the initial load, it's empty.)  Before that, I hadn't thought about it, so I had to go back and spell check all the documents we had been developing.

It's "merely" annoying assuming you even know to look for it...otherwise it's outright buggy and inconsistent-feeling, especially alongside how otherwise solid and polished FF generally is.
Comment 20 u88484 2010-01-18 08:25:04 PST
This lost it's blocking-1.9.2? flag when it was marked a dupe and therefore fell off the radar for it to be deemed worthy of blocking 1.9.2.  I doubt it will block at this point but it is a regression from prior releases.
Comment 21 Mike Connor [:mconnor] 2010-01-18 08:38:12 PST
Given that it's a regression since Firefox 2 (and thus, to users, has been broken since June 2008) I don't think it's a blocker.  We do have an active need to find a new spellchecker owner to drive this, however, and this is probably high on the list, along with bug 433860...
Comment 22 Mike Beltzner [:beltzner, not reading bugmail] 2010-01-18 10:53:56 PST
Wei: any chance you could look at this and see if Facebook might be able to change the markup for the status update field? The empty div seems redundant and is what's causing the breakage here, AIUI. Feel free to cc: someone more appropriate at Facebook if you're not the right person to talk to, here.
Comment 23 Kevin Field 2010-01-18 12:16:45 PST
Mike: Seems like fixing the wrong problem here.  There are legitimate use cases for empty tags preceding contenteditable DIVs, such as validators.  Of course there are always workarounds for it, but still...

In my particular web app interface to a legacy system, we are only able to append data and not edit it directly, so there's a DIV showing existing content (which may be blank) followed by a contenteditable DIV in which to compose additions.

If 'ask facebook' is the final take on it, maybe some documentation could be added to MDC or something so 'lesser' web app developers that don't have profile enough to be contacted directly about it won't be left out?
Comment 24 Jeff Walden [:Waldo] (remove +bmo to email) 2010-01-18 12:27:44 PST
It's not the final take; however, as an intermediate-time-frame hackaround until we can fix the actual bug, it would be sufficient to fix the problem for end users.
Comment 25 Kevin Field 2010-01-18 12:36:20 PST
(Ah, good to know, Jeff.  Thanks.)
Comment 26 Kevin Brosnan 2010-01-22 16:21:16 PST
*** Bug 541555 has been marked as a duplicate of this bug. ***
Comment 27 Jeff Martens 2010-02-24 20:48:27 PST
I don't see that getting Facebook to change their markup fixes anything. For example, this is an issue with the Blackboard courseware tool.
Comment 28 Damon Sicore (:damons) 2010-05-28 13:32:56 PDT
Looks like we need a real regression range from this to determine when it actually regressed.  Clearing the nom for now until we get this info.  If it is indeed a regression from 3.5, this should block.  

Assigning to Ehsan for now, but needs QA help to find the range.
Comment 29 Luke Iliffe (Harlequin99) 2010-05-29 16:41:34 PDT
I tried a build from 2007-06-27 (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2007/06/2007-06-27-11-trunk/)
The testcase in comment 1 was not even editable.
I tried the next windows build 2008-06-28 (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2007/06/2007-06-28-05-trunk/)
And the 1st text area in the testcase behaves as it says, i.e misspelled words are not detected. The 2nd text are they are detected.

This was all done on a new profile created for these builds on windows xp.
Hope this helps.
Comment 30 Luke Iliffe (Harlequin99) 2010-05-29 16:44:43 PDT
There was a typo in the second date in my comment above.

To clarify the testcase in commnent 1 wasn't editable in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070627 Minefield/3.0a6pre

It was editable but the spellcheck was broken in:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a6pre) Gecko/20070628 Minefield/3.0a6pre
Comment 31 :Ehsan Akhgari 2010-05-31 14:38:38 PDT
This is not a regression, it's just something which was broken since forever.

I have a patch for this which I will attach in a second.  Given the simplicity of the fix, and the visibility of the bug (on facebook's home page, for example), I think this should block.
Comment 32 :Ehsan Akhgari 2010-05-31 14:42:59 PDT
Created attachment 448428 [details] [diff] [review]
Patch (v1)

The problem happened because somewhere down the line from AfterEdit, we tried to delete empty nodes.  nsHTMLEditor::DeleteNode first checks to make sure that the node is actually modifiable (a.k.a inside an editable region) and if it's not, it just bails out with NS_ERROR_FAILURE.  We were seeing that error, and we just bailed out, and skipped calling into our spell checker engine.

This patch fixes that behavior by first checking to make sure if the node is modifiable before attempting to delete it.

I wrote a simple test for this which breaks without this patch to make sure that it doesn't regress in the future.
Comment 33 :Ehsan Akhgari 2010-05-31 14:45:53 PDT
Oh, I forgot to mention that this patch fixes the problem as visible on facebook.com.
Comment 34 Mike Beltzner [:beltzner, not reading bugmail] 2010-06-01 12:14:51 PDT
Agree that this should block, also should be back-ported to any and all branches that exhibit it.
Comment 36 :Ehsan Akhgari 2010-06-01 13:22:17 PDT
This doesn't have anything to do with bug 433860.  Removing the bogus dependency.
Comment 37 :Ehsan Akhgari 2010-06-01 13:22:54 PDT
Comment on attachment 448428 [details] [diff] [review]
Patch (v1)

I assume that we don't need this on 1.9.0.x.  :-)
Comment 38 u88484 2010-06-02 09:08:34 PDT
Verified fixed on facebook

Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100602 Minefield/3.7a5pre
Comment 39 Daniel Veditz [:dveditz] 2010-06-07 10:42:24 PDT
Comment on attachment 448428 [details] [diff] [review]
Patch (v1)

Approved for 1.9.2.5 and 1.9.1.11, a=dveditz for release-drivers
Comment 41 Matt Caywood 2010-06-25 21:53:00 PDT
*** Bug 536461 has been marked as a duplicate of this bug. ***
Comment 42 Tony Chung [:tchung] 2010-07-01 12:43:14 PDT
Verified fix on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.7) Gecko/20100701 Firefox/3.6.7 (.NET CLR 3.5.30729)
Comment 43 Al Billings [:abillings] 2010-07-16 16:17:47 PDT
Verified for 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.11) Gecko/20100701 Firefox/3.5.11 (.NET CLR 3.5.30729) using attached testcase. Also tested against 1.9.1.10 to see failure case.

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