Closed Bug 602141 Opened 9 years ago Closed 9 years ago

Right arrow navigation broken on later contenteditable content on single line

Categories

(Core :: DOM: Selection, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- final+

People

(Reporter: megabyte, Assigned: ehsan)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files)

Attached file Testcases
Navigation using the right arrow does not work on subsequent contenteditable node on a single line.

Steps to reproduce (see testcase for other examples):
1. Start with two spans, second with contenteditable set.
2. Click on text in second span.
3. Navigate text with right arrow key.

Expected results: cursor moves right

Actual results: cursor does not move beyond length(span) - length(preceding text) + 1 if preceding span

Left arrow still works.
blocking2.0: --- → ?
Keywords: regression
Aaron, while I agree that this bug sucks, is there any particular reason that you requested the blocking flag here?
Assignee: nobody → ehsan
It's breaking some internal webapps for me (in particular because I needed the arrow behavior to work around the multitude of "can't delete/backspace text at the beginning or end of contenteditable spans" bugs), so I was just hoping that this didn't make it out to final.  Since this is a fairly recent regression, I was hoping there was more chance to get this fixed than the other contenteditable bugs (though I would actually like to see those fixed more).  The combination of this bug and the others makes inline contenteditable essentially unusable.  If nobody gets to it before the next few days (I'm pressed for time right now), I'll look for the regression window.
Keywords: testcase
Absolutely.  If it's breaking web apps, it should block.

It would really be helpful if we have a regression range...  I've done a bunch of recent work on the caret movement, which might have affected this.
Actually I think I saw an instance of this while typing a very long email in Gmail...
I've seen a similar problem in larger contenteditable divs where the right arrow ends up sending you to the next line rather than right, but I haven't been able to reproduce consistently.
(In reply to comment #5)
> I've seen a similar problem in larger contenteditable divs where the right
> arrow ends up sending you to the next line rather than right, but I haven't
> been able to reproduce consistently.

Yes, this is exactly what I saw in Gmail!
Smaug said that he's been seeing this in the location bar as well...
blocking2.0: ? → final+
This is a regression from http://hg.mozilla.org/mozilla-central/rev/2d960a2dd288.

What happens here is that the changeset in question modified the forward code path of the nsTextFrame::PeekOffsetCharacter function in a slightly incorrect way.  In order to determine whether we're looking at the end of the line, we compare iter.GetSkippedOffset() to trimmed.GetEnd(), which is incorrect, because iter.GetSkippedOffset() should be used in what I'm going to call "textrun offsets", while trimmed's members are in "content node offsets".  Therefore, right navigation is broken for all textnodes which have textruns consisting of text from textnodes before the node in question.

Take the first line of the testcase as an example.  Here we have a textrun with the following contents:

"\nnavigable__navigable|unnavigable"

with length=33.  This is constructed for the following three textnodes:

"\n" "navigable__" "navigable|unnavigable"

The length of the last textrun is 22 here.

So, let's look at the check in question.  When the cursor is after the vertical bar character, iter.GetSkippedOffset() is going to return 22, and trimmed.GetEnd() is also going to return 22.  So, we mistakenly think that we're at the end of the line, and don't increment the content offset, instead we return, and then the caret movement code tries to put the caret on the BR frame (which is the next frame on this line after the 3rd textframe, and fails along the way because the ancestor validity check fails, that's why the caret just appears to be stuck and doesn't move.

I have a patch, I'm working on converting this test case to a set of automated tests.  I'll post the patch when I'm done.
Attached patch Patch (v1)Splinter Review
Attachment #481695 - Flags: review?(bzbarsky)
Assignee: ehsan → nobody
Component: Editor → Selection
QA Contact: editor → selection
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Whiteboard: [has patch][needs review bz]
Comment on attachment 481695 [details] [diff] [review]
Patch (v1)

r=me
Attachment #481695 - Flags: review?(bzbarsky) → review+
Whiteboard: [has patch][needs review bz] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/fa5ecde710e3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla2.0b8
Depends on: 604078
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.