[webkit-reviews] review denied: [Bug 55552] Text selection changes unexpectedly when dragging out of the <input> : [Attachment 87828] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat Apr 2 08:37:14 PDT 2011
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.
More information about the webkit-reviews
mailing list