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)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: cristi_talau, Assigned: ayg)
References
Details
(Keywords: regression, testcase, Whiteboard: [bugday-20131007])
Attachments
(2 files)
223 bytes,
text/html
|
Details | |
5.37 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•11 years ago
|
||
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
Assignee | ||
Comment 3•11 years ago
|
||
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)
Updated•10 years ago
|
Keywords: regression,
testcase
Assignee | ||
Comment 4•10 years ago
|
||
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.
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
Try for new patch version: https://tbpl.mozilla.org/?tree=Try&rev=202cf484ac3a
Hardware: x86_64 → All
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f9bd1c29a40
Flags: in-testsuite+
Comment 8•10 years ago
|
||
sorry had to backout this change, seems this caused https://tbpl.mozilla.org/php/getParsedLog.php?id=37368356&tree=Mozilla-Inbound
Assignee | ||
Comment 9•10 years ago
|
||
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.
Assignee | ||
Comment 10•10 years ago
|
||
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.
Comment 11•10 years ago
|
||
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>.
Assignee | ||
Comment 12•10 years ago
|
||
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.
Comment 13•10 years ago
|
||
(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...
Assignee | ||
Comment 14•10 years ago
|
||
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)
Comment 15•10 years ago
|
||
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)
Assignee | ||
Comment 16•10 years ago
|
||
Using test_reftests_with_caret.html: https://tbpl.mozilla.org/?tree=Try&rev=441fa0e60aba
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
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
Comment 19•10 years ago
|
||
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.
Description
•