Closed Bug 970363 Opened 9 years ago Closed 9 years ago

Pressing Delete key before a non editable span in a contenteditable block causes both the span to be deleted and a following character.

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Version:
20 Branch
Platform:
x86_64
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla30

People

(Reporter: firefox, Assigned: firefox)

References

Details

(Keywords: reproducible, testcase, Whiteboard: [bugday-20140212])

Attachments

(3 files, 4 obsolete files)

testcase from comment 0
132 bytes, text/html
Details
Change return type of PeekOffset - No Logic changes.
17.44 KB, patch
Details | Diff | Splinter Review
Fix Bug 970363
3.78 KB, patch
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/32.0.1700.107 Safari/537.36

Steps to reproduce:

Pressing the delete key before a non editable span in a contenteditable block causes both the span to be deleted and the first character in the following text node.


Tested in Firefox 29.0a2 and Firefox 22.

Load following HTML in Firefox:

<html>
<body  contenteditable="true">
<BR>
<div>
<span contenteditable="false">Span Contents</span>helloworld
</div>
</body>
</html>

Then:

1. Put Insertion point before "Span Contents"
2. Press the "Delete" key.
Expected: 
Actual: The span is removed plus the 'h' for the following text node is also deleted.



Actual results:

The span is removed plus the 'h' for the following text node is also deleted.


Expected results:

Just the span should be removed.
Reproduced with 2014-02-12-03-02-01-mozilla-central-firefox-30.0a1.en-US.linux-x86_64.
Blocks: contenteditable
Status: UNCONFIRMED → NEW
Component: Untriaged → Editor
Ever confirmed: true
OS: Windows 7 → All
Product: Firefox → Core
Whiteboard: [bugday-20140212]
Keywords: reproducible, testcase
The span containing "Span Contents" is contained in a nsInlineFrame.

nsInlineFrame overrides nsFrame PeekOffsetCharacter behaviour so it never returns true.

If this specialized nsInlineFrame behaviour is removed then this bug doesn't occur.
(In reply to Tom from comment #2)
> The span containing "Span Contents" is contained in a nsInlineFrame.
> 
> nsInlineFrame overrides nsFrame PeekOffsetCharacter behaviour so it never
> returns true.
> 
> If this specialized nsInlineFrame behaviour is removed then this bug doesn't
> occur.

Hey Tom, I feel like you might be on to something here.  I don't think it makes sense to completely remove that code, but I think we should be returning true after this line: <http://hg.mozilla.org/mozilla-central/annotate/1238ef12b996/layout/generic/nsInlineFrame.cpp#l170>.  Is that what you meant?
Flags: needinfo?(firefox)
Attached patch mochitest for bug. (obsolete) — — Splinter Review 
This is a patch containing a mochitest for this bug.

Using a "todo_is" to show bug.
Flags: needinfo?(firefox)
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #3)
> Hey Tom, I feel like you might be on to something here.  I don't think it
> makes sense to completely remove that code, but I think we should be
> returning true after this line:
> <http://hg.mozilla.org/mozilla-central/annotate/1238ef12b996/layout/generic/
> nsInlineFrame.cpp#l170>.  Is that what you meant?

Yes that is what I did when testing, just inserted a "return true" at line 171.

The nsInlineFrame::PeekOffsetCharacter logic has been the same since hg rev 1. But I'm pleased you think it would be a good change to make.

I have yet to run the full mochitest suite with the change made. If It doesn't causes any problems I will add a patch with that change, to this bug report.

Thanks!
Yeah I think that fix makes sense.  Do you have access to the try server?  Running mochitests locally can take a *long* amount of time, so I suggest you use the try server instead.
(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #6)
> Yeah I think that fix makes sense.  Do you have access to the try server? 
> Running mochitests locally can take a *long* amount of time, so I suggest
> you use the try server instead.

No I don't have access to a try server. (Unless of course there is public access to one that I don't know about).
(In reply to Tom from comment #7)
> (In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness,
> emailapocalypse) from comment #6)
> > Yeah I think that fix makes sense.  Do you have access to the try server? 
> > Running mochitests locally can take a *long* amount of time, so I suggest
> > you use the try server instead.
> 
> No I don't have access to a try server. (Unless of course there is public
> access to one that I don't know about).

The try server is a resource that you can use to test your patches on all of the platforms that we support, please see <https://wiki.mozilla.org/ReleaseEngineering/TryServer> for more information.  It's heavily used by developers to try out the patches that they have to make sure that they don't regress anything.  We have a TBPL instance for try here: <https://tbpl.mozilla.org/?tree=Try>

Getting access to the try server requires commit access level 1, please see <http://www.mozilla.org/hacking/commit-access-policy/> for the exact requirements.  To make things easier, I filed bug 975596 (you're CCed) to give you commit access level 1 and have vouched for you!  Please see <http://www.mozilla.org/hacking/committer/> for the information that you need to provide on the bug.

Please let me know if you need help with any of this process!
Assignee: nobody → firefox
It turned out, at least to my current understanding, that returning true in nsInlineFrame::PeekOffsetCharacter is not a sensible thing to do.

The implementation of nsIFrame::PeekOffset traverses leaf frames not every frame. (see nsIFrame::GetFrameFromDirection use of NS_NewFrameTeaversal)

This means that, nsIFrame::PeekOffset, should never move to a nsInLineFrame that is not empty. (as an aside Its possible that nsIFrame::PeekOffset is called on a non empty nsInlineFrame itself).


So In summary I think the problem lies in nsIFrame::PeekOffset iself.

Consider the following two examples (assuming content editable blocks with readonly spans. {} is the IP.):

example 1: abc{}<span></span>def
example 2: abc{}<span>HELLO</span>def

Expected behaviour of pressing the right arrow:
example 1: IP moves after the 'd' character.
example 2: IP moves after the span close tag and before the 'd' character.


Current pseudo nsFrame::PeekOffset frame moving behaviour and PeekOffsetCharacter return value in brackets:
example 1: 'abc' nsTextFrame -> nsInlineFrame (false) -> 'def' nsTextFrame (true)
example 2: 'abc' nsTextFrame -> 'HELLO' nsTextFrame (false) -> 'def' nsTextFame (true)

It seems entirely sensible for both the nsInlineFrame and readonly nsTextFrame to return false. (as the IP/Selection doesn't not end there)

However PeekCharacterOffset doesn't really return enough explicit information to nsFrame::PeekOffset as to why it returns false, (was it because the text isn't selectable or is there no text?). The nsFrame::PeekOffset implementation can work it out by exampling the return offsets:

As shown by this try request.

https://tbpl.mozilla.org/?tree=Try&rev=009a30702eb4
https://hg.mozilla.org/try/rev/009a30702eb4

While this commit fixes the bug, and doesn't cause any test fails (unlike just returning true in nsInlineFrame does)
I think a better way to fix this would be to add more return information from PeekOffsetCharacter as to why it returns false. (or even change the bool return to an enum). PeekOffsetCharacter is only used 6 times in code, and has 6 different implementations. (nsBRFrame, nsContainerFrame, nsFrame, nsInlineFrame, nsTextFrame)

However I would quite like to know if others agree before continue this way.
Flags: needinfo?(ehsan)
Altering the return value of PeekOffsetCharacter sounds fine to me, and I think it's much better than the solution in your try push, but we should ultumately seek roc's blessing on that approach.  Need-info'ing him.

(Also, please make sure to also test on debug builds on try to make sure we don't break debugging assertions that we have in our debug builds.)
Flags: needinfo?(ehsan) → needinfo?(roc)
Latest try submission (with debug builds this time - thanks for the correction :) )

The proposed solution is as follows:

I've changed PeekOffsetNoAmount, PeekOffsetCharacter + PeekOffsetWord to return a FrameSearchResult enum, where 0 is stop and non zero is continue. Implementations can optionally set extra bits to specify a reason. (currently only empty and unselectable are defined)

This allows nsIFrame::PeekOffset to behave different for different continue reasons. 

https://tbpl.mozilla.org/?tree=Try&rev=303136673f5d
https://hg.mozilla.org/try/rev/303136673f5d
(In reply to Tom Hindle from comment #11)
> Latest try submission (with debug builds this time - thanks for the
> correction :) )
> 
> https://tbpl.mozilla.org/?tree=Try&rev=303136673f5d
> https://hg.mozilla.org/try/rev/303136673f5d

Looks like this caused a bunch of test fails. I need to look into why these failed.
The basic idea sounds good to me.

+  /** Flags for PeekOffsetCharacter, PeekOffsetNoAmount, PeekOffsetWord return values.
+    */
+  enum FrameSearchResult { STOP = 0x00, CONTINUE = 0x01, CONTINUE_EMPTY = 0x10, CONTINUE_UNSELECTABLE = 0x100 };

Put the values on separate lines.
Document them carefully.
Use consecutive bit values, either 0x01, 0x02, 0x04 etc or 1 << 0, 1 << 1, 1 << 2 etc.

Thanks!
Flags: needinfo?(roc)
Try attempt, with all passing tests.

Also fixed and documented the enum values.

https://tbpl.mozilla.org/?tree=Try&rev=258dace58b3f
https://hg.mozilla.org/try/rev/258dace58b3f
Status: NEW → ASSIGNED
Attached patch proposed fix. (obsolete) — — Splinter Review 
[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: ContentEditable blocks have selection bugs when using keyboard.
Testing completed (on m-c, etc.): Run all mochitest on try Server.
Risk to taking this patch (and alternatives if risky): ?
String or IDL/UUID changes made by this patch: None
Attachment #8389395 - Flags: review?(ehsan)
Attachment #8389395 - Flags: approval-mozilla-aurora?
Comment on attachment 8389395 [details] [diff] [review]
proposed fix.

Review of attachment 8389395 [details] [diff] [review]:
-----------------------------------------------------------------

The general idea here is sane as I've said before.  roc is a better reviewer for this patch, he's the owner of the Layout module.

Also, we usually get the patch landed on mozilla-central and then do a risk evaluation to decide whether it should be uplifted to Aurora.  Clearing the approval flag for now.
Attachment #8389395 - Flags: review?(roc)
Attachment #8389395 - Flags: review?(ehsan)
Attachment #8389395 - Flags: approval-mozilla-aurora?
Comment on attachment 8389395 [details] [diff] [review]
proposed fix.

Review of attachment 8389395 [details] [diff] [review]:
-----------------------------------------------------------------

Please split this patch into two patches: a patch that changes the return type of PeekOffset which preserves existing behavior, and another patch which makes the actual behavior change you want here.
Attachment #8389395 - Flags: review?(roc) → review-

        Attachment #8379972 -
        Attachment is obsolete: true

        Attachment #8389395 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix Bug 970363 (obsolete)
        — 
          — Splinter Review
      

    


  

    
  

        Attachment #8389799 -
        Flags: review?(roc)

    
  

        Attachment #8389800 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 8389799 [details] [diff] [review]
Change return type of PeekOffset - No Logic changes.

Review of attachment 8389799 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those changes

::: layout/generic/nsFrame.cpp
@@ +6182,3 @@
>                                                aPos->mAmount == eSelectCluster);
>  
> +        if (keepLooking) {

if (keepLooking != FOUND) {

::: layout/generic/nsIFrame.h
@@ +666,5 @@
> +  /** Flags for PeekOffsetCharacter, PeekOffsetNoAmount, PeekOffsetWord return values.
> +    */
> +  enum FrameSearchResult { 
> +    // Peek found a appropriate offset within frame.
> +    STOP = 0x00, 

Call this FOUND?

@@ +670,5 @@
> +    STOP = 0x00, 
> +    // try next frame for offset. 
> +    CONTINUE = 0x1, 
> +    // offset not found because the frame was empty of text.  
> +    CONTINUE_EMPTY = 0x2 | CONTINUE, 

Please delete trailing whitespace

        Attachment #8389799 -
        Flags: review?(roc) → review+

    
    

    
  
Comment on attachment 8389800 [details] [diff] [review]
Fix Bug 970363

Review of attachment 8389800 [details] [diff] [review]:
-----------------------------------------------------------------

Good stuff!

::: layout/generic/test/test_bug970363.html
@@ +8,5 @@
> +  <title>Test for Bug 970363</title>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> + 

Just delete this line

        Attachment #8389800 -
        Flags: review?(roc) → review+

    
    

    
  


  

        Attachment #8389799 -
        Attachment is obsolete: true

        Attachment #8390544 -
        Flags: review?(roc)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix Bug 970363
        — 
          — Splinter Review
      

    


  

        Attachment #8389800 -
        Attachment is obsolete: true

        Attachment #8390548 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 8390544 [details] [diff] [review]
Change return type of PeekOffset - No Logic changes.

Removing "review?" (added in error because I didn't realize what review+ ment)

        Attachment #8390544 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 8390548 [details] [diff] [review]
Fix Bug 970363

Removing "review?" (added in error because I didn't realize what review+ ment)

        Attachment #8390548 -
        Flags: review?(roc)

    
  
Keywords: checkin-needed

    
    

    
  
Thanks again for your patch, Tom!  :-)

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6e179a67d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/f52f0588f431

Thanks for the patch, Tom! One request, can you please update your Hg committer information with your name and email address? Thanks :)
Flags: in-testsuite+
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/1d6e179a67d7
https://hg.mozilla.org/mozilla-central/rev/f52f0588f431
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

    
    

    
  
Verified with 2014-06-22-03-02-03-mozilla-central-firefox-33.0a1.ru.linux-x86_64.

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