Bug 20083 – end and then right arrow moves one character too far

• Status: UNCONFIRMED
• Component: Forms

[Ojan Vafai]

Input enough text in a multiline text input (e.g. textarea, div with contentEditable, etc.) so that the text wraps across multiple lines.  Now go to the first line, hit "end", then hit the right arrow key.
Actual behavior: cursor is after the first character on the next line.
Expected behavior: cursor is at the beginning of next line.

If you then use the left/right arrow keys to navigate around, you'll see that the rest behaves appropriately.

[Deepak Mittal]

Is Anybody working on this issue..?

[Deepak Mittal]

Created attachment 225437 [details]
Patch

[WebKit Commit Bot]

Attachment 225437 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/VisiblePosition.cpp:329:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
ERROR: Source/WebCore/editing/VisiblePosition.cpp:325:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 225437 [details]
Patch

Attachment 225437 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5701056944144384

New failing tests:
editing/deleting/delete-inserts-br-after-button.html
fast/events/multiline-link-arrow-navigation.html
editing/selection/move-by-word-visually-inline-block-positioned-element.html
editing/selection/move-by-word-visually-multi-line.html
editing/selection/move-left-right.html
editing/selection/move-by-word-visually-mac.html

[Build Bot]

Created attachment 225440 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 225437 [details]
Patch

Attachment 225437 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4958735734669312

New failing tests:
editing/selection/move-by-word-visually-multi-line.html
editing/deleting/delete-inserts-br-after-button.html
editing/selection/move-by-word-visually-inline-block-positioned-element.html
editing/selection/move-by-word-visually-mac.html
fast/events/multiline-link-arrow-navigation.html

[Build Bot]

Created attachment 225441 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 225437 [details]
Patch

Attachment 225437 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6751850221010944

New failing tests:
editing/deleting/delete-inserts-br-after-button.html
fast/events/multiline-link-arrow-navigation.html
editing/selection/move-by-word-visually-inline-block-positioned-element.html
editing/selection/move-by-word-visually-multi-line.html
editing/selection/move-left-right.html
editing/selection/move-by-word-visually-mac.html

[Build Bot]

Created attachment 225445 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Deepak Mittal]

Created attachment 226580 [details]
Patch

[Deepak Mittal]

Created attachment 226652 [details]
Patch

[Build Bot]

Comment on attachment 226652 [details]
Patch

Attachment 226652 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4809832037089280

New failing tests:
editing/selection/endkey-rightarrow-move.html

[Build Bot]

Created attachment 226661 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Deepak Mittal]

Created attachment 226663 [details]
Patch

[Build Bot]

Comment on attachment 226663 [details]
Patch

Attachment 226663 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5789861969657856

New failing tests:
editing/selection/endkey-rightarrow-move.html

[Build Bot]

Created attachment 226665 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226663 [details]
Patch

Attachment 226663 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4880350568251392

New failing tests:
editing/selection/endkey-rightarrow-move.html

[Build Bot]

Created attachment 226666 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Deepak Mittal]

Created attachment 226670 [details]
Patch

[Build Bot]

Comment on attachment 226670 [details]
Patch

Attachment 226670 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6674753544781824

New failing tests:
editing/selection/endkey-rightarrow-move.html

[Build Bot]

Created attachment 226676 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226670 [details]
Patch

Attachment 226670 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4890878069964800

New failing tests:
editing/selection/endkey-rightarrow-move.html

[Build Bot]

Created attachment 226678 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226670 [details]
Patch

Attachment 226670 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6199707076395008

New failing tests:
editing/selection/endkey-rightarrow-move.html

[Build Bot]

Created attachment 226689 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Deepak Mittal]

I wanted to set the cursor in the text box at the end of the line But in the mac their is no 'end' key so 

eventSender.keyDown("end"); is not working and my test case is getting failed in mac, Can some one let me know how to simulate 'end' key in javascript so that it will work in the mac..

I know fun+rightArrow is 'end' key in mac..but I wanted to use  that in the javascript ..please let me kow if you have any idea..

[Deepak Mittal]

I wanted to set the cursor in the text box at the end of the line But in the mac their is no 'end' key so 

eventSender.keyDown("end"); is not working and my test case is getting failed in mac, Can some one let me know how to simulate 'end' key in javascript so that it will work in the mac..

I know fun+rightArrow is 'end' key in mac..but I wanted to use  that in the javascript ..please let me kow if you have any idea..

[Deepak Mittal]

Created attachment 226806 [details]
Patch

[Build Bot]

Comment on attachment 226806 [details]
Patch

Attachment 226806 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6397816201019392

New failing tests:
editing/selection/dump-as-markup.html
editing/selection/endkey-rightarrow-move.html

[Build Bot]

Created attachment 226814 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226806 [details]
Patch

Attachment 226806 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5595703678074880

New failing tests:
editing/selection/dump-as-markup.html
editing/selection/endkey-rightarrow-move.html

[Build Bot]

Created attachment 226815 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 226806 [details]
Patch

Attachment 226806 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5315581616062464

New failing tests:
editing/selection/dump-as-markup.html
editing/selection/endkey-rightarrow-move.html

[Build Bot]

Created attachment 226816 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Deepak Mittal]

Created attachment 226817 [details]
Patch

[Deepak Mittal]

Please review this patch..

[Ryosuke Niwa]

Comment on attachment 226817 [details]
Patch

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

Thanks for the patch but r- due to various issues.

> Source/WebCore/ChangeLog:7
> +

Please describe what caused the bug and how you fixed it.

> Source/WebCore/ChangeLog:15
> +        position should not change,check added for the same.

There should be a space after a punctuation character such as , and . Please fix elsewhere in your patch as as well.

> Source/WebCore/editing/VisiblePosition.cpp:325
> +                    if (!box->nextLeafChild() && box->prevLeafChild() && toInlineTextBox(box)->len() == 1 && positionOnRight.deprecatedEditingOffset()) {

Why are we checking that len() is 1 and positionOnRight.deprecatedEditingOffset() is not 0?
This should definitely be explained in the change log.

Also, why are we not fixing leftVisuallyDistinctCandidate?

> Source/WebCore/editing/VisiblePosition.cpp:439
> -    ASSERT(right != *this);
> +    // The position will be same when we are at the end of the line by selecting
> +    // The end key.and then we select right arrow key.
> +    // ASSERT(right != *this);

Why are we disabling this assertion?
I don't think this comment makes sense since rightVisuallyDistinctCandidate is supposed to return null position when it has reached the end of the line,
in which case it'll be distinct from *this.
r- because of this change.

> LayoutTests/editing/selection/endkey-rightarrow-move-expected.txt:2
> +
> +PASS

This output doesn't tell us anything about the nature of the test.

> LayoutTests/editing/selection/endkey-rightarrow-move.html:1
> +<html>

Missing DOCTYPE.

> LayoutTests/editing/selection/endkey-rightarrow-move.html:3
> +        <meta http-equiv="content-type" content="text/html; charset=UTF-8">

Since this document doesn't contain non-ASCII characters, we don't need this meta element.

> LayoutTests/editing/selection/endkey-rightarrow-move.html:7
> +            if (window.testRunner) {
> +                testRunner.dumpAsText();
> +            }

No curly brackets around a single line statement.

> LayoutTests/editing/selection/endkey-rightarrow-move.html:17
> +                var CaretPos = 0;   // IE Support
> +                if (document.selection) {
> +                ctrl.focus ();
> +                var Sel = document.selection.createRange ();
> +                Sel.moveStart ('character', -ctrl.value.length);
> +                CaretPos = Sel.text.length;
> +                }
> +                // Firefox support
> +                else if (ctrl.selectionStart || ctrl.selectionStart == '0')

Why do we need to support IE and Firefox in this test?
Also, indentation is all messed up here.

> LayoutTests/editing/selection/endkey-rightarrow-move.html:33
> +                if(ctrl.setSelectionRange)
> +                {
> +                    ctrl.focus();
> +                    ctrl.setSelectionRange(pos,pos);
> +                }
> +                else if (ctrl.createTextRange) {
> +                var range = ctrl.createTextRange();
> +                range.collapse(true);
> +                range.moveEnd('character', pos);
> +                range.moveStart('character', pos);
> +                range.select();
> +                }

I don't think we should support other browsers here as it's making the code incomprehensible.

> LayoutTests/editing/selection/endkey-rightarrow-move.html:37
> +                var para = document.getElementById("para");

Please don't use abbreviations like para. Spell out paragraph.
But really, we should be able to use querySelector('textarea') instead.

> LayoutTests/editing/selection/endkey-rightarrow-move.html:39
> +                testRunner.execCommand('MoveToEndOfLine',false,null);

Use getSelection().modify('Move', 'Forward', 'LineBoundary') instead.

> LayoutTests/editing/selection/endkey-rightarrow-move.html:41
> +                eventSender.keyDown("rightArrow");

Use Use getSelection().modify('Move', 'Forward', 'Line') instead.

> LayoutTests/editing/selection/endkey-rightarrow-move.html:48
> +        <textarea id="para" width="100" height="100" rows="4" cols="30">Atw3schools.comyouwilllearnhowto make a website. We offer free tutorials in all web development technologies.</textarea>

We usually prefer using contenteditable to test these cursor movement behaviors.

[Deepak Mittal]

Created attachment 227049 [details]
Patch

[Deepak Mittal]

(In reply to comment #37)
> (From update of attachment 226817 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=226817&action=review
> 
> Thanks for the patch but r- due to various issues.
> 
> > Source/WebCore/ChangeLog:7
> > +
> done

> Please describe what caused the bug and how you fixed it.
> 
> > Source/WebCore/ChangeLog:15
> > +        position should not change,check added for the same.
done
> 
> There should be a space after a punctuation character such as , and . Please fix elsewhere in your patch as as well.
> 
> > Source/WebCore/editing/VisiblePosition.cpp:325
> > +                    if (!box->nextLeafChild() && box->prevLeafChild() && toInlineTextBox(box)->len() == 1 && positionOnRight.deprecatedEditingOffset()) {
> 
> Why are we checking that len() is 1 and positionOnRight.deprecatedEditingOffset() is not 0?
> This should definitely be explained in the change log.
> 
> Also, why are we not fixing leftVisuallyDistinctCandidate?
> change done

> > Source/WebCore/editing/VisiblePosition.cpp:439
> > -    ASSERT(right != *this);
> > +    // The position will be same when we are at the end of the line by selecting
> > +    // The end key.and then we select right arrow key.
> > +    // ASSERT(right != *this);
> 
> Why are we disabling this assertion?
> I don't think this comment makes sense since rightVisuallyDistinctCandidate is supposed to return null position when it has reached the end of the line,
> in which case it'll be distinct from *this.
> r- because of this change.
> This is required as the offset should not change when 'end' key is selected and the rightArrow key is selected, as offset is remain same when cursor moves to the start of the second line, so this assertion should not be present.

> > LayoutTests/editing/selection/endkey-rightarrow-move-expected.txt:2
> > +
> > +PASS
> 
> This output doesn't tell us anything about the nature of the test.
> Changes Done.
> > LayoutTests/editing/selection/endkey-rightarrow-move.html:1
> > +<html>
> 
> Missing DOCTYPE.
> Changes Done.
> > LayoutTests/editing/selection/endkey-rightarrow-move.html:3
> > +        <meta http-equiv="content-type" content="text/html; charset=UTF-8">
> Changes Done.
> Since this document doesn't contain non-ASCII characters, we don't need this meta element.
> 
> > LayoutTests/editing/selection/endkey-rightarrow-move.html:7
> > +            if (window.testRunner) {
> > +                testRunner.dumpAsText();
> > +            }
> Changes Done.
> No curly brackets around a single line statement.
> 
> > LayoutTests/editing/selection/endkey-rightarrow-move.html:17
> > +                var CaretPos = 0;   // IE Support
> > +                if (document.selection) {
> > +                ctrl.focus ();
> > +                var Sel = document.selection.createRange ();
> > +                Sel.moveStart ('character', -ctrl.value.length);
> > +                CaretPos = Sel.text.length;
> > +                }
> > +                // Firefox support
> > +                else if (ctrl.selectionStart || ctrl.selectionStart == '0')
> 
> Why do we need to support IE and Firefox in this test?
> Also, indentation is all messed up here.
> Changes Done.
> > LayoutTests/editing/selection/endkey-rightarrow-move.html:33
> > +                if(ctrl.setSelectionRange)
> > +                {
> > +                    ctrl.focus();
> > +                    ctrl.setSelectionRange(pos,pos);
> > +                }
> > +                else if (ctrl.createTextRange) {
> > +                var range = ctrl.createTextRange();
> > +                range.collapse(true);
> > +                range.moveEnd('character', pos);
> > +                range.moveStart('character', pos);
> > +                range.select();
> > +                }
> 
> I don't think we should support other browsers here as it's making the code incomprehensible.
> Changes Done.
> > LayoutTests/editing/selection/endkey-rightarrow-move.html:37
> > +                var para = document.getElementById("para");
> 
> Please don't use abbreviations like para. Spell out paragraph.
> But really, we should be able to use querySelector('textarea') instead.
> Changes Done.
> > LayoutTests/editing/selection/endkey-rightarrow-move.html:39
> > +                testRunner.execCommand('MoveToEndOfLine',false,null);
> 
> Use getSelection().modify('Move', 'Forward', 'LineBoundary') instead.
> Changes Done.
> > LayoutTests/editing/selection/endkey-rightarrow-move.html:41
> > +                eventSender.keyDown("rightArrow");
> 
> Use Use getSelection().modify('Move', 'Forward', 'Line') instead.
> I have to use eventSender.keyDown("rightArrow"); as the issue is specific to end key and right arrow.
I have tried with getSelection().modify('Move', 'Forward', 'character') 
But it is not working for me..
> > LayoutTests/editing/selection/endkey-rightarrow-move.html:48
> > +        <textarea id="para" width="100" height="100" rows="4" cols="30">Atw3schools.comyouwilllearnhowto make a website. We offer free tutorials in all web development technologies.</textarea>
> 
> We usually prefer using contenteditable to test these cursor movement behaviors.
This issue is not happening in the contenteditable and happened very easily wit textArea for me..

[Build Bot]

Comment on attachment 227049 [details]
Patch

Attachment 227049 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6465951293767680

New failing tests:
editing/selection/move-left-right.html

[Build Bot]

Created attachment 227056 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 227049 [details]
Patch

Attachment 227049 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5163446626680832

New failing tests:
editing/selection/move-left-right.html

[Build Bot]

Created attachment 227064 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Deepak Mittal]

Created attachment 227258 [details]
Patch

[Build Bot]

Comment on attachment 227258 [details]
Patch

Attachment 227258 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4888443091943424

New failing tests:
editing/selection/move-by-word-visually-single-space-one-element.html
editing/selection/move-by-word-visually-null-box.html
editing/selection/move-by-word-visually-multi-line.html

[Build Bot]

Created attachment 227264 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 227258 [details]
Patch

Attachment 227258 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5962973042442240

New failing tests:
editing/selection/move-by-word-visually-single-space-one-element.html
editing/selection/move-left-right.html
editing/selection/move-by-word-visually-null-box.html
editing/selection/move-by-word-visually-multi-line.html

[Build Bot]

Created attachment 227268 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 227258 [details]
Patch

Attachment 227258 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6589662189584384

New failing tests:
editing/selection/move-by-word-visually-single-space-one-element.html
editing/selection/move-left-right.html
editing/selection/move-by-word-visually-null-box.html
editing/selection/move-by-word-visually-multi-line.html

[Build Bot]

Created attachment 227273 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Deepak Mittal]

Created attachment 227277 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 227277 [details]
Patch

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

> Source/WebCore/editing/VisiblePosition.cpp:442
> -    ASSERT(right != *this);
> +    // The position will be same when we are at the end of the line by selecting
> +    // The end key.and then we select right arrow key.
> +    // ASSERT(right != *this);

Again, this is wrong.

[Deepak Mittal]

(In reply to comment #52)
> (From update of attachment 227277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227277&action=review
> 
> > Source/WebCore/editing/VisiblePosition.cpp:442
> > -    ASSERT(right != *this);
> > +    // The position will be same when we are at the end of the line by selecting
> > +    // The end key.and then we select right arrow key.
> > +    // ASSERT(right != *this);
> 
> Again, this is wrong.

When we select the 'end' and then selecting 'rightArrow' key then the offset position should remain same, That is the reason for removing this assert,as Without my changes the value is coming always one offset more than the *this visiblePosition offset..
And Assert is not failing, 
But we want same position when "overshot to right' happen so that cursor can be placed before the first alphabet of first word in the next line, not after the first alphabet.
so this assert need to be removed as we need to maintain the same offset.