Made text inside <input> elements scrollable using touch gestures

Made text inside <input> elements scrollable using touch gestures

BUG=233183
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156162

[tdanderson, 2013-08-08 20:01:16]

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
File
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field-expected.txt
(right):

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field-expected.txt:2:
This tests that an input text field can be properly scrolled with touch gestures
Can you do a quick sanity check that everything works as intended if the text
field has the attributes disabled or readonly? (Just try this informally, no
need to include it in the layout test.)

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field-expected.txt:7:
Flinging input text should scroll the text
On lines 7 and 16 make it clear that you're not flinging/scrolling the text
fully.

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field-expected.txt:11:
PASS box.scrollLeft is box.scrollWidth - box.clientWidth
In the expected results it is better report the actual value of expressions,
e.g.:

PASS box.scrollLeft is 462

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field-expected.txt:16:
Gesture scrolling input text a bit should scroll the text
I would add a sanity check between the end of the fling stuff and the start of
the non-fling stuff to verify that everything has been reset properly. It isn't
clear that you have reset anything just by reading what you have in this file.

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
File LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field.html
(right):

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field.html:13:
<input id="box" style="height:100px; width:400px; font-size:xx-large"
type="text"
value="asasd;flkajsd;flkasjdf;alksdjf;alskdfja;lksdja;sdlfjkas;ldkf"></input>
Unless it would be too much of a pain, maybe specify the <input> attribute
size=n, where n is the number of characters (and then make the font the default
size)? That way someone reading the html can be confident that there is/is not a
horizontal overflow. Maybe this change isn't too worthwhile, though... just a
thought.

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field.html:18:
<p id="description"></p>
Also including something in this/another test to verify that everything works as
intended when the user tries to scroll a text field for which the text does not
overflow.

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field.html:46:
setTimeout('checkFlingOffset();', 100);
Your actual reviewer probably won't like these setTimeout()s. Are they
necessary?

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field.html:143:
testRunner.notifyDone();
Once you're done testing horizontal fling and scroll, I would reset the state
again and check that everything works as intended with vertical fling/scroll.

https://codereview.chromium.org/22488004/diff/4001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/22488004/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.cpp:1325: if (box->isTextField())
I would re-write as:

if (box->isTextField())
    return orientation == HorizontalScrollbar;

since vertical scrolling of text fields is not possible.

Also (as you mentioned to me in person) a follow-up patch to rename
ScrollbarOrientation to ScrollOrientation may be in order, since elements such
as text fields or divs may in fact be scrollable in a certain direction but not
have any scrollbars.

https://codereview.chromium.org/22488004/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.cpp:1956: // Scroll textfields on the main
thread so we get appropriate touch gesture propagation
Add a period at the end of the comment.

https://codereview.chromium.org/22488004/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.cpp:1957: if (renderBox() &&
renderBox()->isTextField())
Are you sure that usesCompositedScrolling() is called every time there is a
user-generated scroll event (fling or otherwise) originating in a text field?

https://codereview.chromium.org/22488004/diff/4001/Source/core/rendering/Rend...
File Source/core/rendering/RenderTextControlSingleLine.h (right):

https://codereview.chromium.org/22488004/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderTextControlSingleLine.h:118: virtual bool
scrollsOverflowY() const OVERRIDE { return hasOverflowClip(); }
Shouldn't scrollsOverflowY() return false here since vertical scrolling in a
text field is impossible? If I have a vertical fling originating on a text field
that has horizontal overflow, I wouldn't want the text field to even be
considered as a possible target.

[bokan, 2013-08-12 16:19:28]

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
File
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field-expected.txt
(right):

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field-expected.txt:7:
Flinging input text should scroll the text
On 2013/08/08 20:01:16, tdanderson wrote:
> On lines 7 and 16 make it clear that you're not flinging/scrolling the text
> fully.
Done

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field-expected.txt:11:
PASS box.scrollLeft is box.scrollWidth - box.clientWidth
On 2013/08/08 20:01:16, tdanderson wrote:
> In the expected results it is better report the actual value of expressions,
> e.g.:
> 
> PASS box.scrollLeft is 462
Done

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field-expected.txt:16:
Gesture scrolling input text a bit should scroll the text
On 2013/08/08 20:01:16, tdanderson wrote:
> I would add a sanity check between the end of the fling stuff and the start of
> the non-fling stuff to verify that everything has been reset properly. It
isn't
> clear that you have reset anything just by reading what you have in this file.

Done.

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
File LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field.html
(right):

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field.html:13:
<input id="box" style="height:100px; width:400px; font-size:xx-large"
type="text"
value="asasd;flkajsd;flkasjdf;alksdjf;alskdfja;lksdja;sdlfjkas;ldkf"></input>
On 2013/08/08 20:01:16, tdanderson wrote:
> Unless it would be too much of a pain, maybe specify the <input> attribute
> size=n, where n is the number of characters (and then make the font the
default
> size)? That way someone reading the html can be confident that there is/is not
a
> horizontal overflow. Maybe this change isn't too worthwhile, though... just a
> thought.
I made the text large so it can be easily hit with a touch screen and so it's
easy to target in the test, but I've changed the input box to use size=10.

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field.html:18:
<p id="description"></p>
On 2013/08/08 20:01:16, tdanderson wrote:
> Also including something in this/another test to verify that everything works
as
> intended when the user tries to scroll a text field for which the text does
not
> overflow.

Done.

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field.html:46:
setTimeout('checkFlingOffset();', 100);
On 2013/08/08 20:01:16, tdanderson wrote:
> Your actual reviewer probably won't like these setTimeout()s. Are they
> necessary?
I thought event sender sent events asynchronously but it looks like I was wrong,
I've rearranged everything without these kinds of calls.

https://codereview.chromium.org/22488004/diff/4001/LayoutTests/fast/events/to...
LayoutTests/fast/events/touch/gesture/touch-gesture-scroll-input-field.html:143:
testRunner.notifyDone();
On 2013/08/08 20:01:16, tdanderson wrote:
> Once you're done testing horizontal fling and scroll, I would reset the state
> again and check that everything works as intended with vertical fling/scroll.

Done.

https://codereview.chromium.org/22488004/diff/4001/Source/core/rendering/Rend...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/22488004/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.cpp:1325: if (box->isTextField())
On 2013/08/08 20:01:16, tdanderson wrote:
> I would re-write as:
> 
> if (box->isTextField())
>     return orientation == HorizontalScrollbar;
> 
> since vertical scrolling of text fields is not possible.
> 
> Also (as you mentioned to me in person) a follow-up patch to rename
> ScrollbarOrientation to ScrollOrientation may be in order, since elements such
> as text fields or divs may in fact be scrollable in a certain direction but
not
> have any scrollbars.

Done.

https://codereview.chromium.org/22488004/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.cpp:1956: // Scroll textfields on the main
thread so we get appropriate touch gesture propagation
On 2013/08/08 20:01:16, tdanderson wrote:
> Add a period at the end of the comment.

Done.

https://codereview.chromium.org/22488004/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderLayer.cpp:1957: if (renderBox() &&
renderBox()->isTextField())
On 2013/08/08 20:01:16, tdanderson wrote:
> Are you sure that usesCompositedScrolling() is called every time there is a
> user-generated scroll event (fling or otherwise) originating in a text field?
It's called when a frame has its layout updated to compute which portions of the
page should be scrolled on the main thread/impl thread.

https://codereview.chromium.org/22488004/diff/4001/Source/core/rendering/Rend...
File Source/core/rendering/RenderTextControlSingleLine.h (right):

https://codereview.chromium.org/22488004/diff/4001/Source/core/rendering/Rend...
Source/core/rendering/RenderTextControlSingleLine.h:118: virtual bool
scrollsOverflowY() const OVERRIDE { return hasOverflowClip(); }
On 2013/08/08 20:01:16, tdanderson wrote:
> Shouldn't scrollsOverflowY() return false here since vertical scrolling in a
> text field is impossible? If I have a vertical fling originating on a text
field
> that has horizontal overflow, I wouldn't want the text field to even be
> considered as a possible target.

Done.

[tdanderson, 2013-08-12 18:53:49]

Thanks for making those changes. Patch set 3 LGTM.

[esprehn, 2013-08-12 23:38:06]

https://codereview.chromium.org/22488004/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/22488004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1326: return orientation ==
HorizontalScrollbar;
This doesn't seem writing mode aware.

https://codereview.chromium.org/22488004/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTextControlSingleLine.h (right):

https://codereview.chromium.org/22488004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderTextControlSingleLine.h:116: virtual bool
isTextField() const { return true; }
You can't override this, we assume that if isTextField is true that
toRenderTextControlSingleLine is safe, but that's not true now.

https://codereview.chromium.org/22488004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderTextControlSingleLine.h:118: virtual bool
scrollsOverflowY() const OVERRIDE { return false; }
missing OVERRIDE on all of these

[esprehn, 2013-08-12 23:38:19]

not lgtm, this introduces security bugs.

[bokan, 2013-08-13 01:13:47]

https://codereview.chromium.org/22488004/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/22488004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1326: return orientation ==
HorizontalScrollbar;
On 2013/08/12 23:38:06, esprehn wrote:
> This doesn't seem writing mode aware.
I've changed this to defer the decision to the control itself so the issue is
now in RenderTextControlSingleLine, but I couldn't force an <input> box to use a
vertical writing mode, does Chrome support vertical input boxes?

https://codereview.chromium.org/22488004/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTextControlSingleLine.h (right):

https://codereview.chromium.org/22488004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderTextControlSingleLine.h:116: virtual bool
isTextField() const { return true; }
On 2013/08/12 23:38:06, esprehn wrote:
> You can't override this, we assume that if isTextField is true that
> toRenderTextControlSingleLine is safe, but that's not true now.
Replaced with a new virtual RenderBox::isIntristicallyScrollable that scrollable
controls can use. This actually seems cleaner.

https://codereview.chromium.org/22488004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderTextControlSingleLine.h:118: virtual bool
scrollsOverflowY() const OVERRIDE { return false; }
On 2013/08/12 23:38:06, esprehn wrote:
> missing OVERRIDE on all of these

Done.

[esprehn, 2013-08-15 17:49:27]

LGTM

[commit-bot: I haz the power, 2013-08-15 17:50:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/22488004/45001

[commit-bot: I haz the power, 2013-08-15 19:19:40]

Change committed as 156162