[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 102007] patch with tests

Tue Jul 26 12:22:56 PDT 2011

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 102007: patch with tests
https://bugs.webkit.org/attachment.cgi?id=102007&action=review

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

r- because the test has many stylistic issues to be addressed.

> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:1
> +<head>

Missing DOCTYPE or does this bug only reproduces in quirks mode?

> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:14
> +    return element.getClientRects()[0].top +
(element.getClientRects()[0].height >> 1);

Why on the earth are we using bit-shift instead of division here?

> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:21
> +    var upmost = document.getElementById("upmost");

Nit: I don't think upmost is a descriptive name.

> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:31
> +    var centered = Math.abs(offsetOfMiddleFromViewportTop(input) -
(viewportHeight >> 1)) <= 3;

Ditto about bit-shift.

> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:32
> +    document.getElementById("results").innerHTML += "ScrollVertically: " +
(centered ? "PASS" : "FAIL");

I'd like to see some useful information that aid us debugging the issue when
this test fails instead of just "FAIL".

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

Nit: I would have used % value instead (e.g. 200%).

> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:41
> +<div style="overflow:hidden; width:150px; white-space:nowrap; border: thin
solid black" contenteditable="true" id="input"></div>

I don't understand why we need overflow, width, whitespace, etc... for this
test to work.

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

Ditto.

> LayoutTests/editing/input/reveal-edit-on-input-vertically.html:1
> +<head>

Same comments on this test as well.