Closed Bug 674212 Opened 11 years ago Closed 11 years ago

Modifying text of a contenteditable DOM Node removes spellcheck underlinings.


(Core :: DOM: Editor, defect)

5 Branch
Not set





(Reporter: kodierer, Assigned: kaze)


(Blocks 1 open bug)



(2 files, 1 obsolete file)

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>
		<div id="editable" contenteditable="true" spellcheck="true">
			This text is misspellored
		<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>

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.
Attachment #548505 - Attachment mime type: text/plain → text/html
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
Ever confirmed: true
Blocks: 679011
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) {
     mBogusNode = nsnull;
   // Try to recreate the bogus node if needed.
+  // Reset the spell checker
+  SyncRealTimeSpell();
Attached patch patch proposal (obsolete) — Splinter Review
Attachment #553583 - Flags: review?(ehsan)
Comment on attachment 553583 [details] [diff] [review]
patch proposal

If this has passed try, we can land it.
Attachment #553583 - Flags: review?(ehsan) → review+
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()?
Attached patch patch proposalSplinter Review
adding a setTimeout([…], 0) to see if it helps. :-/
Attachment #553583 - Attachment is obsolete: true
(In reply to comment #9)
> Created attachment 553790 [details] [diff] [review]
>   -->
> patch proposal
> adding a setTimeout([…], 0) to see if it helps. :-/

So, does this help?
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):
OK, please re-request review once you get results on this?
Comment on attachment 553790 [details] [diff] [review]
patch proposal

Pushed to inbound.
Attachment #553790 - Flags: review+
The reftest was failing on Android.  Pushed a followup to disable it there:
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Depends on: 684638
Depends on: 696618
Depends on: 743819
You need to log in before you can comment on or make changes to this bug.