Bug 65027 – Inserting text into scrolled out text controls should bring them into the center of the view, not on an edge

• Component: HTML Editing
• Status: RESOLVED FIXED

[Mikhail Naganov]

Created attachment 101726 [details]
Reference - mobile Safari

Consider the following scenario (the example deals with m.cnn.com just for an easier comparison with mobile Safari version):
 - open m.cnn.com
 - having a small browser window, scroll down to a text input control (this may be zip code input for weather, or the search field at the bottom of the screen)
 - put text cursor inside the text control
 - scroll away from the text control
 - start typing -- the control will be brought on the screen but at the very bottom of the view

In contrast, mobile Safari scrolls the text control in the middle of the view, so it's much easier to spot it.

I believe, the desktop version should behave the same.

I've attached screenshots for comparison.

[Mikhail Naganov]

Created attachment 101727 [details]
Patch

[Mikhail Naganov]

Created attachment 101728 [details]
Desktop version - with patch

[Alexey Proskuryakov]

Does svn blame tell when and why the current behavior was chosen? FWIW, TextEdit on Snow Leopard scrolls its document to move insertion point to the center, which is not exactly the same case, but it supports this change.

Nested scrolling (e.g. a form inside a subframe inside a long document) may be a case to consider. Mobile platforms don't get as much of that in normal browsing due to frame flattening.

The fix doesn't seem limited to forms, so changing component.

[Mikhail Naganov]

The roots are in this change: https://bugs.webkit.org/show_bug.cgi?id=7113 (author: Adele).

It says: "Changed scrollRectToVisible call so that it will align to the edge instead of trying to center the selection. Centering looks funny when it happens after typing, deleting, moving the cursor, etc."

I've checked out, what's the difference. If you have a long text edit, not fitting in a view, typing at the edge of the view will:
  - leave the cursor at the edge without my patch; I'm not sure this is nice, because the cursor can hardly be recognized near the window edge, even despite cursor's blinking;
  - make the cursor to be centered in the view with my patch, so I can see not only text before the cursor, but also after him, which seems more convenient; yes, the cursor jumps back to the center when it's close to the edge, but this doesn't look funny to me :)

I can't say what mobile Safari will do, because it doesn't allow me to edit text on the repro page from the original issue. Should I fire a bug about this?

[Alexey Proskuryakov]

Comment on attachment 101727 [details]
Patch

What happens when scrolling horizontally is certainly an interesting additional question, and it's great that looking at svn blame made us think of it. Looking at native Cocoa text controls (such as search field in Safari) suggests that scrolling to center the insertion point will be correct.

> Should I fire a bug about this?

Thank you for offering to file a bug. It is known that iOS doesn't support contenteditable. Anyway, the correct behavior on that test shouldn't differ from <input type=text> on any platform, so you could try that.

Of course, the final patch will need to include tests for both horizontal and vertical scrolling, in both forms and contenteditable. I'm going to mark r- due to lack of tests now, given that several people who would care about such change are already CC'ed.

[Mikhail Naganov]

Created attachment 102007 [details]
patch with tests

I've added tests. They pass with my patch and fail without -- as required.

I wrote a test for horizontal scrolling, but have found out that the control is being centered properly without my patch as well, so it makes no sense adding it.

[Ryosuke Niwa]

Comment on attachment 102007 [details]
patch with tests

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.

[Mikhail Naganov]

(In reply to comment #7)
> (From update of attachment 102007 [details])
> 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?
>

Added.
 
> > 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?
> 

Because it produces an integer result. Changed to Math.round(... / 2).

> > 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.
> 

Changed to "topmost-element".

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

Changed as well.

> > 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".
>

Added.
 
> > 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%).
> 

Doesn't work. The div doesn't have content. 200% of zero gives zero.

> > 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.
> 

Left out width (because the div doesn't have content initially) and border (so you can spot it visually, and it looks like an edit box).

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

The same here.

> > LayoutTests/editing/input/reveal-edit-on-input-vertically.html:1
> > +<head>
> 
> Same comments on this test as well.

Applied for the second test as well.

I'm uploading a new patch in a minute.

[Mikhail Naganov]

Created attachment 102114 [details]
comments addressed

[Alexey Proskuryakov]

> I wrote a test for horizontal scrolling, but have found out that the control is being centered properly without my patch as well, so it makes no sense adding it.

This does not match my testing in Mac Safari. Try the following steps:

1. Type a long string into Keywords field in Bugzilla, so that it doesn't fit. Caret stays at the right all the time, which is correct behavior.
2. Press Cmd+Left arrow to move caret to start.
3. Type a long string again.

Expected results: when caret reaches the right edge, the input control is scrolled moving it to the center.
Actual results: it stays at the edge.

[Mikhail Naganov]

(In reply to comment #10)
> > I wrote a test for horizontal scrolling, but have found out that the control is being centered properly without my patch as well, so it makes no sense adding it.
> 
> This does not match my testing in Mac Safari. Try the following steps:
> 
> 1. Type a long string into Keywords field in Bugzilla, so that it doesn't fit. Caret stays at the right all the time, which is correct behavior.
> 2. Press Cmd+Left arrow to move caret to start.
> 3. Type a long string again.
> 
> Expected results: when caret reaches the right edge, the input control is scrolled moving it to the center.
> Actual results: it stays at the edge.

But the bug isn't about cursor positioning behaviour. It's about positioning of a edit control.

[Alexey Proskuryakov]

Indeed, I overlooked that the original report is about revealing the control, not selection in a control.

You've found that the logic was added in bug 7113 to support proper caret/selection revealing. Did the code paths get decoupled since then, or does your patch inadvertently change both?

[Mikhail Naganov]

(In reply to comment #12)
> Indeed, I overlooked that the original report is about revealing the control, not selection in a control.
> 
> You've found that the logic was added in bug 7113 to support proper caret/selection revealing. Did the code paths get decoupled since then, or does your patch inadvertently change both?

I wouldn't call it "proper". It's what you are observing now and has described as the "Actual" behaviour. With my change in place, you will observe the "Expected" behaviour. So, yes, my change inadvertently changes cursor positioning.

I'm not sure how to handle it better. Splitting code paths and then fixing both separately seems like a stupid idea. We can fire a bug describing cursor behavour, and mark it as related to this one. I don't think that adding cursor positioning tests to this very issue is logical.

[Mikhail Naganov]

Created attachment 102339 [details]
Added layout tests for caret behaviour at the end of input

Tests for caret have been added as pixel tests because DRT doesn't dump pixel position of the caret, only character position.

[Mikhail Naganov]

Created attachment 102340 [details]
now with images

[WebKit Review Bot]

Comment on attachment 102340 [details]
now with images

Attachment 102340 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9269366

New failing tests:
editing/input/caret-at-the-edge-of-input.html
editing/input/caret-at-the-edge-of-contenteditable.html

[Mikhail Naganov]

(In reply to comment #12)
> Indeed, I overlooked that the original report is about revealing the control, not selection in a control.
> 
> You've found that the logic was added in bug 7113 to support proper caret/selection revealing. Did the code paths get decoupled since then, or does your patch inadvertently change both?

Alexei, please look at the bug when you'll have a chance.

[Ryosuke Niwa]

Comment on attachment 102340 [details]
now with images

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.

[Alexey Proskuryakov]

I have only one comment: any behaviors that we now know this patch changes should be tested for in this patch. As far as I can tell, this patch changes behavior to be more like AppKit, which is a good thing.

[Mikhail Naganov]

(In reply to comment #18)
> (From update of attachment 102340 [details])
> 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
>

Done.
 
> > 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.
>

True. Fixed.
 
> > 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();
>

Done.
 
> > LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:3
> > +<script>
> 
> You can put this script inside the script element where you call runTest.
>

Moved.
 
> > 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.
> 

I've figured out how to make it work -- one needs also to specify size for html and body, now it work. 

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

Done.

> > LayoutTests/editing/input/reveal-edit-on-input-vertically.html:3
> > +<script>
> 
> Ditto about merging this script element with one below.

Done.

[Mikhail Naganov]

(In reply to comment #19)
> I have only one comment: any behaviors that we now know this patch changes should be tested for in this patch. As far as I can tell, this patch changes behavior to be more like AppKit, which is a good thing.

I've looked at code paths, and found that codepath for the paste operation is different. Also added test for this.

[Mikhail Naganov]

Created attachment 103491 [details]
Patch

[WebKit Review Bot]

Comment on attachment 103491 [details]
Patch

Attachment 103491 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9336680

New failing tests:
editing/input/caret-at-the-edge-of-input.html
editing/input/caret-at-the-edge-of-contenteditable.html

[Alexey Proskuryakov]

I still don't understand why this patch doesn't have a test case for vertical scrolling.

[Mikhail Naganov]

These tests verify that control is centered when we have vertical scrolling:

editing/input/reveal-contenteditable-on-input-vertically.html
editing/input/reveal-contenteditable-on-paste-vertically.html
editing/input/reveal-edit-on-input-vertically.html
editing/input/reveal-edit-on-paste-vertically.html

As for horizontal scrolling, centering of input controls themselves was working prior to patch. Only caret centering has changed, and this is covered by tests.

[Alexey Proskuryakov]

Yes, I understand the difference between scrolling a control into view and positioning insertion point inside a control.

My question still stands - what tests verify scrolling a text area (or an overflow:scroll div) to reveal a caret vertically? E.g. if you put a lot of text into this "Additional Comments" field in Bugzilla, scroll insertion point out of view, and press a key.

[Mikhail Naganov]

(In reply to comment #26)
> Yes, I understand the difference between scrolling a control into view and positioning insertion point inside a control.
> 
> My question still stands - what tests verify scrolling a text area (or an overflow:scroll div) to reveal a caret vertically? E.g. if you put a lot of text into this "Additional Comments" field in Bugzilla, scroll insertion point out of view, and press a key.

Oh, my bad. Yes, my patch changes this behaviour as well. I will add tests.

[Mikhail Naganov]

Created attachment 103684 [details]
Added tests for caret revealing in multiline edit

Behaviour of caret in multiline input and contenteditable now match caret behaviour in TextEdit:
 - when caret is scrolled out of visible area and you start typing, the caret is brought to the center of the visible area -- this is achieved with the patch, tests added;
 - when you have just reached the bottom of the input, and continue typing, caret will stay at the bottom -- this behaviour has been left intact.

[WebKit Review Bot]

Comment on attachment 103684 [details]
Added tests for caret revealing in multiline edit

Attachment 103684 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9358368

New failing tests:
editing/input/reveal-caret-of-multiline-contenteditable.html
editing/input/reveal-caret-of-multiline-input.html
editing/input/caret-at-the-edge-of-input.html
editing/input/caret-at-the-edge-of-contenteditable.html

[Ryosuke Niwa]

Comment on attachment 103684 [details]
Added tests for caret revealing in multiline edit

View in context: https://bugs.webkit.org/attachment.cgi?id=103684&action=review

> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:20
> +    if (window.layoutTestController)
> +        layoutTestController.dumpAsText();

You shouldn't indent scripts like this.  Please outdent.

> LayoutTests/editing/input/reveal-contenteditable-on-paste-vertically.html:40
> +    textInputController.insertText("WebKit");

I'd prefer using document.execCommand('insertText') instead.

> LayoutTests/editing/input/reveal-edit-on-input-vertically.html:30
> +function offsetFromViewportTop(element)
> +{
> +    return element.getClientRects()[0].top;
> +}
> + 
> +function offsetOfMiddleFromViewportTop(element)
> +{
> +    return element.getClientRects()[0].top + Math.round(element.getClientRects()[0].height / 2);
> +}

Please share code between tests by putting some of these code in a shared file in resources directory.

[Mikhail Naganov]

Created attachment 104323 [details]
Patch

(In reply to comment #30)
> (From update of attachment 103684 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103684&action=review
> 
> > LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:20
> > +    if (window.layoutTestController)
> > +        layoutTestController.dumpAsText();
> 
> You shouldn't indent scripts like this.  Please outdent.
>

Fixed.
 
> > LayoutTests/editing/input/reveal-contenteditable-on-paste-vertically.html:40
> > +    textInputController.insertText("WebKit");
> 
> I'd prefer using document.execCommand('insertText') instead.
>

This way, it doesn't work. Perhaps I need to pass control to the renderer after executing the command via setTimeout(0)? Using textInputController works and looks simpler.
 
> > LayoutTests/editing/input/reveal-edit-on-input-vertically.html:30
> > +function offsetFromViewportTop(element)
> > +{
> > +    return element.getClientRects()[0].top;
> > +}
> > + 
> > +function offsetOfMiddleFromViewportTop(element)
> > +{
> > +    return element.getClientRects()[0].top + Math.round(element.getClientRects()[0].height / 2);
> > +}
> 
> Please share code between tests by putting some of these code in a shared file in resources directory.

I've factored out these functions. I also evaluated, if it makes sense to extract control flow common for several scripts, but the result turned out to be more cumbersome, as test-specific actions need access to variables that will be extracted as a part of common flow.

[Mikhail Naganov]

Created attachment 104326 [details]
Patch (this time from the right repository)

[WebKit Review Bot]

Comment on attachment 104326 [details]
Patch (this time from the right repository)

Attachment 104326 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9425409

New failing tests:
editing/input/reveal-caret-of-multiline-contenteditable.html
editing/input/reveal-caret-of-multiline-input.html
editing/input/caret-at-the-edge-of-input.html
editing/input/caret-at-the-edge-of-contenteditable.html

[Alexey Proskuryakov]

It's a mystery why document.execCommand didn't work for you. The problem with textInputController is that it is not supported in DRT on many platforms.

[Mikhail Naganov]

(In reply to comment #34)
> It's a mystery why document.execCommand didn't work for you. The problem with textInputController is that it is not supported in DRT on many platforms.

document.execCommand('insertText'...) doesn't scroll the edit control into view. It just inserts text. I've verified it interactively with Safari. While textInputContoller and eventSender seem to emulate keyboard input.

[Alexey Proskuryakov]

Thanks, that makes sense.

In fact, TextInputController just implicitly passes an option to Document::execCommand() pretending that the command comes from a keyboard shortcut or menu. That's a bit subtle, possibly unintended, and might change in the future.

As the behavior can indeed be different between programmatic and manual text insertion, I suggest using eventSender to type a character instead of any execCommand variant.

[Mikhail Naganov]

(In reply to comment #36)
> Thanks, that makes sense.
> 
> In fact, TextInputController just implicitly passes an option to Document::execCommand() pretending that the command comes from a keyboard shortcut or menu. That's a bit subtle, possibly unintended, and might change in the future.
> 
> As the behavior can indeed be different between programmatic and manual text insertion, I suggest using eventSender to type a character instead of any execCommand variant.

But this wouldn't be pasting in this case. The test is called 'reveal-contenteditable-on-paste-vertically' and simulates pasting behaviour. I already have tests for keypresses emulated using eventSender.

Is there a way to easily emulate paste using eventSender? I can imagine sending keyboard events like "Ctrl-V" / "Cmd-V" but they are platform dependent, right?

[Darin Adler]

(In reply to comment #37)
> Is there a way to easily emulate paste using eventSender? I can imagine sending keyboard events like "Ctrl-V" / "Cmd-V" but they are platform dependent, right?

Not using eventSender, but I believe layoutTestController.execCommand("Paste") works.

[Mikhail Naganov]

Created attachment 104402 [details]
Got rid of textInputController

Thanks, Alexei! Using "paste" commands works.

[WebKit Review Bot]

Comment on attachment 104402 [details]
Got rid of textInputController

Attachment 104402 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9419924

New failing tests:
editing/input/reveal-caret-of-multiline-contenteditable.html
editing/input/reveal-caret-of-multiline-input.html
editing/input/caret-at-the-edge-of-input.html
editing/input/caret-at-the-edge-of-contenteditable.html

[Mikhail Naganov]

(In reply to comment #36)
> Thanks, that makes sense.
> 
> In fact, TextInputController just implicitly passes an option to Document::execCommand() pretending that the command comes from a keyboard shortcut or menu. That's a bit subtle, possibly unintended, and might change in the future.
> 
> As the behavior can indeed be different between programmatic and manual text insertion, I suggest using eventSender to type a character instead of any execCommand variant.

Hi Alexey,

From our recent IRC conversation, I've concluded that you are OK with the set of tests I provided but still hesitate about the change itself. Is that correct?

In this case, what are your arguments against this change? With it, edit controls caret behaviour is now consistent between e.g. Safari's address and search inputs and inputs on web pages. Which is good.

Perhaps, we can ask for opinions from more people? I'm just unhappy with this change being stuck.

[Alexey Proskuryakov]

I don't have anything against this change in behavior. I'm just not feeling enough enthusiasm to r+ it myself, but if someone else does, please go ahead!

[Mikhail Naganov]

Darin, I see that most of reviews for changes into this file are done by you. What do you think about this change?

[Ryosuke Niwa]

Comment on attachment 104402 [details]
Got rid of textInputController

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"

[Mikhail Naganov]

Comment on attachment 104402 [details]
Got rid of textInputController

View in context: https://bugs.webkit.org/attachment.cgi?id=104402&action=review

>> 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.

Fixed.

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

Fixed.

>> LayoutTests/editing/input/reveal-caret-of-multiline-contenteditable.html:38
>> +30<br>
> 
> Can we auto generate this by script?

Done.

>> LayoutTests/editing/input/reveal-caret-of-multiline-input.html:37
>> +30
> 
> Ditto about generating this by script.

Done.

>> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:26
>> +var initialOffset = offsetFromViewportTop(topmostElement);
> 
> All these variables are making this test less readable.

Removed.

>> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:30
>> +        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.

Done. Thanks, great idea!

>> LayoutTests/editing/input/reveal-contenteditable-on-input-vertically.html:40
>> +    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"

Done.

[Mikhail Naganov]

Created attachment 107735 [details]
comments addressed

[WebKit Review Bot]

Comment on attachment 107735 [details]
comments addressed

Attachment 107735 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9735176

New failing tests:
editing/input/reveal-caret-of-multiline-contenteditable.html
editing/input/reveal-caret-of-multiline-input.html
editing/input/caret-at-the-edge-of-input.html
editing/input/caret-at-the-edge-of-contenteditable.html

[Mikhail Naganov]

Manually landed http://trac.webkit.org/changeset/103073


Scroll non-visible edit controls and caret into the center of the view when starting typing.
https://bugs.webkit.org/show_bug.cgi?id=65027

Reviewed by Ryosuke Niwa.

Tests: editing/input/caret-at-the-edge-of-contenteditable.html
       editing/input/caret-at-the-edge-of-input.html
       editing/input/reveal-caret-of-multiline-contenteditable.html
       editing/input/reveal-caret-of-multiline-input.html
       editing/input/reveal-contenteditable-on-input-vertically.html
       editing/input/reveal-contenteditable-on-paste-vertically.html
       editing/input/reveal-edit-on-input-vertically.html
       editing/input/reveal-edit-on-paste-vertically.html

* editing/Editor.cpp:
(WebCore::Editor::insertTextWithoutSendingTextEvent):
(WebCore::Editor::revealSelectionAfterEditingOperation):

* editing/input/caret-at-the-edge-of-contenteditable-expected.png: Added.
* editing/input/caret-at-the-edge-of-contenteditable-expected.txt: Added.
* editing/input/caret-at-the-edge-of-contenteditable.html: Added.
* editing/input/caret-at-the-edge-of-input-expected.png: Added.
* editing/input/caret-at-the-edge-of-input-expected.txt: Added.
* editing/input/caret-at-the-edge-of-input.html: Added.
* editing/input/resources/reveal-utilities.js: Added.
* editing/input/reveal-caret-of-multiline-contenteditable-expected.png: Added.
* editing/input/reveal-caret-of-multiline-contenteditable-expected.txt: Added.
* editing/input/reveal-caret-of-multiline-contenteditable.html: Added.
* editing/input/reveal-caret-of-multiline-input-expected.png: Added.
* editing/input/reveal-caret-of-multiline-input-expected.txt: Added.
* editing/input/reveal-caret-of-multiline-input.html: Added.
* editing/input/reveal-contenteditable-on-input-vertically-expected.txt: Added.
* editing/input/reveal-contenteditable-on-input-vertically.html: Added.
* editing/input/reveal-contenteditable-on-paste-vertically-expected.txt: Added.
* editing/input/reveal-contenteditable-on-paste-vertically.html: Added.
* editing/input/reveal-edit-on-input-vertically-expected.txt: Added.
* editing/input/reveal-edit-on-input-vertically.html: Added.
* editing/input/reveal-edit-on-paste-vertically-expected.txt: Added.
* editing/input/reveal-edit-on-paste-vertically.html: Added.
* platform/chromium/test_expectations.txt: Mark new tests as FAIL to grab results from bots.
* platform/gtk/test_expectations.txt: Mark new tests as FAIL to grab results from bots.
* platform/qt/test_expectations.txt: Mark new tests as FAIL to grab results from bots.
* platform/win/test_expectations.txt: Mark new tests as FAIL to grab results from bots.

Then, updated test expectations:

http://trac.webkit.org/changeset/103081


Rebaseline and add expectations after r103073
[chromium] https://bugs.webkit.org/show_bug.cgi?id=74726

* platform/chromium-linux/editing/input/caret-at-the-edge-of-contenteditable-expected.png: Added.
* platform/chromium-linux/editing/input/caret-at-the-edge-of-input-expected.png: Added.
* platform/chromium-linux/editing/input/reveal-caret-of-multiline-contenteditable-expected.png: Added.
* platform/chromium-linux/editing/input/reveal-caret-of-multiline-input-expected.png: Added.
* platform/chromium-mac-leopard/editing/input/caret-at-the-edge-of-contenteditable-expected.png: Added.
* platform/chromium-mac-leopard/editing/input/caret-at-the-edge-of-input-expected.png: Added.
* platform/chromium-mac-leopard/editing/input/reveal-caret-of-multiline-contenteditable-expected.png: Added.
* platform/chromium-mac-leopard/editing/input/reveal-caret-of-multiline-input-expected.png: Added.
* platform/chromium-mac-leopard/fast/forms/input-text-scroll-left-on-blur-expected.png:
* platform/chromium-mac-snowleopard/editing/input/caret-at-the-edge-of-contenteditable-expected.png: Added.
* platform/chromium-mac-snowleopard/editing/input/caret-at-the-edge-of-input-expected.png: Added.
* platform/chromium-mac-snowleopard/editing/input/reveal-caret-of-multiline-contenteditable-expected.png: Added.
* platform/chromium-mac-snowleopard/editing/input/reveal-caret-of-multiline-input-expected.png: Added.
* platform/chromium-mac-snowleopard/fast/forms/input-text-scroll-left-on-blur-expected.png: Added.
* platform/chromium-mac/fast/forms/input-text-scroll-left-on-blur-expected.png: Removed.
* platform/chromium-win/editing/input/caret-at-the-edge-of-contenteditable-expected.png: Added.
* platform/chromium-win/editing/input/caret-at-the-edge-of-contenteditable-expected.txt: Added.
* platform/chromium-win/editing/input/caret-at-the-edge-of-input-expected.png: Added.
* platform/chromium-win/editing/input/caret-at-the-edge-of-input-expected.txt: Added.
* platform/chromium-win/editing/input/reveal-caret-of-multiline-contenteditable-expected.png: Added.
* platform/chromium-win/editing/input/reveal-caret-of-multiline-contenteditable-expected.txt: Added.
* platform/chromium-win/editing/input/reveal-caret-of-multiline-input-expected.png: Added.
* platform/chromium-win/editing/input/reveal-caret-of-multiline-input-expected.txt: Added.
* platform/chromium-win/fast/forms/input-text-scroll-left-on-blur-expected.png:
* platform/chromium-win/fast/forms/input-text-scroll-left-on-blur-expected.txt:
* platform/chromium/test_expectations.txt:
* platform/gtk/test_expectations.txt:
* platform/mac/fast/forms/input-text-scroll-left-on-blur-expected.txt:
* platform/qt/test_expectations.txt:
* platform/win/test_expectations.txt:

and http://trac.webkit.org/changeset/103085:


[Gtk] Add platform-specific test results after r103073

* platform/gtk/editing/input/caret-at-the-edge-of-contenteditable-expected.txt: Added.
* platform/gtk/editing/input/caret-at-the-edge-of-input-expected.txt: Added.
* platform/gtk/editing/input/reveal-caret-of-multiline-contenteditable-expected.txt: Added.
* platform/gtk/editing/input/reveal-caret-of-multiline-input-expected.txt: Added.