Closed Bug 672709 Opened 13 years ago Closed 13 years ago

Adding elements to contenteditable DOM in designMode will cause the -moz-read-write state not to update properly

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla8
Tracking Flags:
Tracking Status
firefox7 + fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Testcase
428 bytes, text/html
Details
Patch (v1)
5.07 KB, patch
bzbarsky
: review+
johnath
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Attached file Testcase (obsolete) — 
See the attached testcase.

This is a regression from http://hg.mozilla.org/mozilla-central/rev/5ad2fcf8d9ed I believe.  This is the code responsible:  http://hg.mozilla.org/mozilla-central/annotate/aa2de73abc19/content/base/src/nsGenericElement.cpp#l1236

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.)
The reason for the optimization is that calling UpdateState on a link is astronomically expensive.   I have a proposal to maybe fix that at http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/eb0a9472df8c0593/30fd10ec83f48a2a but it hasn't happened yet.

That said, what's breaking in this case?  Let me take a look.
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.....
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...
OS: Mac OS X → All
Hardware: x86 → All
Attached file Testcase — 
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.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #546961 - Attachment is obsolete: true
We need to fix this on Aurora too.
tracking-firefox7: --- → ?
Attached patch Patch (v1) — — Splinter Review 

        Attachment #547162 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 547162 [details] [diff] [review]
Patch (v1)

r=me

        Attachment #547162 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
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.

        Attachment #547162 -
        Flags: approval-mozilla-aurora?

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/579944e7e8f7
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8

    
  

          status-firefox7:
          --- → affected

    
  
Flags: in-testsuite+

    
    

    
  
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.

        Attachment #547162 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
  

          tracking-firefox7:
          ? → +

    
    

    
  
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?
Thanks!

    
    

    
  
Yes, this has been fixed in Firefox 7 and above.
Status: RESOLVED → VERIFIED
Component: DOM → DOM: Core & HTML

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: