[webkit-reviews] review denied: [Bug 55552] Text selection changes unexpectedly when dragging out of the <input> : [Attachment 87828] 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 87828: Patch
https://bugs.webkit.org/attachment.cgi?id=87828&action=review

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

r- because you're missing LayoutTests/ChangeLog

> LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:14
> +    eventSender.mouseDown();
> +    x = floatedEditable.offsetLeft + floatedEditable.offsetWidth + 5;
> +    eventSender.mouseMoveTo(x, y);
> +    eventSender.mouseUp();

I believe you need to add leapForward between mouseDown and mouseUp or
otherwise this test becomes flaky.

> LayoutTests/editing/selection/resources/select-out-of-floated-editable.js:27
> +    return el.isContentEditable ? window.getSelection().baseOffset :
el.selectionStart;

Please do not use arbitrary abbreviations like "el".

> LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:15
> +To test manually, drag from the middle of the editable div to the
> +right, into the non-floated text. The selection should go to the end
> +of the input element and not jump to the beginning.

Are you trying to fit everything in 80 columns? No need to do that here.

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

Can't we have just one pre/p?  It seems silly to have two separate elements for
automated / manual test.

> LayoutTests/editing/selection/select-out-of-floated-contenteditable.html:20
> +<script type="text/javascript"
src="resources/select-out-of-floated-editable.js"></script>

We don't normally specify type attribute.

> LayoutTests/editing/selection/select-out-of-floated-input.html:22
> +<p id="result">
> +</p>
> +<pre id="console"></pre>
> +<script type="text/javascript"
src="resources/select-out-of-floated-editable.js"></script>

Ditto.

> LayoutTests/editing/selection/select-out-of-floated-textarea.html:22
> +<p id="result">
> +</p>
> +<pre id="console"></pre>
> +<script type="text/javascript"
src="resources/select-out-of-floated-editable.js"></script>

Ditto.