873883 - inconsistencies in cursor movement and deletion inside contenteditable element

Status: NEW

(Core :: DOM: Editor, defect)

Description

[mail]

Attachment: contenteditable-test.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release)
Build ID: 20130512193848

Steps to reproduce:

1. Load the attached file
2. Click in the text, move the cursor from the start of the first paragraph to the end of the last paragraph
3. Delete individual imaes, canvas elements, SVGs, noneditable elements and noneditable elements with editable islands


Actual results:

The cursor could move inbetween and delete IMG elements and SVG elements consistently and correctly. The other elements showed the following errors:

CANVAS:
-- the cursor jumps one extra character when moving across a CANVAS element in the middle of a paragraph
-- the cursor does not move inbetween two CANVAS elements
-- the cursor does not move in front of a CANVAS element that starts a paragraph or behind one that ends a paragraph.

noneditable elements:
The same issues as CANVAS elements AND additionally:
-- the backspace key does not remove noneditable elements

Noneditable elements with editable islands:
The same issues as noneditable elements AND additionally:
-- the cursor will not move in to the editable islands when moving back and forth.  

contenteditable:



Expected results:

The cursor should move between all the elements (SVGS, IMGs, CANVASes, noncontenteditable elements, noncontenteditable elements with contenteditable islands) in a consistent manner. The cursor should be able to move between any two elements and hitting backspace while the cursor is just behind an element should delete it entirely.

Comment 1

[mail]

A similar bug has been filed against Chromium. It also has bugs in this area, but they are different: https://code.google.com/p/chromium/issues/detail?id=242110

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

I don't really know anything about cursor handling code, but maybe it's about time to learn.  Clarification question -- you included backspace behavior in the bug.  Surely real-world editors don't use browsers' built-in backspace behavior?

Comment 3

[mail]

You are right. Backspace no longer matters much.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Ehsan, do you know where to look to fix this, or who I could ask?  Two issues I spot are:

1) The cursor will not go in between two consecutive non-editable elements or canvas elements using left/right arrow keys.  (Ctrl+Left does work!)

2) Nor will it necessarily go to the beginning of a line that starts with a non-editable element or canvas, or the end of a line that ends with one.  Sometimes it goes right after/before the element.  (But it depends how you try to get there.)

3) When positioned to the right of two consecutive non-editable elements or canvas elements, pressing the left arrow key sometimes jumps past both of them, to before whatever is before them.

It seems some issues identified in the initial report have been resolved already (editable islands).

Comment 5

[(no longer active)]

(In reply to :Aryeh Gregor (working until September 2) from comment #4)
> Ehsan, do you know where to look to fix this, or who I could ask?  Two
> issues I spot are:
> 
> 1) The cursor will not go in between two consecutive non-editable elements
> or canvas elements using left/right arrow keys.  (Ctrl+Left does work!)
> 
> 2) Nor will it necessarily go to the beginning of a line that starts with a
> non-editable element or canvas, or the end of a line that ends with one. 
> Sometimes it goes right after/before the element.  (But it depends how you
> try to get there.)
> 
> 3) When positioned to the right of two consecutive non-editable elements or
> canvas elements, pressing the left arrow key sometimes jumps past both of
> them, to before whatever is before them.
> 
> It seems some issues identified in the initial report have been resolved
> already (editable islands).

I can't really speak to any of the specific issues since I haven't debugged them, but I can point you to the code that you need to start looking at.  Debugging these issues is usually straightforward.

You usually start from nsFrameSelection::MoveCaret().  That functions is the central code that deals with moving the selection in any direction while holding any combination of modifier keys.  The aDirection argument is in either logical or visual coordinates, depending on aMovementStyle.  aContinueSelection will be true when shift is being held plus arrow keys.  aAmount tells you whether you're moving one character, one word, and so on.  Note that we have prefs that modify our behavior about things such as whether to include spaces when moving word by word, etc.  We use these prefs to implement various platform specific behaviors which is nice if you want for example to debug an OS X issue on Linux.

MoveCaret() first computes some information about how it's supposed to move the selection, and then starts the real work by first calling GetPrimaryFrameForFocusNode() to find the first frame and the offset under the current caret position.  It then calls nsIFrame::PeekOffset() to compute where the selection is supposed to move to.  This code navigates the frame tree in the direction computed by MoveCaret() to find a corresponding frame where the selection would move to.  Issues such as us failing to stop in the middle of the two canvases typically happen by us getting something wrong inside PeekOffset().  Once we find the target frame, we get the corresponding content node and offset and then call TakeFocus() to modify the selection according to the destination node and offset.

Usually I start debugging issues like this by breaking in MoveCaret(), double checking that the arguments we're passed to make sense (which they usually do) and then delve down into PeekOffset() to see where we go into the weeds.

A bit about how PeekOffset() works.  There's essentially two main code paths:

* When you're moving a certain number of characters to the left or right, we enter a loop where we first check to see if the amount we're supposed to move to is an offset into the current frame we're looking at (we start at |this|) and if not, we call GetFrameFromDirection() to find the correct frame.  Typical places where things go wrong are where we end up on a line boundary, when we're skipping non-selectable frames (think contenteditable=false for example) by calling IsSelected(), etc.

* When you're moving a certain number of lines to the top or bottom, we use the line iterator to check which line we're looking at, and we keep moving by calling GetNextPrevLineFromeBlockFrame().  When that function encounters a block frame it tries to move inside it looking for a frame inside it.  We also try to land on frames above/below where we started moving from.  Again a usual suspect in this path would be skipping non-selectable frames.

One word of advice is looking at the history of the code you're trying to understand.  A lot of the existing checks are coming from bugs that we've fixed in the past, so looking at the history oftentimes helps you understand the existing setup better.

Many parts of this code are tested in test_reftests_with_caret.html, but there's probably other tests spread across random parts of the tree too.

Hope this helps!

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

So I looked at this, and the problem seems pretty straightforward.  The code skips any run of non-selectable content, but the desired behavior is that we should only skip one non-selectable item, and place the cursor in between that item and the next non-selectable item.  An "item" here would I guess be one element.  Do we actually want to change behavior here?  If so, any idea how I'd go about doing it?  Maybe remember the current element (assuming there necessarily is one?) when I receive the first CONTINUE_UNSELECTABLE, then treat the next CONTINUE_UNSELECTABLE as FOUND if the element is different?

(Looks like we'll miss each other, but I'll see your response when I get back.)

Comment 7

[(no longer active)]

I think we do want to change our behavior here.  However I'm not 100% sure which specific code you were looking at....  CONTINUE_UNSELECTABLE is only returned from text frames, but none of the non-selectable elements here are text nodes, so I'm lost as to how these are related to each other...  Can you please elaborate?

(For future reference, Mats also knows this code, in case I end up being away when you need me...)