Bug 119511 - Adding testcase for verifying editing behavior for up/down caret movement between lines.
Summary: Adding testcase for verifying editing behavior for up/down caret movement bet...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Arpita Bahuguna
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-05 21:47 PDT by Arpita Bahuguna
Modified: 2013-08-13 01:14 PDT (History)
2 users (show)

See Also:


Attachments
Patch (5.14 KB, patch)
2013-08-06 02:53 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (7.58 KB, patch)
2013-08-07 06:00 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (5.93 KB, patch)
2013-08-12 04:20 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff
Patch (5.61 KB, patch)
2013-08-13 00:24 PDT, Arpita Bahuguna
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arpita Bahuguna 2013-08-05 21:47:30 PDT
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.
Comment 1 Arpita Bahuguna 2013-08-06 02:53:19 PDT
Created attachment 208179 [details]
Patch
Comment 2 Ryosuke Niwa 2013-08-06 10:04:35 PDT
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.
Comment 3 Ryosuke Niwa 2013-08-06 10:06:05 PDT
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.
Comment 4 Arpita Bahuguna 2013-08-07 06:00:44 PDT
Created attachment 208262 [details]
Patch
Comment 5 Arpita Bahuguna 2013-08-07 22:27:39 PDT
(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.
Comment 6 Arpita Bahuguna 2013-08-07 22:28:11 PDT
(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.
Comment 7 Ryosuke Niwa 2013-08-11 23:17:07 PDT
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.
Comment 8 Arpita Bahuguna 2013-08-12 01:43:17 PDT
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.
Comment 9 Arpita Bahuguna 2013-08-12 04:20:53 PDT
Created attachment 208532 [details]
Patch
Comment 10 Arpita Bahuguna 2013-08-12 04:21:39 PDT
Comment on attachment 208532 [details]
Patch

Added a patch with a layout testcase for verifying WebKit's behavior.
Comment 11 Ryosuke Niwa 2013-08-12 10:53:06 PDT
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.
Comment 12 Arpita Bahuguna 2013-08-13 00:24:15 PDT
Created attachment 208603 [details]
Patch
Comment 13 Arpita Bahuguna 2013-08-13 00:26:17 PDT
(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.
Comment 14 WebKit Commit Bot 2013-08-13 01:14:13 PDT
Comment on attachment 208603 [details]
Patch

Clearing flags on attachment: 208603

Committed r153992: <http://trac.webkit.org/changeset/153992>
Comment 15 WebKit Commit Bot 2013-08-13 01:14:16 PDT
All reviewed patches have been landed.  Closing bug.