[webkit-reviews] review denied: [Bug 55552] Text selection changes unexpectedly when dragging out of the <input> : [Attachment 88970] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Alice Boxhall
<aboxhall at chromium.org>'s request for review:
Bug 55552: Text selection changes unexpectedly when dragging out of the <input>
https://bugs.webkit.org/show_bug.cgi?id=55552

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88970&action=review

Okay, I like the patch in general but there are few nits I want you to address
before landing this patch. I'd say r- for now given that you're not a committer
yet.

> LayoutTests/ChangeLog:38
> +
> +2011-04-03  Alice Boxhall  <aboxhall at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Text selection changes unexpectedly when dragging out of the
<input>
> +	   https://bugs.webkit.org/show_bug.cgi?id=55552
> +
> +	   Tests that dragging outside of a contenteditable, input or textarea
selects to the end of the
> +	   element, rather than jumping back to the beginning.
> +
> +	   * editing/selection/resources/select-out-of-floated-editable.js:
Added.
> +	   *
editing/selection/select-out-of-floated-contenteditable-expected.txt: Added.
> +	   * editing/selection/select-out-of-floated-contenteditable.html:
Added.
> +	   * editing/selection/select-out-of-floated-input-expected.txt: Added.

> +	   * editing/selection/select-out-of-floated-input.html: Added.
> +	   * editing/selection/select-out-of-floated-textarea-expected.txt:
Added.
> +	   * editing/selection/select-out-of-floated-textarea.html: Added.
> +

You have two change log entires. Please fix.

> LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:12
> +    x = floatedEditable.offsetLeft + floatedEditable.offsetWidth + 5;

You need to call leap forward here.

> LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:37
> +							 :
floatedEditable.value;

I don't think we normally align after tertiary operator like this.  Please do:
var inputText = floatedEditable.isContentEditable ? floatedEditable.textContent

    : floatedEditable.value;

> LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:5
> +<style>
> +  div { float: left; }
> +  p { padding-top: 1em; }
> +</style>

I don't know why we need to specify these as style rules. p already has some
margins and you can specify float: left in the inline style declaration.

> LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:19
> +<p id="result">
> +</p>
> +<pre id="console"></pre>

Nit: It seems odd that you have <p> and </p> on a separate line even though
it's empty and right beneath it, you have <pre></pre>.

> Source/WebCore/ChangeLog:15
> +	   (WebCore::targetPositionForSelectionEndpoint): When dragging from an
editable element, check that
> +	       the endpoint is not outside the element. If it is, translate the
point into a local point within the
> +	       editable element.

Nit: I don't think we intend in change log like this. Please align the start of
each line to (. Also, it's odd that line break exists between an article and
the noun that immediately follows it. We usually truncate a line at more
natural places between "within" and "the" or between "local point" and "within"


> Source/WebCore/page/EventHandler.cpp:634
> +	   // If selecting out of an editable element, translate the endpoint
into a local point within the 
> +	   // editable element, to avoid confusing behaviour which can occur
when dragging into a node which is
> +	   // earlier in the DOM.

Nit: this comment is too verbose.  We can either omit the comment entirely or
simply state that we're respecting editing boundary.
We certainly don't need to say that we're translating the endpoint to a local
point within the editable element because that's self-evident from the code.
"avoid confusing behavior when dragging out of an editable element" will be
better but we need to be a little more specific than "confusing".