Modifying text of a contenteditable DOM Node removes spellcheck underlinings.

RESOLVED FIXED in mozilla9

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: kodierer, Assigned: kaze)

Tracking

(Blocks: 1 bug)

5 Branch
mozilla9
All
Other
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
Created attachment 548505 [details]
Example file demonstrating contenteditable / spellcheck + DOM modification behaviour
(Reporter)

Comment 2

6 years ago
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
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Blocks: 679011
(Assignee)

Comment 4

6 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

6 years ago
Created attachment 553583 [details] [diff] [review]
patch proposal
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+
(Assignee)

Comment 7

6 years ago
TryServer looks OK:
http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=74400879090e
(Assignee)

Comment 8

6 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

6 years ago
Created attachment 553790 [details] [diff] [review]
patch proposal

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]
>   --> 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

6 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
OK, please re-request review once you get results on this?
(Assignee)

Comment 13

6 years ago
TryServer is OK now:
http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=03fb6fd84fd9
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:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5c8e4c01f13
http://hg.mozilla.org/mozilla-central/rev/800f7541fb20
http://hg.mozilla.org/mozilla-central/rev/d5c8e4c01f13
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9

Updated

6 years ago
Depends on: 684638

Updated

6 years ago
Depends on: 696618

Updated

5 years ago
Depends on: 743819
You need to log in before you can comment on or make changes to this bug.