Closed Bug 611182 Opened 14 years ago Closed 14 years ago

Backspace key does not work without typing something first

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b8
Tracking Flags:
Tracking Status
blocking2.0 --- beta8+

People

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

References

(Depends on 1 open bug,
URL
)

Details

Attachments

(7 files, 4 obsolete files)

Part 3: Handle dynamic changes to the editable documents and create/remove the bogus node if needed
13.21 KB, patch
bzbarsky
: review-
Details | Diff | Splinter Review
Part 4: Add some more tests
7.43 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Part 5: Adjust the assertion annotation for an assertion which now gets triggered more because we're doing things correctly
936 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Part 6: Actually allow the bogus node to be deleted from the document, and check if the parent is modifiable when inserting an element
2.08 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Part 1: test cases
6.06 KB, patch
Details | Diff | Splinter Review
Part 7: Don't notify the edit rules object for document modifications outside of the editable area
1.38 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Part 2: Correct the definition of EndOfDocument for the HTML editor in order to fix some tests which have been relying on the bogus node existing in non-empty documents
4.91 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
STR:

1. Load the test case in the URL.
2. Click somewhere on "foo".
3. Press backspace.

roc: this definitely needs to block.  So much for my empty blocker list!  ;-)
blocking2.0: --- → ?
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
What's happening here is that this check <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#1882> resolves to true, so we cancel the deletion.

mBogusNode is set here: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/nsTextEditRules.cpp#1102> because GetFirstChild here returns null: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/nsTextEditRules.cpp#1126>, so we think that the document is empty, and inject a bogus node.  This happens because we're initing the editor too early, before we've had a chance to see any possible children for the <body> element.

For example, this test case is not affected by the same problem:

data:text/html,<body onload="document.body.setAttribute('contenteditable', 'true')">foo</body>
Attached patch WIP (obsolete) — — Splinter Review 
Boris, this is basically what we discussed on IRC today.  It doesn't work, since SinkContext::CloseContainer doesn't even get called!  Am I missing something really badly?
Attachment #489743 - Flags: feedback?(bzbarsky)
Comment on attachment 489743 [details] [diff] [review]
WIP

The SinkContext stuff is the old HTML parser.  You need to change nsHtml5TreeBuilder::elementPopped as well.

For the rest, won't this make non-parser inserted content editable bodies not work at all?

Also, why put the new DoneAddingChildren on nsGenericHTMLElement and not nsHTMLBodyElement?
Attachment #489743 - Flags: feedback?(bzbarsky) → feedback-
(In reply to comment #3)
> Comment on attachment 489743 [details] [diff] [review]
> WIP
> 
> The SinkContext stuff is the old HTML parser.  You need to change
> nsHtml5TreeBuilder::elementPopped as well.

Ah, right.  Trying that right now...

> For the rest, won't this make non-parser inserted content editable bodies not
> work at all?

Yes, and that's a problem I need to address by switching to NS_IMPL_NS_NEW_HTML_ELEMENT_CHECK_PARSER for body elements and differentiate between those types of elements.  This will be in the future iterations of this patch.

> Also, why put the new DoneAddingChildren on nsGenericHTMLElement and not
> nsHTMLBodyElement?

No particular reason other than going through the fastest route to experiment with this!  :-)  This will also be addressed in future iterations.
Attached patch Part 1: main fix (obsolete) — — Splinter Review 

        Attachment #489743 -
        Attachment is obsolete: true

        Attachment #489929 -
        Flags: review?(bzbarsky)

    
    

    
  


  

        Attachment #489933 -
        Flags: review?(bzbarsky)

    
    

    
  
The patches are looking pretty good so far.  I'm doing some more testing, and will attach more patches if I find more problems with the patches.

    
  
Whiteboard: [has patch][needs review bz]

    
    

    
  


  
This patch triggers two more assertions in this test case, both coming from AfterEdit, because we don't have a bogus node in the DOM with these changes.  Solving the underlying issue (bug 163838) will solve all of the assertions here.

        Attachment #489970 -
        Flags: review?(bzbarsky)

    
    

    
  


  
These patches caused a failure in layout/reftests/bugs/579985-1.html, which was caused because a bogus node was being inserted in the document under a contenteditable="false" body, and could not be removed later because we didn't use to allow deletion of a bogus node.  This patch fixes both of these problems.

        Attachment #490096 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 489929 [details] [diff] [review]
Part 1: main fix

The comment above HandleContentEditableElement talks about attributes...  but there are no attributes involved here.  Fix the comment?

Adding yet another virtual function call to the BindToTree path is sadmaking, but we should optimize that in a separate bug... (perhaps using a node flag here).

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

    
    

    
  


  
It turns out that a lot of stuff is depending on the broken InsertElementTxn behavior.  Let's localize the check for read-only parent to bogus node creation for now.

BTW, IsModifiableNode is no-op for plaintext editors, which is cool.

        Attachment #490096 -
        Attachment is obsolete: true

        Attachment #490190 -
        Flags: review?(bzbarsky)

        Attachment #490096 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 489930 [details] [diff] [review]
Part 2: Correct the definition of EndOfDocument for the HTML editor in order to fix some tests which have been relying on the bogus node existing in non-empty documents

I don't understand this change.  Why do we want different behavior for nsHTMLEditor and nsEditor here?

    
    

    
  
Comment on attachment 489931 [details] [diff] [review]
Part 3: Handle dynamic changes to the editable documents and create/remove the bogus node if needed

Looks good... but then why do we need the from-parser hackery in the first patch here?

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

    
    

    
  
Comment on attachment 489933 [details] [diff] [review]
Part 4: Add some more tests

r=me (very quick skim; please tell me if you want a serious review here)

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

    
    

    
  
Comment on attachment 489970 [details] [diff] [review]
Part 5: Adjust the assertion annotation for an assertion which now gets triggered more because we're doing things correctly

r=me

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

    
    

    
  
Comment on attachment 489931 [details] [diff] [review]
Part 3: Handle dynamic changes to the editable documents and create/remove the bogus node if needed

Actually, one issue here.  It'd be better to not sync mess with the DOM under those notifications; can we put that messing off on a script runner?

        Attachment #489931 -
        Flags: review+ → review-

    
    

    
  
Comment on attachment 490190 [details] [diff] [review]
Part 6: Actually allow the bogus node to be deleted from the document, and check if the parent is modifiable when inserting an element

r=me

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

    
    

    
  


  
(In reply to comment #12)
> Comment on attachment 489929 [details] [diff] [review]
> Part 1: main fix
> 
> The comment above HandleContentEditableElement talks about attributes...  but
> there are no attributes involved here.  Fix the comment?
> 
> Adding yet another virtual function call to the BindToTree path is sadmaking,
> but we should optimize that in a separate bug... (perhaps using a node flag
> here).

So, actually with the stuff in part 3, none of the code changes in this patch are necessary any more.  So I will just let the test part live in this patch.

        Attachment #489929 -
        Attachment is obsolete: true

    
    

    
  
(In reply to comment #14)
> Comment on attachment 489930 [details] [diff] [review]
> Part 2: Correct the definition of EndOfDocument for the HTML editor in order to
> fix some tests which have been relying on the bogus node existing in non-empty
> documents
> 
> I don't understand this change.  Why do we want different behavior for
> nsHTMLEditor and nsEditor here?

In the case of the plaintext editor, we're basically dealing with a DIV with a text node under it, with a trailing BR node sometimes.  In the case of the HTML editor, we're dealing with an arbitrary DOM.

Consider this example:

<body contenteditable><p>foo</p></body>

Here, EndOfDocument will put the selection's parent on <body> with offset 1, which places the caret "after" the paragraph, which is not what we want.  Furthermore, entering text will append text to the next line, which is not again what we want.  We should basically differentiate between container and non-container elements in nsHTMLEditor::EndOfDocument, which is what this patch does.

    
    

    
  
(In reply to comment #16)
> Comment on attachment 489933 [details] [diff] [review]
> Part 4: Add some more tests
> 
> r=me (very quick skim; please tell me if you want a serious review here)

Not really!  :-)

    
    

    
  
> We should basically differentiate between container and non-container elements
> in nsHTMLEditor::EndOfDocument

OK, but why can't we just do that in nsEditor::EndOfDocument?  Other than a tiny bit of extra work, it'll have no ill effect on plaintext editor, right?

    
    

    
  


  
We don't need to notify the edit rules object if the modification to the document happens outside of the editable area (for example, to nodes somewhere in the parent chain of a contenteditable node).  This should improve performance a bit, and also fixes a test failure in test_replace_root_element.html caused by us injecting a bogus node when a document with an editable body element is replaced.

        Attachment #490417 -
        Flags: review?(bzbarsky)

    
    

    
  


  
(In reply to comment #23)
> > We should basically differentiate between container and non-container elements
> > in nsHTMLEditor::EndOfDocument
> 
> OK, but why can't we just do that in nsEditor::EndOfDocument?  Other than a
> tiny bit of extra work, it'll have no ill effect on plaintext editor, right?

Yes, it should result in the same thing for plaintext editors.  I moved the changes to nsEditor::EndOfDocument.

        Attachment #489930 -
        Attachment is obsolete: true

        Attachment #490427 -
        Flags: review?(bzbarsky)

        Attachment #489930 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 490427 [details] [diff] [review]
Part 2: Correct the definition of EndOfDocument for the HTML editor in order to fix some tests which have been relying on the bogus node existing in non-empty documents

r=me; we really need to switch this gunk to nsINode sometime....

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

    
    

    
  
Comment on attachment 490417 [details] [diff] [review]
Part 7: Don't notify the edit rules object for document modifications outside of the editable area

r=me

We should start passing an nsINode aContainer....

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

    
    

    
  
(In reply to comment #26)
> Comment on attachment 490427 [details] [diff] [review]
> Part 2: Correct the definition of EndOfDocument for the HTML editor in order to
> fix some tests which have been relying on the bogus node existing in non-empty
> documents
> 
> r=me; we really need to switch this gunk to nsINode sometime....

Not only that, we should start using the internal content interfaces instead of DOM interfaces in all of the editor code, which would be a huge project...

(In reply to comment #27)
> Comment on attachment 490417 [details] [diff] [review]
> Part 7: Don't notify the edit rules object for document modifications outside
> of the editable area
> 
> r=me
> 
> We should start passing an nsINode aContainer....

Filed bug 612159 for that.
Whiteboard: [has patch][needs review bz] → [needs landing]

    
    

    
  
Patch 3 has review-. Why it has been landed together with the final check-in? It has been caused bug 612565.

    
    

    
  
Er... that should have been a review+

    
    

    
  
Er, no.  review- is right; see comment 18.

    
    

    
  
I had addressed comment 18, but for some reason I forgot to attach it, and thought that I had r+ on the modified patch :(

It is http://hg.mozilla.org/mozilla-central/rev/54d00e3411ab.  Boris, is that what you were thinking of?

    
    

    
  
Yeah, that looks good.

    
    

    
  
Was this a regression from something recently or has it been around for some time?

    
    

    
  
(In reply to comment #35)
> Was this a regression from something recently or has it been around for some
> time?

As far as I can tell, this problem has existed since we added support for contenteditable first in 1.9.

    
  
Depends on: 614919

    
  
Depends on: 643786

    
  
Depends on: 667512

    
  
No longer depends on: 667512

    
  
Depends on: 1248187

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: