602141 - Right arrow navigation broken on later contenteditable content on single line

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Aaron Kaluszka]

Attachment: 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.

Comment 1

[(no longer active)]

Aaron, while I agree that this bug sucks, is there any particular reason that you requested the blocking flag here?

Comment 2

[Aaron Kaluszka]

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.

Comment 3

[(no longer active)]

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.

Comment 4

[(no longer active)]

Actually I think I saw an instance of this while typing a very long email in Gmail...

Comment 5

[Aaron Kaluszka]

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.

Comment 6

[(no longer active)]

(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!

Comment 7

[(no longer active)]

Smaug said that he's been seeing this in the location bar as well...

Comment 8

[(no longer active)]

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.

Comment 9

[(no longer active)]

Attachment: Patch (v1)

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 11

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/fa5ecde710e3