Closed
Bug 674212
Opened 13 years ago
Closed 13 years ago
Modifying text of a contenteditable DOM Node removes spellcheck underlinings.
Categories
(Core :: DOM: Editor, defect)
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: kodierer, Assigned: kaze)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
|
472 bytes,
text/html
|
Details | |
|
3.13 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; rv:5.0.1) Gecko/20100101 Firefox/5.0.1
Build ID: 20110707182747
Steps to reproduce:
- Set the contenteditable and the spellcheck attribute of a DOM Node to "true".
- Type some misspelled text inside the contenteditable element. Red underlinings should appear.
- Modify the text of the contentedtiable element by using innerHTML or textContent.
Example code:
<!doctype html>
<html>
<body>
<div id="editable" contenteditable="true" spellcheck="true">
This text is misspellored
</div>
<div onclick="document.getElementById('editable').textContent = 'This text is misspellored, too.'" style="cursor:pointer; margin-top: 10px">Click here to modify text</div>
<div onclick="document.body.innerHTML = document.body.innerHTML" style="cursor:pointer; margin-top: 10px">Click here to reset body.innerHTML</div>
</body>
</html>
Actual results:
The underlinings of the misspelled words disappear. If you start typing inside the contenteditable again, only words that are a few words before the caret, regain their underlinings. There is one case I figured out that brings spellchecking back: If the whole document body is set to contenteditable="true", you can set document.body.innerHTML = document.body.innerHTML, but Of course this messes up any javascript node references.
Expected results:
Spellchecking should be automatically be retriggered after modification of the nodes' text without any user interaction.
Correction for initial post regarding the workaround to retrigger spellchecking: the document body does not have to be contenteditable itself.
Nevertheless, this issue is very annoying as it renders spellchecking in a javascript application (using static dom node references) unusable if you don't want to put every editable content into its own iframe and use said workaround. Any javascript that's doing on-the-fly processing on the editable content (like word hyphenation or highlighting) cannot make use of noderefs at all.
|
||
Updated•13 years ago
|
Attachment #548505 -
Attachment mime type: text/plain → text/html
|
|
||
Comment 3•13 years ago
|
||
This can be fixed by initiating a spell check from nsHTMLEditRules::DocumentModifiedWorker. You may also need to override nsHTMLEditor::CharacterDataChanged (look at the rest of the nsIMutationObserver method implementations in nsHTMLEditor for a guide).
Assignee: nobody → kaze
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
|
Assignee | |
Comment 4•13 years ago
|
||
Would this + a reftest be an acceptable solution?
--- a/editor/libeditor/html/nsHTMLEditRules.cpp
+++ b/editor/libeditor/html/nsHTMLEditRules.cpp
@@ -9256,9 +9256,12 @@ nsHTMLEditRules::DocumentModifiedWorker(
// empty any more.
if (mBogusNode) {
mEditor->DeleteNode(mBogusNode);
mBogusNode = nsnull;
}
// Try to recreate the bogus node if needed.
CreateBogusNodeIfNeeded(selection);
+
+ // Reset the spell checker
+ SyncRealTimeSpell();
}
|
|
Assignee | |
Comment 5•13 years ago
|
||
Attachment #553583 -
Flags: review?(ehsan)
|
|
||
Comment 6•13 years ago
|
||
Comment on attachment 553583 [details] [diff] [review]
patch proposal
If this has passed try, we can land it.
Attachment #553583 -
Flags: review?(ehsan) → review+
|
|
Assignee | |
Comment 7•13 years ago
|
||
TryServer looks OK:
http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=74400879090e
|
|
Assignee | |
Comment 8•13 years ago
|
||
hmmm, the reftest I’ve added has failed on two platforms:
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/reftest/tests/layout/reftests/editor/674212-spellcheck.html | image comparison (==)
Maybe I should add use a setTimeout()?
|
|
Assignee | |
Comment 9•13 years ago
|
||
adding a setTimeout([…], 0) to see if it helps. :-/
Attachment #553583 -
Attachment is obsolete: true
|
|
||
Comment 10•13 years ago
|
||
(In reply to comment #9)
> Created attachment 553790 [details] [diff] [review]
> --> https://bugzilla.mozilla.org/attachment.cgi?id=553790&action=edit
> patch proposal
>
> adding a setTimeout([…], 0) to see if it helps. :-/
So, does this help?
|
|
Assignee | |
Comment 11•13 years ago
|
||
Hard to tell. On my laptop it worked without any bogus setTimeout, but when running on Tinderbox I got those reftest failures.
Here’s the latest push (only 1/13 platform tested atm):
http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=03fb6fd84fd9
|
|
||
Comment 12•13 years ago
|
||
OK, please re-request review once you get results on this?
|
|
Assignee | |
Comment 13•13 years ago
|
||
TryServer is OK now:
http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=03fb6fd84fd9
|
|
||
Comment 14•13 years ago
|
||
Comment on attachment 553790 [details] [diff] [review]
patch proposal
Pushed to inbound.
Attachment #553790 -
Flags: review+
|
||
Comment 15•13 years ago
|
||
The reftest was failing on Android. Pushed a followup to disable it there:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5c8e4c01f13
|
||
Comment 16•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/800f7541fb20
http://hg.mozilla.org/mozilla-central/rev/d5c8e4c01f13
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in
before you can comment on or make changes to this bug.
Description
•