[webkit-reviews] review denied: [Bug 65027] Inserting text into scrolled out text controls should bring them into the center of the view, not on an edge : [Attachment 102340] now with images

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Mikhail Naganov
<mnaganov at chromium.org>'s request for review:
Bug 65027: Inserting text into scrolled out text controls should bring them
into the center of the view, not on an edge
https://bugs.webkit.org/show_bug.cgi?id=65027

Attachment 102340: now with images
https://bugs.webkit.org/attachment.cgi?id=102340&action=review

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


r- due to style issues.

> LayoutTests/editing/input/caret-at-the-edge-of-contenteditable.html:9
> +    contentEditable.style.cssText = contentEditable.style.cssText +
";width:" + (singleDigitWidth * contentEditableSize) + "px";

Please do
contentEditable.style.width = (singleDigitWidth * contentEditableSize) + 'px';
instead

> LayoutTests/editing/input/caret-at-the-edge-of-contenteditable.html:17
> +    contentEditable.focus();
> +    // Move the caret to the beginning of the input.
> +    var range = getSelection().collapse(contentEditable.firstChild, 0);
> +    var editableContent = contentEditable.textContent;
> +    if (window.eventSender) {
> +	   for (var i = 0; i < contentEditableSize * 1.2; ++i)
> +	       eventSender.keyDown(editableContent[i]);
> +    }

Why don't you put all of this in where you call runTest?  Then you wouldn't
need this script element or head for that matter.  It seems there's no benefit
in putting this in a function.

> LayoutTests/editing/input/caret-at-the-edge-of-input.html:13
> +    var inputEdit = document.getElementById("input-edit");
> +    inputEdit.focus();
> +    var inputEditContent = inputEdit.value;
> +    inputEdit.setSelectionRange(0, 0);
> +    if (window.eventSender) {
> +	   for (var i = 0; i < inputEdit.size * 1.2; ++i)
> +	       eventSender.keyDown(inputEditContent[i]);
> +    }

Ditto about putting this code into where you currently call runTest();

> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:3
> +<script>

You can put this script inside the script element where you call runTest.

> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:44
> +<div style="height:2000px"></div>

Again, I'd use any value above 100% in percentage instead of a fixed 2000px.

> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:47
> +<div style="height:2000px"></div>

Ditto.

> LayoutTests/editing/input/reveal-edit-on-input-vertically.html:3
> +<script>

Ditto about merging this script element with one below.