Last Comment Bug 672709 - Adding elements to contenteditable DOM in designMode will cause the -moz-read-write state not to update properly
: Adding elements to contenteditable DOM in designMode will cause the -moz-read...
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla8
Assigned To: :Ehsan Akhgari
: Andrew Overholt [:overholt]
Depends on:
Blocks: 449243 598833
  Show dependency treegraph
Reported: 2011-07-19 18:29 PDT by :Ehsan Akhgari
Modified: 2011-08-22 00:55 PDT (History)
10 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Testcase (389 bytes, text/html)
2011-07-19 18:29 PDT, :Ehsan Akhgari
no flags Details
Testcase (428 bytes, text/html)
2011-07-20 09:12 PDT, :Ehsan Akhgari
no flags Details
Patch (v1) (5.07 KB, patch)
2011-07-20 11:25 PDT, :Ehsan Akhgari
bzbarsky: review+
bugzilla: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image :Ehsan Akhgari 2011-07-19 18:29:23 PDT
Created attachment 546961 [details]

See the attached testcase.

This is a regression from I believe.  This is the code responsible:

Boris, what was the reason for this optimization?  This has the potential of breaking editor operations _very_ badly (such as pressing backspace at the end of a paragraph deleting the whole paragraph because it's treated as a contenteditable=false section.)
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2011-07-19 19:45:30 PDT
The reason for the optimization is that calling UpdateState on a link is astronomically expensive.   I have a proposal to maybe fix that at but it hasn't happened yet.

That said, what's breaking in this case?  Let me take a look.
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2011-07-19 19:52:51 PDT
In particular, I see us enter this function with aNotify == false, oldEditable != newEditable, and newEditable == true, so we remove the READONLY state and add the READWRITE state.

If I remove the "skip calling UpdateState if !aNotify" optimization, the bug remains.

If I remove the "skip messing with state if oldEditable != newEditable" optimization then the bug remains.....
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2011-07-19 20:16:35 PDT
And in fact, what's happening in that testcase is that the <body> is readonly, so gets styled with a red color and there are no other color styles around, so the red inherits into everything...
Comment 4 User image :Ehsan Akhgari 2011-07-20 09:12:17 PDT
Created attachment 547115 [details]

Sorry, I uploaded the wrong version of the testcase (setting designMode to "true" doesn't work).

The bug happens because of the comparison of oldEditable and newEditable.  In this scenario, oldEditable is true because the document has the NODE_IS_EDITABLE flag, and newEditable is true because the node has the NODE_IS_EDITABLE flag.  But we think that we can skip updating the readonly/readwrite intrinsic states, so we end up with the p being readonly, which causes the editor to treat it as a contenteditable=false block, which leads to very bad results.
Comment 5 User image :Ehsan Akhgari 2011-07-20 10:32:13 PDT
We need to fix this on Aurora too.
Comment 6 User image :Ehsan Akhgari 2011-07-20 11:25:20 PDT
Created attachment 547162 [details] [diff] [review]
Patch (v1)
Comment 7 User image Boris Zbarsky [:bz] (still a bit busy) 2011-07-20 11:43:31 PDT
Comment on attachment 547162 [details] [diff] [review]
Patch (v1)

Comment 8 User image :Ehsan Akhgari 2011-07-20 16:23:23 PDT
Comment on attachment 547162 [details] [diff] [review]
Patch (v1)

This patch is rather low-risk here.  We definitely want this on Aurora, otherwise the HTML editor might be broken in unexpected and major ways.
Comment 9 User image Marco Bonardo [::mak] 2011-07-21 06:20:12 PDT
Comment 10 User image Johnathan Nightingale [:johnath] 2011-07-21 14:53:01 PDT
Comment on attachment 547162 [details] [diff] [review]
Patch (v1)

a=me for aurora, but this is a regression from what looks like a pretty large landing - if we see more regressions coming from that, we're going to want to look at backing out the original offender on aurora instead of chasing clean up fixes.
Comment 12 User image Trif Andrei-Alin[:AlinT] 2011-08-19 04:35:26 PDT
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110818 Firefox/9.0a1

I cannot reproduce the issue.
Was it Fixed?
Comment 13 User image :Ehsan Akhgari 2011-08-19 11:55:49 PDT
Yes, this has been fixed in Firefox 7 and above.

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