[webkit-reviews] review denied: [Bug 20083] end and then right arrow moves one character too far : [Attachment 226817] Patch

Mon Mar 17 14:59:53 PDT 2014

Ryosuke Niwa <rniwa at webkit.org> has denied Deepak Mittal
<deepak.m1 at samsung.com>'s request for review:
Bug 20083: end and then right arrow moves one character too far
https://bugs.webkit.org/show_bug.cgi?id=20083

Attachment 226817: Patch
https://bugs.webkit.org/attachment.cgi?id=226817&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.