[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 104402] Got rid of textInputController

[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 104402: Got rid of textInputController
https://bugs.webkit.org/attachment.cgi?id=104402&action=review

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


r- due to various stylistic issues in the tests.

> LayoutTests/editing/input/caret-at-the-edge-of-contenteditable.html:6
> +<div style="width:auto; position:absolute; visibility:hidden"
id="single-digit">0</div>

It seems like you should just use span for this.

> LayoutTests/editing/input/reveal-caret-of-multiline-contenteditable.html:6
> +<div style="width:auto; position:absolute; visibility:hidden"
id="single-digit">0</div>

Ditto.

> LayoutTests/editing/input/reveal-caret-of-multiline-contenteditable.html:38
> +00<br>
> +01<br>
> +02<br>
> +03<br>
> +04<br>
> +05<br>
> +06<br>
> +07<br>
> +08<br>
> +09<br>
> +10<br>
> +11<br>
> +12<br>
> +13<br>
> +14<br>
> +15<br>
> +16<br>
> +17<br>
> +18<br>
> +19<br>
> +20<br>
> +21<br>
> +22<br>
> +23<br>
> +24<br>
> +25<br>
> +26<br>
> +27<br>
> +28<br>
> +29<br>
> +30<br>

Can we auto generate this by script?

> LayoutTests/editing/input/reveal-caret-of-multiline-input.html:37
> +00
> +01
> +02
> +03
> +04
> +05
> +06
> +07
> +08
> +09
> +10
> +11
> +12
> +13
> +14
> +15
> +16
> +17
> +18
> +19
> +20
> +21
> +22
> +23
> +24
> +25
> +26
> +27
> +28
> +29
> +30

Ditto about generating this by script.

> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:26
> +var viewportHeight = window.innerHeight;
> +var input = document.getElementById("input");
> +var topmostElement = document.getElementById("topmost-element");
> +var initialOffset = offsetFromViewportTop(topmostElement);

All these variables are making this test less readable.

> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:30
> +    while (offsetFromViewportTop(topmostElement) < initialOffset)
> +	   eventSender.keyDown("pageUp");

Can we extract this as a function in reveal-utilities.js?  It's repeated in 3
files.	Just use document.body.children[0] instead of giving it an id and then
defining a variable.

> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:40
> +// Ensure that the content-editable div is in the middle of the viewport.
> +var viewportMiddle = Math.round(viewportHeight / 2);
> +var offsetOfInput = offsetOfMiddleFromViewportTop(input);
> +var centered = Math.abs(offsetOfInput - viewportMiddle) <= 3;
> +document.getElementById("results").innerHTML += "ScrollVertically: " +
(centered ? "PASS" : "FAIL<br/>");
> +if (!centered)
> +    document.getElementById("results").innerHTML += "viewportMiddle: " +
viewportMiddle + ", offsetOfInput: " + offsetOfInput;

Can we make this a function?  Also, I'd re-organize the conditions so that I
don't have to define "centered"