484181 - spellcheck broken in contenteditable DIV with preceding empty DIV within a DIV

Status: VERIFIED FIXED

(Core :: Spelling checker, defect)

Description

[Kevin Field]

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]

Attachment: testcase from url

Comment 2

[Kevin Field]

Ah, another detail.  Thanks.  I didn't notice that, since in our context the editor always starts empty (for comment-adding).

Comment 3

[Douglas]

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]

Requesting blocking do to this occurring on facebook with the status update input field.

Comment 5

[Douglas]

Attachment: 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]

Does anybody know if this is a regression from Firefox 3.1 or 3.5?

Comment 7

[u88484]

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]]

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][⌚️UTC+1]]

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]

(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]

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][⌚️UTC+1]]

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][⌚️UTC+1]]

Dupe of bug 433860?

Comment 14

[u88484]

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][⌚️UTC+1]]

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.

Comment 16

[Douglas]

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][⌚️UTC+1]]

Martijn, could you cross-check please? I believe it's the same underlying issue.

Comment 18

[Howie]

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]

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]

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]]

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]]

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]

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]]

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]

(Ah, good to know, Jeff.  Thanks.)

Comment 27

[Jeff Martens]

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)]

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)]

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)]

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

[(no longer active)]

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

[(no longer active)]

Attachment: 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

[(no longer active)]

Oh, I forgot to mention that this patch fixes the problem as visible on facebook.com.

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Agree that this should block, also should be back-ported to any and all branches that exhibit it.

Comment 35

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/fc0f91d19dbf

Comment 36

[(no longer active)]

This doesn't have anything to do with bug 433860.  Removing the bogus dependency.

Comment 37

[(no longer active)]

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]

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]]

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 40

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/09df590981f0
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d515226b223f

Comment 42

[Tony Chung [:tchung]]

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 - ex-MoCo]]

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.