Bug 9312 – REGRESSION: Selection bug in new text fields when selecting past the first letter

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Daniele Metilli]

The new text fields behave differently when you select text inside them.

Please see the test case, it should be pretty clear.

Version of WebKit: r14733.

[Daniele Metilli]

Created attachment 8713 [details]
Test case

[Adele Peterson]

I can't reproduce this problem.  Maybe we fixed this one.  Can anyone confirm?

(In reply to comment #2)
> I can't reproduce this problem.  Maybe we fixed this one.  Can anyone confirm?
> 

I can still reproduce. Dragging across the field from right to left, the highlight disappears as soon as I move past the first (leftmost) letter.

[Adele Peterson]

oops, I can repro now.  I must've been using the wrong version.

[Joost de Valk (AlthA)]

i guess this one needs a Radar then.

[Alice Liu]

<rdar://problem/4631642>

Created attachment 9597 [details]
Work in progress

This patch fixes what's currently causing crazy highlighting and selection in nodes outside the text field as you select from within it outside. It also fixes an assertion failure when you select from the text under the field all the way to after the last letter in the field. The behavior with this patch is that dragging past the first letter behaves as expected, but dragging past the last letter  (starting, say, from the middle of the text field) is broken. This is due to a regression from r14172 (the fix for bug 8159), which actually affects all selections: you can see it by starting a selection in the middle of the first paragraph after the text field in the test case and dragging down into the space between the paragraphs. Instead of selecting everything up to the end of the first paragraph, the selection goes from the beginning of the paragraph to where you started dragging.

Created attachment 9601 [details]
Patch, still no change log and tests

The patch fixes this bug and the regression from fixing bug 8159. I ran the /editing and /fast tests with it, and the only differences were two progressions.

Created attachment 9602 [details]
Patch, still no change log and tests

Removed unrelated test

[Darin Adler]

Comment on attachment 9602 [details]
Patch, still no change log and tests

This change looks pretty good to me.

[Darin Adler]

Comment on attachment 9602 [details]
Patch, still no change log and tests

r=me, but I want to see ChangeLog and a test case -- I suppose the "progressions" might substitute for a test case

I *hate* *hate* *hate* the caretMaxOffset change, but that's because I still dream of a world where we don't use [element, 1] to express "past a replaced element" and that's just not the world we live in.

[Darin Adler]

Comment on attachment 9602 [details]
Patch, still no change log and tests

Oops, forgot to review+ this -- see my earlier comments please.

Created attachment 9603 [details]
Patch, now with change log and a layout test

[Darin Adler]

Comment on attachment 9603 [details]
Patch, now with change log and a layout test

r=me -- Justin should read the code too, but he can do it after it's checked in.

[Justin Garcia]

Comment on attachment 9603 [details]
Patch, now with change log and a layout test

             Position p = previousVisuallyDistinctCandidate(m_end);
+            if (p.isNull() && endRoot && endRoot->isShadowNode())
+                p = Position(endRoot->shadowParentNode(), maxDeepOffset(endRoot->shadowParentNode()));

             Position p = nextVisuallyDistinctCandidate(m_start);
+            if (p.isNull() && startRoot && startRoot->isShadowNode())
+                p = Position(startRoot->shadowParentNode(), 0);

I think that the offsets should be swapped.  They also need to be swapped inside the for loops.
This is minor and it can be changed with a later checkin if I'm right.  r=me

[Adele Peterson]

committed in r 15558