Last Comment Bug 674212 - Modifying text of a contenteditable DOM Node removes spellcheck underlinings.
: Modifying text of a contenteditable DOM Node removes spellcheck underlinings.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: 5 Branch
: All Other
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Fabien Cazenave [:kaze]
:
Mentors:
Depends on: 684638 696618 743819
Blocks: 679011
  Show dependency treegraph
 
Reported: 2011-07-26 06:19 PDT by kodierer
Modified: 2012-04-10 20:28 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Example file demonstrating contenteditable / spellcheck + DOM modification behaviour (472 bytes, text/html)
2011-07-26 09:51 PDT, kodierer
no flags Details
patch proposal (3.03 KB, patch)
2011-08-16 14:08 PDT, Fabien Cazenave [:kaze]
ehsan: review+
Details | Diff | Review
patch proposal (3.13 KB, patch)
2011-08-17 08:56 PDT, Fabien Cazenave [:kaze]
ehsan: review+
Details | Diff | Review

Description kodierer 2011-07-26 06:19:08 PDT
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.
Comment 1 kodierer 2011-07-26 09:51:55 PDT
Created attachment 548505 [details]
Example file demonstrating contenteditable / spellcheck + DOM modification behaviour
Comment 2 kodierer 2011-07-26 10:24:37 PDT
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.
Comment 3 :Ehsan Akhgari (out sick) 2011-07-26 14:44:12 PDT
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).
Comment 4 Fabien Cazenave [:kaze] 2011-08-16 13:02:24 PDT
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();
 }
Comment 5 Fabien Cazenave [:kaze] 2011-08-16 14:08:44 PDT
Created attachment 553583 [details] [diff] [review]
patch proposal
Comment 6 :Ehsan Akhgari (out sick) 2011-08-17 04:58:23 PDT
Comment on attachment 553583 [details] [diff] [review]
patch proposal

If this has passed try, we can land it.
Comment 7 Fabien Cazenave [:kaze] 2011-08-17 07:03:00 PDT
TryServer looks OK:
http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=74400879090e
Comment 8 Fabien Cazenave [:kaze] 2011-08-17 08:14:58 PDT
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()?
Comment 9 Fabien Cazenave [:kaze] 2011-08-17 08:56:19 PDT
Created attachment 553790 [details] [diff] [review]
patch proposal

adding a setTimeout([…], 0) to see if it helps. :-/
Comment 10 :Ehsan Akhgari (out sick) 2011-08-17 10:04:34 PDT
(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?
Comment 11 Fabien Cazenave [:kaze] 2011-08-17 10:19:37 PDT
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 :Ehsan Akhgari (out sick) 2011-08-17 10:24:04 PDT
OK, please re-request review once you get results on this?
Comment 13 Fabien Cazenave [:kaze] 2011-08-17 13:36:10 PDT
TryServer is OK now:
http://tbpl.allizom.org/?usebuildbot=1&tree=Try&rev=03fb6fd84fd9
Comment 14 :Ehsan Akhgari (out sick) 2011-08-17 14:13:09 PDT
Comment on attachment 553790 [details] [diff] [review]
patch proposal

Pushed to inbound.
Comment 15 Matt Brubeck (:mbrubeck) 2011-08-17 16:37:45 PDT
The reftest was failing on Android.  Pushed a followup to disable it there:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5c8e4c01f13

Note You need to log in before you can comment on or make changes to this bug.