Bug 119511 – Adding testcase for verifying editing behavior for up/down caret movement between lines.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Arpita Bahuguna]

Sequence to reproduce the issue:

1. For the given test page place the caret anywhere on the first line but not at the start of the line. (Placing the caret towards the end of the first line shall further pronounce the behavior).
2. Press the "up" arrow key to move the caret to the start of the line. This should place the caret at the start of the first line.
3. From there press the "down" arrow key. This should ideally move the caret to the start of the following (second) line. However notice that the caret is instead displaced by an offset equivalent to our start position on the first line.

The caret should be placed at the start of the second line.

[Arpita Bahuguna]

Created attachment 208179 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 208179 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208179&action=review

> Source/WebCore/editing/FrameSelection.cpp:1051
> -    if (granularity == LineGranularity || granularity == ParagraphGranularity)
> +    if (userTriggered != UserTriggered && (granularity == LineGranularity || granularity == ParagraphGranularity))

This doesn't make much sense. The whole point of this variable is to keep track of the x position when the user tried to move the caret upwards or downwards.

[Ryosuke Niwa]

Comment on attachment 208179 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208179&action=review

> LayoutTests/editing/selection/move-caret-to-next-line-expected.txt:3
> +PASS caretRectAfterFirstDownArrow.left is caretRectAtStartOfLine.left
> +PASS caretRectAfterFirstDownArrow.top is caretRectAtStartOfLine.top
> +

This test output is very mysterious. It doesn't tell us what it's testing and why the output is correct.

> LayoutTests/editing/selection/move-caret-to-next-line.html:10
> +<p>
> +Test for <a href="http://bugs.webkit.org/show_bug.cgi?id=119511">119511</a>Caret should be placed somewhere in between this line and then moved to the start using up arrow key.
> +</p>

This should be in description() function.

> LayoutTests/editing/selection/move-caret-to-next-line.html:17
> +    var testElement = document.getElementById('test');
> +    testElement.focus();
> +    

We don't need to focus since focus follows selection in WebKit.

[Arpita Bahuguna]

Created attachment 208262 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #2)
> (From update of attachment 208179 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208179&action=review
> 
> > Source/WebCore/editing/FrameSelection.cpp:1051
> > -    if (granularity == LineGranularity || granularity == ParagraphGranularity)
> > +    if (userTriggered != UserTriggered && (granularity == LineGranularity || granularity == ParagraphGranularity))
> 
> This doesn't make much sense. The whole point of this variable is to keep track of the x position when the user tried to move the caret upwards or downwards.

I actually wanted to reset in nextLinePosition() and prevLinePosition() methods within the piece of code that returns the position at the start or end if the editable block if further traversal is not possible. But since that wasn't possible I tried this approach.
However, in the new patch am now making use of the isSameLine() method to figure whether the start and the returned positions are on the same line or not.

[Arpita Bahuguna]

(In reply to comment #3)
> (From update of attachment 208179 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208179&action=review
> 
> > LayoutTests/editing/selection/move-caret-to-next-line-expected.txt:3
> > +PASS caretRectAfterFirstDownArrow.left is caretRectAtStartOfLine.left
> > +PASS caretRectAfterFirstDownArrow.top is caretRectAtStartOfLine.top
> > +
> 
> This test output is very mysterious. It doesn't tell us what it's testing and why the output is correct.
> 
> > LayoutTests/editing/selection/move-caret-to-next-line.html:10
> > +<p>
> > +Test for <a href="http://bugs.webkit.org/show_bug.cgi?id=119511">119511</a>Caret should be placed somewhere in between this line and then moved to the start using up arrow key.
> > +</p>
> 
> This should be in description() function.
> 
> > LayoutTests/editing/selection/move-caret-to-next-line.html:17
> > +    var testElement = document.getElementById('test');
> > +    testElement.focus();
> > +    
> 
> We don't need to focus since focus follows selection in WebKit.

Have made the aforementioned changes and submitted another patch.

[Ryosuke Niwa]

Comment on attachment 208262 [details]
Patch

The current behavior matches that of TextEdit so it’s probably intentional.
Is new behavior expected on Windows or Linux?
We need to tie this new behavior to editingBehavior.

[Arpita Bahuguna]

Our behavior for up/down caret movement between lines matches the textEdit behavior for mac, windows and linux.

When no further up or down movement is possible, the caret is moved either to the start (for up) or end (for down) of the line. This however should NOT reset the 'x' start position of the caret.

FF on the other hand resets the 'x' position which is not as per the default text editor behavior on various platforms. Bug for the same has been logged here: https://bugzilla.mozilla.org/show_bug.cgi?id=903976

The 'x' position should always be maintained while traversing up/down between lines.

However, we need to add a layout testcase for verifying this behavior on WebKit.

[Arpita Bahuguna]

Created attachment 208532 [details]
Patch

[Arpita Bahuguna]

Comment on attachment 208532 [details]
Patch

Added a patch with a layout testcase for verifying WebKit's behavior.

[Ryosuke Niwa]

Comment on attachment 208532 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208532&action=review

> LayoutTests/editing/selection/verify-editing-behavior-for-line-granularity-expected.txt:3
> +Testcase for bug 119511: Adding testcase for verifying editing behavior for up/down caret movement between lines. To manually verify the issue, place the caret somewhere in between the first line and then press the up arrow key. This should move the caret to the start of the first line. Next press the down arrow key. This should result in the caret being placed on the next line offset by an amount equivalent to the start "x" position of the caret on the previous line. Similarly, forward caret movement too can be verified.

It's redundant to say this is for bug 119511 since that's evident from svn log.

[Arpita Bahuguna]

Created attachment 208603 [details]
Patch

[Arpita Bahuguna]

(In reply to comment #11)
> (From update of attachment 208532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=208532&action=review
> 
> > LayoutTests/editing/selection/verify-editing-behavior-for-line-granularity-expected.txt:3
> > +Testcase for bug 119511: Adding testcase for verifying editing behavior for up/down caret movement between lines. To manually verify the issue, place the caret somewhere in between the first line and then press the up arrow key. This should move the caret to the start of the first line. Next press the down arrow key. This should result in the caret being placed on the next line offset by an amount equivalent to the start "x" position of the caret on the previous line. Similarly, forward caret movement too can be verified.
> 
> It's redundant to say this is for bug 119511 since that's evident from svn log.

Thanks for the review R Niwa. Have removed the bug ID and title from the description.

[WebKit Commit Bot]

Comment on attachment 208603 [details]
Patch

Clearing flags on attachment: 208603

Committed r153992: <http://trac.webkit.org/changeset/153992>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.