The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.9.3a5

Status

()

Core
Spelling checker
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: Kevin Field, Assigned: Ehsan)

Tracking

({verified1.9.1, verified1.9.2})

Trunk
mozilla1.9.3a5
verified1.9.1, verified1.9.2
Points:
---
Bug Flags:
blocking1.9.2 -
wanted1.9.2 +
in-testsuite +

Firefox Tracking Flags

(blocking2.0 alpha5+, blocking1.9.2 needed, status1.9.2 .5-fixed, status1.9.1 .11-fixed)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

8 years ago
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.

Updated

8 years ago
Status: UNCONFIRMED → NEW
Component: General → Spelling checker
Ever confirmed: true
Product: Firefox → Core
QA Contact: general → spelling-checker

Comment 1

8 years ago
Created attachment 368244 [details]
testcase from url
(Reporter)

Comment 2

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

Comment 3

8 years ago
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

8 years ago
Requesting blocking do to this occurring on facebook with the status update input field.
Flags: blocking1.9.2?
OS: Windows Server 2003 → All
Version: unspecified → 1.9.2 Branch

Comment 5

8 years ago
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

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

Comment 7

8 years ago
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.
Hardware: x86 → All
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.
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

8 years ago
(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

8 years ago
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
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?
Dupe of bug 433860?

Comment 14

8 years ago
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.
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.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 433860
Flags: blocking1.9.2?

Comment 16

7 years ago
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.
Martijn, could you cross-check please? I believe it's the same underlying issue.

Updated

7 years ago
Status: RESOLVED → REOPENED
Depends on: 433860
Resolution: DUPLICATE → ---

Comment 18

7 years ago
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.
(Reporter)

Comment 19

7 years ago
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

7 years ago
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.
Flags: blocking1.9.2?
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...
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.
blocking2.0: --- → ?
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
(Reporter)

Comment 23

7 years ago
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?
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.
(Reporter)

Comment 25

7 years ago
(Ah, good to know, Jeff.  Thanks.)

Updated

7 years ago
Duplicate of this bug: 541555

Comment 27

7 years ago
I don't see that getting Facebook to change their markup fixes anything. For example, this is an issue with the Blackboard courseware tool.
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.
Assignee: nobody → ehsan
blocking2.0: ? → ---
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.
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
(Assignee)

Comment 31

7 years ago
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.
Status: REOPENED → ASSIGNED
blocking2.0: --- → ?
Version: 1.9.2 Branch → Trunk
(Assignee)

Comment 32

7 years ago
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.
Attachment #448428 - Flags: review?(roc)
(Assignee)

Comment 33

7 years ago
Oh, I forgot to mention that this patch fixes the problem as visible on facebook.com.
Attachment #448428 - Flags: review?(roc) → review+
Agree that this should block, also should be back-ported to any and all branches that exhibit it.
blocking1.9.2: --- → needed
blocking2.0: ? → alpha5+
(Assignee)

Comment 35

7 years ago
http://hg.mozilla.org/mozilla-central/rev/fc0f91d19dbf
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla1.9.3a5
(Assignee)

Comment 36

7 years ago
This doesn't have anything to do with bug 433860.  Removing the bogus dependency.
No longer depends on: 433860
(Assignee)

Comment 37

7 years ago
Comment on attachment 448428 [details] [diff] [review]
Patch (v1)

I assume that we don't need this on 1.9.0.x.  :-)
Attachment #448428 - Flags: approval1.9.2.5?
Attachment #448428 - Flags: approval1.9.1.11?

Comment 38

7 years ago
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
Status: RESOLVED → VERIFIED
Attachment #448428 - Flags: approval1.9.2.5?
Attachment #448428 - Flags: approval1.9.2.5+
Attachment #448428 - Flags: approval1.9.1.11?
Attachment #448428 - Flags: approval1.9.1.11+
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
(Assignee)

Comment 40

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/09df590981f0
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d515226b223f
status1.9.1: --- → .11-fixed
status1.9.2: --- → .5-fixed

Updated

7 years ago
Duplicate of this bug: 536461
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)
Keywords: verified1.9.2
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.
Keywords: verified1.9.1
You need to log in before you can comment on or make changes to this bug.