Bug 65027 - Inserting text into scrolled out text controls should bring them into the center of the view, not on an edge
Summary: Inserting text into scrolled out text controls should bring them into the cen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mikhail Naganov
URL: http://m.cnn.com
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-22 06:55 PDT by Mikhail Naganov
Modified: 2011-12-16 11:20 PST (History)
8 users (show)

See Also:


Attachments
Reference - mobile Safari (101.21 KB, image/png)
2011-07-22 06:55 PDT, Mikhail Naganov
no flags Details
Patch (1.33 KB, patch)
2011-07-22 06:59 PDT, Mikhail Naganov
ap: review-
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Desktop version - with patch (33.28 KB, image/png)
2011-07-22 06:59 PDT, Mikhail Naganov
no flags Details
patch with tests (7.12 KB, patch)
2011-07-26 08:19 PDT, Mikhail Naganov
rniwa: review-
mnaganov: commit-queue-
Details | Formatted Diff | Diff
comments addressed (7.70 KB, patch)
2011-07-27 02:54 PDT, Mikhail Naganov
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Added layout tests for caret behaviour at the end of input (13.74 KB, patch)
2011-07-29 02:18 PDT, Mikhail Naganov
mnaganov: commit-queue-
Details | Formatted Diff | Diff
now with images (39.17 KB, patch)
2011-07-29 02:19 PDT, Mikhail Naganov
rniwa: review-
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Patch (45.73 KB, patch)
2011-08-10 07:52 PDT, Mikhail Naganov
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Added tests for caret revealing in multiline edit (97.27 KB, patch)
2011-08-11 15:26 PDT, Mikhail Naganov
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Patch (45.37 KB, patch)
2011-08-18 04:43 PDT, Mikhail Naganov
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Patch (this time from the right repository) (96.94 KB, patch)
2011-08-18 05:04 PDT, Mikhail Naganov
mnaganov: commit-queue-
Details | Formatted Diff | Diff
Got rid of textInputController (97.25 KB, patch)
2011-08-18 14:57 PDT, Mikhail Naganov
rniwa: review-
mnaganov: commit-queue-
Details | Formatted Diff | Diff
comments addressed (95.21 KB, patch)
2011-09-16 15:40 PDT, Mikhail Naganov
rniwa: review+
mnaganov: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mikhail Naganov 2011-07-22 06:55:31 PDT
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.
Comment 1 Mikhail Naganov 2011-07-22 06:59:02 PDT
Created attachment 101727 [details]
Patch
Comment 2 Mikhail Naganov 2011-07-22 06:59:40 PDT
Created attachment 101728 [details]
Desktop version - with patch
Comment 3 Alexey Proskuryakov 2011-07-22 10:06:05 PDT
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.
Comment 4 Mikhail Naganov 2011-07-22 12:56:44 PDT
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?
Comment 5 Alexey Proskuryakov 2011-07-22 13:43:50 PDT
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.
Comment 6 Mikhail Naganov 2011-07-26 08:19:50 PDT
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.
Comment 7 Ryosuke Niwa 2011-07-26 12:22:56 PDT
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.
Comment 8 Mikhail Naganov 2011-07-27 02:52:57 PDT
(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.
Comment 9 Mikhail Naganov 2011-07-27 02:54:38 PDT
Created attachment 102114 [details]
comments addressed
Comment 10 Alexey Proskuryakov 2011-07-27 08:37:01 PDT
> 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.
Comment 11 Mikhail Naganov 2011-07-27 08:40:49 PDT
(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.
Comment 12 Alexey Proskuryakov 2011-07-27 08:55:23 PDT
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?
Comment 13 Mikhail Naganov 2011-07-27 10:03:31 PDT
(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.
Comment 14 Mikhail Naganov 2011-07-29 02:18:00 PDT
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.
Comment 15 Mikhail Naganov 2011-07-29 02:19:21 PDT
Created attachment 102340 [details]
now with images
Comment 16 WebKit Review Bot 2011-07-29 02:45:32 PDT
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
Comment 17 Mikhail Naganov 2011-08-02 04:28:36 PDT
(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.
Comment 18 Ryosuke Niwa 2011-08-08 12:36:55 PDT
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.
Comment 19 Alexey Proskuryakov 2011-08-08 13:09:21 PDT
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.
Comment 20 Mikhail Naganov 2011-08-10 07:49:17 PDT
(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.
Comment 21 Mikhail Naganov 2011-08-10 07:50:23 PDT
(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.
Comment 22 Mikhail Naganov 2011-08-10 07:52:42 PDT
Created attachment 103491 [details]
Patch
Comment 23 WebKit Review Bot 2011-08-10 08:29:31 PDT
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
Comment 24 Alexey Proskuryakov 2011-08-10 09:30:41 PDT
I still don't understand why this patch doesn't have a test case for vertical scrolling.
Comment 25 Mikhail Naganov 2011-08-10 09:37:04 PDT
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.
Comment 26 Alexey Proskuryakov 2011-08-10 10:03:11 PDT
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.
Comment 27 Mikhail Naganov 2011-08-10 11:07:46 PDT
(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.
Comment 28 Mikhail Naganov 2011-08-11 15:26:29 PDT
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.
Comment 29 WebKit Review Bot 2011-08-11 15:49:50 PDT
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
Comment 30 Ryosuke Niwa 2011-08-17 20:29:06 PDT
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.
Comment 31 Mikhail Naganov 2011-08-18 04:43:28 PDT
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.
Comment 32 Mikhail Naganov 2011-08-18 05:04:06 PDT
Created attachment 104326 [details]
Patch (this time from the right repository)
Comment 33 WebKit Review Bot 2011-08-18 07:37:24 PDT
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
Comment 34 Alexey Proskuryakov 2011-08-18 08:59:55 PDT
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.
Comment 35 Mikhail Naganov 2011-08-18 09:56:56 PDT
(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.
Comment 36 Alexey Proskuryakov 2011-08-18 10:33:31 PDT
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.
Comment 37 Mikhail Naganov 2011-08-18 10:43:25 PDT
(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?
Comment 38 Darin Adler 2011-08-18 10:53:37 PDT
(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.
Comment 39 Mikhail Naganov 2011-08-18 14:57:39 PDT
Created attachment 104402 [details]
Got rid of textInputController

Thanks, Alexei! Using "paste" commands works.
Comment 40 WebKit Review Bot 2011-08-18 15:22:33 PDT
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
Comment 41 Mikhail Naganov 2011-08-25 04:37:44 PDT
(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.
Comment 42 Alexey Proskuryakov 2011-08-25 08:59:53 PDT
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!
Comment 43 Mikhail Naganov 2011-08-30 14:39:04 PDT
Darin, I see that most of reviews for changes into this file are done by you. What do you think about this change?
Comment 44 Ryosuke Niwa 2011-09-14 20:33:44 PDT
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"
Comment 45 Mikhail Naganov 2011-09-16 15:38:14 PDT
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.
Comment 46 Mikhail Naganov 2011-09-16 15:40:00 PDT
Created attachment 107735 [details]
comments addressed
Comment 47 WebKit Review Bot 2011-09-17 02:50:12 PDT
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
Comment 48 Mikhail Naganov 2011-12-16 11:20:46 PST
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.