Closed Bug 674927 Opened 13 years ago Closed 11 years ago

contenteditable does not support spellcheck=false

Categories

(Core :: DOM: Editor, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: arno, Assigned: ehsan.akhgari)

References

Details

Attachments

(5 files, 1 obsolete file)

      No description provided.
Attached file testcase
Hi, contenteditable currently does not support spellcheck="false". See attached testcase.
Summary: contenteditable does not support spellcheck= → contenteditable does not support spellcheck=false
Arno, is this something you're willing to work on?  :-)
I don't have time for that at the moment.
Aryeh, can you please take a look at this?
I have a patch for this, and it works -- I just have to fix an unexpected failure in editor/libeditor/html/tests/test_bug611182.html.  It's taking screenshots and they're unexpectedly not matching, probably because we now get some squiggly lines that we didn't before.  I should have that fixed by early Sunday, if not today.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
So it looks like the code here was written for text inputs and designMode, and was just never designed to work with contenteditable.  Three changes were needed:

1) Make nsEditor::GetDesiredSpellCheckState call GetEditorRoot() instead of GetRoot().  Previously it would only check the body for spellcheck="", which is obviously wrong for contenteditable.

2) Change nsGenericHTMLElement::GetSpellcheck to make all editable nodes spellchecked by default, not just the body element of an editable document.

3) Change nsEditor::OnFocus to call SyncRealTimeSpell(), so if the focus changes we re-evaluate spellcheckability.  This is needed because now GetDesiredSpellCheckState() calls GetEditorRoot(), which calls GetActiveEditingHost(), which depends on what's focused.  Previously it just checked the root element, so it wasn't necessary to reevaluate when focus changed.

I had to change test_bug611182.html to set spellcheck false on the root element, not the body -- otherwise tests would fail if they put a text node directly in the body, because the squiggly line would appear and mess up the snapshot.  (While I was there I added an extra is() so that snapshot mismatches would log the respective snapshots.)

For testing this actual bug, I more or less just copied the spellcheck-textarea tests, with minor tweaks.  Usual issue with Splinter not recognizing hg cp applies.

Try: https://tbpl.mozilla.org/?tree=Try&rev=554fd23c8d6d
Attachment #644738 - Flags: review?(ehsan)
Comment on attachment 644738 [details] [diff] [review]
Patch

Oops, I submitted the wrong patch!  Anyway, canceling review request for now -- there are a bunch of try failures in browser/base/content/test/test_contextmenu.html.  Will investigate and resubmit.
Attachment #644738 - Attachment is obsolete: true
Attachment #644738 - Flags: review?(ehsan)
So this turns out to be a lot more annoying than I thought.  Basically, the spellchecker was not written to handle contenteditable.  The existing code doesn't easily handle the case where some elements in the page are spellcheckable and some are not.  All that logic needs to be taken out of nsEditor and moved into the spellchecker, so it can look on a per-text-node basis.  That would also eliminate the need for calls to SyncRealTimeSpell() all over the place, so it might well be substantially simpler.  Also, that will automatically support things like

  <div contenteditable>
    Spellcheck me
    <span spellcheck=false>but don't spellchekc me</span>
  </div>

which should work per spec, and works in IE and Opera (although not WebKit AFAICT).
(In reply to comment #8)
> So this turns out to be a lot more annoying than I thought.  Basically, the
> spellchecker was not written to handle contenteditable.  The existing code
> doesn't easily handle the case where some elements in the page are
> spellcheckable and some are not.  All that logic needs to be taken out of
> nsEditor and moved into the spellchecker, so it can look on a per-text-node
> basis.  That would also eliminate the need for calls to SyncRealTimeSpell() all
> over the place, so it might well be substantially simpler.  Also, that will
> automatically support things like
> 
>   <div contenteditable>
>     Spellcheck me
>     <span spellcheck=false>but don't spellchekc me</span>
>   </div>
> 
> which should work per spec, and works in IE and Opera (although not WebKit
> AFAICT).

This plan makes sense to me.
Anyone want to guess what this outputs in current Gecko?

data:text/html,<!DOCTYPE html>
<body><div contenteditable></div>
<script>
document.body.textContent =
document.body.spellcheck + " " +
document.body.firstChild.spellcheck
</script>

Yes, you're completely correct: "true false".  Nothing is more logical: .spellcheck is false for actual contenteditable elements by default, but true for <body> if it contains some editable descendant.

Having figured that out, I think I actually get what craziness Gecko does now, so I should be able to fix it!
I split these into their own patch for no particularly compelling reason.
Attachment #645233 - Flags: review?(ehsan)
So this does a few things:

1) Check the implementation of nsIDOMHTMLElement::GetSpellcheck to be sane.  It now treats editable elements (designMode/contenteditable) as true-by-default, and the body isn't special.

2) Change nsEditor::GetDesiredSpellCheckState to roughly restore the previous behavior.  It used to call GetSpellcheck on the body element, which worked well enough because of the weird way GetSpellcheck worked -- it indirectly called IsEditingOn on the current doc.  Now I made it just call IsEditingOn directly.  This means that in some cases it will return true when it didn't before, e.g., if you have <html spellcheck=false>.  This is deliberate, because you might have a descendant that resets spellcheck=true.

3) I update mozInlineSpellChecker::SkipSpellCheckForNode to check GetSpellcheck (for non-mail contexts) in addition to IsEditable.

This seems to have the desired effect; however, test_bug611182.html is failing locally for reasons I don't understand.  I'll see if it fails on try too: <https://tbpl.mozilla.org/?tree=Try&rev=ddeecf8aff59>.  The snapshot it takes of the window has a squiggly underline for some reason, but if I try to actually look at the document myself (whether by constructing a separate test-case or inserting a call to nonexistent() to stop the test itself at the right place) I don't see it.  Setting spellcheck to false seems to immediately suppress the squiggly underline when I look myself, but not in the snapshot.  Maybe this is an anomaly and will disappear on try, in which case I can push as-is if necessary.
Attachment #645237 - Flags: review?(ehsan)
(In reply to :Aryeh Gregor from comment #12)
> This seems to have the desired effect; however, test_bug611182.html is
> failing locally for reasons I don't understand.  I'll see if it fails on try
> too: <https://tbpl.mozilla.org/?tree=Try&rev=ddeecf8aff59>.  The snapshot it
> takes of the window has a squiggly underline for some reason, but if I try
> to actually look at the document myself (whether by constructing a separate
> test-case or inserting a call to nonexistent() to stop the test itself at
> the right place) I don't see it.  Setting spellcheck to false seems to
> immediately suppress the squiggly underline when I look myself, but not in
> the snapshot.

The same failures are showing on try.  Do you have any suggestions on how to debug or work around them?  I could try updating all the data: URLs in the test with spellcheck=false instead of adding it dynamically after the iframe loads.  That would probably do it.
There's another failure too -- test_textattrchange.html consistently times out on non-Mac platforms.  I'm leaving the r? in case you can spot anything wrong that might lead to these test failures.
Status: ASSIGNED → NEW
Comment on attachment 645233 [details] [diff] [review]
Patch part 1 -- Add reftests for spellcheck in contenteditable

Review of attachment 645233 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!
Attachment #645233 - Flags: review?(ehsan) → review+
Comment on attachment 645237 [details] [diff] [review]
Patch part 2 -- Make the spellcheck attribute work correctly for contenteditable

Review of attachment 645237 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/base/nsEditor.cpp
@@ +414,5 @@
> +    // is explicitly set anywhere, so if there's anything editable on the page,
> +    // return true and let the spellchecker figure it out.
> +    nsCOMPtr<nsIHTMLDocument> doc = do_QueryInterface(content->GetCurrentDoc());
> +    return doc && doc->IsEditingOn();
> +  }

I think you should do this after the |if (!element)| check below.

::: editor/libeditor/html/tests/test_bug611182.html
@@ +60,5 @@
>        iframe.removeEventListener("load", arguments.callee, false);
>  
>        var doc = iframe.contentDocument;
>        var win = iframe.contentWindow;
> +      doc.documentElement.spellcheck = false;

Why is this needed?  Setting the spellcheck attribute on the body should still work, right?
Attachment #645237 - Flags: review?(ehsan) → review-
(In reply to :Aryeh Gregor from comment #13)
> (In reply to :Aryeh Gregor from comment #12)
> > This seems to have the desired effect; however, test_bug611182.html is
> > failing locally for reasons I don't understand.  I'll see if it fails on try
> > too: <https://tbpl.mozilla.org/?tree=Try&rev=ddeecf8aff59>.  The snapshot it
> > takes of the window has a squiggly underline for some reason, but if I try
> > to actually look at the document myself (whether by constructing a separate
> > test-case or inserting a call to nonexistent() to stop the test itself at
> > the right place) I don't see it.  Setting spellcheck to false seems to
> > immediately suppress the squiggly underline when I look myself, but not in
> > the snapshot.
> 
> The same failures are showing on try.  Do you have any suggestions on how to
> debug or work around them?  I could try updating all the data: URLs in the
> test with spellcheck=false instead of adding it dynamically after the iframe
> loads.  That would probably do it.

Hmm try modifying the test to only check one document and then set a break point in DoSpellCheck and see if the new skip logic works correctly after you press backspace?  FWIW looking at the failure log I think this might be a real bug in your patch.
(FWIW I doubt that the test failures are caused by what I commented on above.)
(In reply to Ehsan Akhgari [:ehsan] from comment #16)
> Comment on attachment 645237 [details] [diff] [review]
> Patch part 2 -- Make the spellcheck attribute work correctly for
> contenteditable
> 
> Review of attachment 645237 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: editor/libeditor/base/nsEditor.cpp
> @@ +414,5 @@
> > +    // is explicitly set anywhere, so if there's anything editable on the page,
> > +    // return true and let the spellchecker figure it out.
> > +    nsCOMPtr<nsIHTMLDocument> doc = do_QueryInterface(content->GetCurrentDoc());
> > +    return doc && doc->IsEditingOn();
> > +  }
> 
> I think you should do this after the |if (!element)| check below.

Okay -- although I hope that doesn't change the results in any sane case!

> ::: editor/libeditor/html/tests/test_bug611182.html
> @@ +60,5 @@
> >        iframe.removeEventListener("load", arguments.callee, false);
> >  
> >        var doc = iframe.contentDocument;
> >        var win = iframe.contentWindow;
> > +      doc.documentElement.spellcheck = false;
> 
> Why is this needed?  Setting the spellcheck attribute on the body should
> still work, right?

Good question . . . I did it initially because I thought text was being inserted directly into the root element, but of course that's not actually true.  Maybe it's not actually needed with the current patch version.

(In reply to Ehsan Akhgari [:ehsan] from comment #17)
> Hmm try modifying the test to only check one document and then set a break
> point in DoSpellCheck and see if the new skip logic works correctly after
> you press backspace?  FWIW looking at the failure log I think this might be
> a real bug in your patch.

Yeah, I suspect there's a real bug, but I can't figure out what.  I'll try debugging as you suggest.
(In reply to comment #19)
> (In reply to Ehsan Akhgari [:ehsan] from comment #16)
> > Comment on attachment 645237 [details] [diff] [review]
> > Patch part 2 -- Make the spellcheck attribute work correctly for
> > contenteditable
> > 
> > Review of attachment 645237 [details] [diff] [review]:
>  --> (https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=674927&attachment=645237)
> > -----------------------------------------------------------------
> > 
> > ::: editor/libeditor/base/nsEditor.cpp
> > @@ +414,5 @@
> > > +    // is explicitly set anywhere, so if there's anything editable on the page,
> > > +    // return true and let the spellchecker figure it out.
> > > +    nsCOMPtr<nsIHTMLDocument> doc = do_QueryInterface(content->GetCurrentDoc());
> > > +    return doc && doc->IsEditingOn();
> > > +  }
> > 
> > I think you should do this after the |if (!element)| check below.
> 
> Okay -- although I hope that doesn't change the results in any sane case!

Me too!

> > ::: editor/libeditor/html/tests/test_bug611182.html
> > @@ +60,5 @@
> > >        iframe.removeEventListener("load", arguments.callee, false);
> > >  
> > >        var doc = iframe.contentDocument;
> > >        var win = iframe.contentWindow;
> > > +      doc.documentElement.spellcheck = false;
> > 
> > Why is this needed?  Setting the spellcheck attribute on the body should
> > still work, right?
> 
> Good question . . . I did it initially because I thought text was being
> inserted directly into the root element, but of course that's not actually
> true.  Maybe it's not actually needed with the current patch version.

OK, then I think it's best if you revert this change.

> (In reply to Ehsan Akhgari [:ehsan] from comment #17)
> > Hmm try modifying the test to only check one document and then set a break
> > point in DoSpellCheck and see if the new skip logic works correctly after
> > you press backspace?  FWIW looking at the failure log I think this might be
> > a real bug in your patch.
> 
> Yeah, I suspect there's a real bug, but I can't figure out what.  I'll try
> debugging as you suggest.

Cool!
It's great to see the progress here!

Bug 779092 and bug 494351 are possibly duplicates
Blocks: 779092
Unfortunately, I'm unlikely to be able to finish this.
Assignee: ayg → nobody
Any chance this will be picked back up in the foreseeable future?
(In reply to comment #24)
> Any chance this will be picked back up in the foreseeable future?

We don't really have anyone who can work on this.  :(
Facebook's new Graph Search uses a contentEditable field as the the primary input. Searches often include names which unfortunately have a tendency to trigger the spell checker. See example: http://imgur.com/ZaVLPx1
Jet, do you know if there's anyone who can finish up the patches here?  It would be great if we can get this fixed -- I don't expect there being too much work left here.
Depends on: 882879
Comment on attachment 645233 [details] [diff] [review]
Patch part 1 -- Add reftests for spellcheck in contenteditable

https://hg.mozilla.org/integration/mozilla-inbound/rev/67b02bc5a2a1
Attachment #645233 - Flags: checkin+
Whiteboard: [leave open]
Comment on attachment 645233 [details] [diff] [review]
Patch part 1 -- Add reftests for spellcheck in contenteditable

Follow-up: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e674c399e3e
Attached patch Part 2Splinter Review
Drew, please see this try run:

https://tbpl.mozilla.org/?tree=Try&rev=913357d16439

I think that the crashtest timeout is caused by the spell checking query selector in reftest-content.js, but I'm not 100% sure how to fix it.  Drew, would you mind taking a look?  With this patch applied, when loading that test case, the only node under a contenteditable element is the "Five" textnode, which is a descendant of a <span contenteditable="false"> so it should not be spell checked.  But I think that makes the reftest framework wait indefinitely for a spellcheck to happen...
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #764182 - Flags: feedback?(adw)
A timeout is bad since it almost certainly means that AsyncSpellCheckTestHelper received a "spell check started" notification for an element's editor and has hung waiting for an "ended" notification for the same editor.  I'd guess that either the ended notification never fired, or it fired but with a different nsEditor from the one that was passed to the started notification.  I'll take a closer look.
What's happening in AsyncSpellCheckTestHelper is:

1. onSpellCheck(<body>).  At this time, waitingForEnded = true because the
   nsEditor's mozInlineSpellChecker's spellCheckPending = true.
2. onSpellCheck(<div>).  Again, waitingForEnded = true.
3. "started" notification broadcasted for the div.  This shouldn't happen since
   according to the mozInlineSpellChecker, spellCheckPending already = true.
   waitingForEnded is flipped to false.
4. "started" notification broadcasted for the body.  Same problem.
   waitingForEnded is flipped to false.
5. "ended" broadcasted for the div.  waitingForEnded flipped back to true.
5. "ended" broadcasted for the body.  Same problem.

Further debugging shows that this patch causes the two mozInlineSpellCheckers created during the test to both have mNumPendingSpellChecks > 0 at a certain point, but there should be at most one with mNumPendingSpellChecks > 0 at any point.  The started notifications in steps 3 and 4 are from the second mozInlineSpellChecker, but the first's mNumPendingSpellChecks hasn't reached zero yet.  I don't understand yet why the patch makes that happen.
Do you mean that mNumPendingSpellChecks should never be incremented, or that something like <http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#806> is never executed?  Could this be by returning false from mozInlineSpellChecker::SkipSpellCheckForNode (since that is the real change in this patch)?  That could change the control flow here <http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1527> but I can't see why this should affect mNumPendingSpellChecks.

Also, what code ensures that the second started notification should not fire before the first mNumPendingSpellChecks reaches 0?
Whiteboard: [leave open]
I think what's happening here is that when the first editor's PreDestroy() method is called, and we call mozInlineSpellChecker::Cleanup(), we destroy it without cleaning up our async stuff.  Basically, I think the code in mozInlineSpellChecker::SetEnableRealTimeSpell should be moved to inside Cleanup.  I'll give that a shot...
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #39)
> it without cleaning up our async stuff.  Basically, I think the code in
> mozInlineSpellChecker::SetEnableRealTimeSpell should be moved to inside
> Cleanup.  I'll give that a shot...

That sounds right I think.  Since mozInlineSpellChecker::SetEnableRealTimeSpell is not the only Cleanup caller.  Just please make sure to pass mEditor to ChangeNumPendingSpellChecks even though Cleanup nulls it out, same as the code currently does. :-)
Attachment #764182 - Flags: feedback?(adw)
Comment on attachment 764910 [details] [diff] [review]
Cleanup the async spellcheck stuff when the editor cleans up mozInlineSpellChecker

Review of attachment 764910 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this!  Sorry it got in your way.
Attachment #764910 - Flags: review?(adw) → review+
Add back a reftest which I removed by mistake: https://hg.mozilla.org/integration/mozilla-inbound/rev/8f1f12f449dc
Depends on: 904553
Depends on: 905174
Depends on: 906979
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: