Closed Bug 923376 Opened 11 years ago Closed 10 years ago

Spellchecker does not work in contentEditable after modifying its content from JS.

Categories

(Core :: Spelling checker, defect)

15 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: cristi_talau, Assigned: ayg)

References

Details

(Keywords: regression, testcase, Whiteboard: [bugday-20131007])

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/29.0.1547.76 Safari/537.36

Steps to reproduce:

1. Load the attached HTML file.
2. Place the caret inside the contentEditable div (for example between 'something' and 'elsed').
3. Press the update button.



Actual results:

The word 'elsed' is not marked as misspelled.


Expected results:

The word 'elsed' should be marked as misspelled.
Confirmed with 2013-10-07-03-02-03-mozilla-central-firefox-27.0a1.en-US.linux-x86_64

wfm: 2012-04-25-03-06-47-mozilla-central-firefox-15.0a1.en-US.linux-x86_64
bug: 2012-04-26-03-05-04-mozilla-central-firefox-15.0a1.en-US.linux-x86_64

https://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2012-04-25&enddate=2012-04-26+03%3A05
Blocks: 743819
Component: Untriaged → Spelling checker
OS: Windows 7 → All
Product: Firefox → Core
Whiteboard: [bugday-20131007]
Version: 24 Branch → 15 Branch
Aryeh, can you take a look at this, please? Thanks!
Flags: needinfo?(ayg)
I can't make any guarantees, but I'll put it on my list to look at when I next have time.  It looks like it will be simple, just needs stepping through with a debugger.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(ayg)
Attached patch patchSplinter Review
Simple fix.  The only problem is, the test seems not to work.  When I view the test and ref files manually, it works as expected: the test file has "missspelled" underlined both pre- and post-patch, "elsed" is underlined only post-patch.  But when the reftest runner runs the tests, it sees neither word as underlined in the test file, although both are underlined in the ref file.  Do you know what might be going wrong?  I fidgeted a little and didn't find any way to get it to work.

The use of setTimeout in the test file doesn't seem ideal to me, but onload didn't trigger the bug.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #8401338 - Flags: review?(ehsan)
Comment on attachment 8401338 [details] [diff] [review]
patch

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

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +3264,5 @@
>      mRules->DocumentModified();
>  
>      // Update spellcheck for only the newly-inserted node (bug 743819)
>      if (mInlineSpellChecker) {
> +      nsRefPtr<nsRange> range = new nsRange(aContainer);

Hmm, the argument to nsRange::nsRange is only used to determine the owner doc...  I don't think you need to change this argument here.

::: editor/libeditor/html/nsHTMLEditor.h
@@ +951,5 @@
>                                         const nsAString* aAttribute,
>                                         const nsAString* aValue);
> +  void DoContentInserted(nsIDocument* aDocument, nsIContent* aContainer,
> +                         nsIContent* aChild, int32_t aIndexInContainer,
> +                         bool aAppend);

Nit: please add a new enum called InsertedOrAppended or something and use that instead of the bool argument.

::: editor/reftests/923376-ref.html
@@ +1,2 @@
> +<!doctype html>
> +<div contenteditable="true">something missspelled<br>something elsed#</div>

Here you should be able to just use class="spell-checked" I think.

::: editor/reftests/923376.html
@@ +5,5 @@
> +setTimeout(function() {
> +  document.body.firstChild.innerHTML =
> +    'something missspelled<br>something elsed#';
> +  document.documentElement.removeAttribute("class");
> +}, 100);

To fix this test, you probably need to use onSpellCheck, similar to the end of attachment 756918 [details] [diff] [review] (see bug 856270 for the changes that we made to this stuff.)
Attachment #8401338 - Flags: review?(ehsan) → review+
Try for new patch version: https://tbpl.mozilla.org/?tree=Try&rev=202cf484ac3a
Hardware: x86_64 → All
Thanks for the backout.  It looks like the Windows 7/8 opt builds are ignoring one of the blur()s and leaving the focus box thing, so they don't match because one iframe has the editable area focused and one doesn't.  I ran into that problem while testing locally, but fixed it by adding div.blur() to both so neither would be focused.  Seems Windows keeps the focus ring even when it's no longer focused -- maybe that's a separate bug.  (But why only opt?)

I triggered a couple of reruns to make sure that the failure is consistent for Windows 7/8 opt.  I guess I'll try some other workaround, like .focus()ing something in the parent frame.
Hmm, so I don't know how to get rid of the focus ring in the second iframe.  .blur() doesn't work, and .focus()ing the documentElement of the parent window also doesn't work.  This seems like a bug in Windows and/or Firefox on Windows somehow, although in a very simple testcase I can't reproduce on stable (23.0.1).  Maybe I should try on a nightly.
You could still have these as regular reftests, not sure why you converted them to be a mochitest.  We've had a ton of problems with test_reftest_with_caret.html failing intermittently on Windows, so it would be ideal to not reinvent that wheel here.

Can't you just use the class="spell-checked" annotation on the div?  If not, you can just use onSpellCheck in the reftest directly, like <https://hg.mozilla.org/mozilla-central/rev/3f4ad11d56da#l10.25>.
I was copying <https://hg.mozilla.org/mozilla-central/rev/3f4ad11d56da#l10.25>, per comment #5.  AFAICT that's a mochitest, not a reftest.  It's a support file loaded by test_reftests_with_caret.html in that directory.  The things I saw lying around on Google indicated that reftests can't use stuff like SimpleTest, which matched my memory, so I didn't try.  I didn't use class="spell-checked" because comment #5 said I probably needed to use onSpellCheck.

I'll take another look at this, though.
(In reply to comment #12)
> I was copying <https://hg.mozilla.org/mozilla-central/rev/3f4ad11d56da#l10.25>,
> per comment #5.  AFAICT that's a mochitest, not a reftest.  It's a support file
> loaded by test_reftests_with_caret.html in that directory.  The things I saw
> lying around on Google indicated that reftests can't use stuff like SimpleTest,
> which matched my memory, so I didn't try.  I didn't use class="spell-checked"
> because comment #5 said I probably needed to use onSpellCheck.
> 
> I'll take another look at this, though.

Ah, you're right, my bad.  I managed to confuse myself here.  I would still try out class="spell-checked" though, it may just work.

Unfortunately I have no idea why you're hitting this Windows problem...
It looks like I need onSpellCheck for the test file, because I need to run code after the first spellcheck runs, so IIUC, class="spell-checked" isn't good enough.  How would you suggest trying to use it?  I need to wait for the initial spellcheck to finish, then set the innerHTML, then wait for the new spellcheck to finish before taking the snapshot.

Perhaps class="spell-checked" together with a timer would work, if the harness will wait for the spellcheck to finish *after* .finish() is called.  But a timer is obviously not a great way to do this.

(class="spell-checked" looks like it should be fine for the ref file.)
Flags: needinfo?(ehsan)
Looking at the reftest harness code more closely, it looks like we just take the snapshot when onSpellCheck is called for those elements, so I guess you can't really hook into that without horrible hacks...  I guess I wasn't paying too much attention to the fact you need to wait for the spell check event twice in the test file.

Thinking of other workarounds, perhaps you can eliminate the focus ring by setting outline:none or some such on the div (note that I'm just guessing, please test that)

Also, another suggestion is to add your test to <http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/test_reftests_with_caret.html?force=1>.  That miniharness takes care of things such as focusing the iframes, waitForFocusing etc, none of which is done in your miniharness.  If anything, doing that might avoid a bunch of intermittent test failures in the future.
Flags: needinfo?(ehsan)
(In reply to comment #16)
> Using test_reftests_with_caret.html:
> https://tbpl.mozilla.org/?tree=Try&rev=441fa0e60aba

This fails on b2g, which is expected since spell checking is disabled there.  You can skip the test if the layout.spellcheckDefault pref is set to false, which will take of that.
Finally, a passing try push, after some more test revisions to fix intermittent orange on Windows:

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/b755e9ea6293
https://hg.mozilla.org/mozilla-central/rev/b755e9ea6293
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.