Last Comment Bug 674212 - Modifying text of a contenteditable DOM Node removes spellcheck underlinings.
: Modifying text of a contenteditable DOM Node removes spellcheck underlinings.
Product: Core
Classification: Components
Component: Editor (show other bugs)
: 5 Branch
: All Other
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Fabien Cazenave [:kaze]
: Makoto Kato [:m_kato]
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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 | Splinter Review
patch proposal (3.13 KB, patch)
2011-08-17 08:56 PDT, Fabien Cazenave [:kaze]
ehsan: review+
Details | Diff | Splinter 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>
		<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.
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 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) {
     mBogusNode = nsnull;
   // Try to recreate the bogus node if needed.
+  // 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 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:
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 2011-08-17 10:04:34 PDT
(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?
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):
Comment 12 :Ehsan Akhgari 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:
Comment 14 :Ehsan Akhgari 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:

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