Should be able to scroll text in <input> fields with touch

Should be able to scroll text in <input> fields with touch

BUG=233183

[Rick Byers, 2013-05-30 16:15:00]

This feels really nice in practice, thanks David!  Some quick suggestions before
we ask jamesr or another reviewer to take a look.

https://codereview.chromium.org/15725009/diff/1/Source/core/platform/Scrollab...
File Source/core/platform/ScrollableArea.cpp (right):

https://codereview.chromium.org/15725009/diff/1/Source/core/platform/Scrollab...
Source/core/platform/ScrollableArea.cpp:85: bool
ScrollableArea::touchScroll(ScrollDirection direction, ScrollGranularity
granularity, float multiplier)
Adding a new method for this feels wrong to me (as does the duplication of code
here).

Can't we just handle this case inside of 'scroll'?  Eg. maybe
horizontalScrollbar returns null or a dummy value, then we fall back to
scrollbar-less scrolling behavior.

https://codereview.chromium.org/15725009/diff/1/Source/core/platform/Scrollab...
Source/core/platform/ScrollableArea.cpp:85: bool
ScrollableArea::touchScroll(ScrollDirection direction, ScrollGranularity
granularity, float multiplier)
This is called 'touchScroll' but there's nothing in the code that limits it to
touch.  After playing with it, I actually kind of like how this behaves with the
trackpad and horiz mousewheel - seems like a nice feature there too, but not as
essential as the touchscreen.

We should probably check with jamesr to see what he thinks.  If letting trackpad
scroll here too is what we want then we need tests for the other input cases as
well.

https://codereview.chromium.org/15725009/diff/1/Source/core/platform/Scrollab...
Source/core/platform/ScrollableArea.cpp:97: return
scrollAnimator()->scroll(orientation, granularity, 1.0f, multiplier);
Is treating all values of 'granularity' really ok?  How might a value other than
ScrollByPixel/ScrollByPrecisePixel get here?

https://codereview.chromium.org/15725009/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/15725009/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.h:586: bool isScrollableWithoutScrollbars()
const
It seems a little confusing to me to add a case where scrolling is possible but
scrollsOverflowX returns false.

Why not just add this case to a condition of scrollsOverflowX (rather than
introduce a new concept)?  Will that cause scrollbars to get added, and if so
can you check at that point to make sure EOverflow actually permits scroll bars?

[bokan, 2013-05-30 18:23:51]

https://codereview.chromium.org/15725009/diff/1/Source/core/platform/Scrollab...
File Source/core/platform/ScrollableArea.cpp (right):

https://codereview.chromium.org/15725009/diff/1/Source/core/platform/Scrollab...
Source/core/platform/ScrollableArea.cpp:85: bool
ScrollableArea::touchScroll(ScrollDirection direction, ScrollGranularity
granularity, float multiplier)
On 2013/05/30 16:15:00, Rick Byers wrote:
> Adding a new method for this feels wrong to me (as does the duplication of
code
> here).
> 
> Can't we just handle this case inside of 'scroll'?  Eg. maybe
> horizontalScrollbar returns null or a dummy value, then we fall back to
> scrollbar-less scrolling behavior.
That's what I tried at first, but that breaks non-scrollable divs since they
indicate they're non-scrollable by lack of Scrollbar. I also tried giving the
div an "invisible scrollbar" but that seemed much more heavy-weight and risky.

The other option is to add a flag parameter to scroll, something like "bool
forceScroll" that forces the scroll to occur even in the absence of scrollbars.
Thoughts?

https://codereview.chromium.org/15725009/diff/1/Source/core/platform/Scrollab...
Source/core/platform/ScrollableArea.cpp:97: return
scrollAnimator()->scroll(orientation, granularity, 1.0f, multiplier);
On 2013/05/30 16:15:00, Rick Byers wrote:
> Is treating all values of 'granularity' really ok?  How might a value other
than
> ScrollByPixel/ScrollByPrecisePixel get here?
In theory, it looks like a wheel could send a non-pixel granularity but the
horizontal wheel I tried sends pixel granularity. In that case the size of the
scroll is controlled by the multiplier parameter (6.0 for wheel when I tested).
Otherwise it looks like it should always be pixel granularity.

https://codereview.chromium.org/15725009/diff/1/Source/core/rendering/RenderB...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/15725009/diff/1/Source/core/rendering/RenderB...
Source/core/rendering/RenderBox.h:586: bool isScrollableWithoutScrollbars()
const
On 2013/05/30 16:15:00, Rick Byers wrote:
> It seems a little confusing to me to add a case where scrolling is possible
but
> scrollsOverflowX returns false.
> 
> Why not just add this case to a condition of scrollsOverflowX (rather than
> introduce a new concept)?  Will that cause scrollbars to get added, and if so
> can you check at that point to make sure EOverflow actually permits scroll
bars?

Fixed. Removed this method and moved the condition inside scrollsOverflowX. It
doesn't cause scrollbars to be created.

https://codereview.chromium.org/15725009/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/15725009/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayer.cpp:3479: return touchScroll(direction,
granularity, multiplier);
Moved this call into RenderTextControlSingleLine::scroll

[aelias_OOO_until_Jul13, 2013-05-30 19:36:26]

[+wangxianzhu] This patch is another workaround around the longstanding WebKit
problem that the concept of "scrollability" and "has scrollbars" are
intermingled.  Now that we have forked I think we should fix the issue at the
root by separating the concepts instead of adding even more workarounds. 
Xianzhu, you've already run into the problem, any thoughts on this?

[Xianzhu, 2013-05-31 21:14:00]

On 2013/05/30 19:36:26, aelias wrote:
> [+wangxianzhu] This patch is another workaround around the longstanding WebKit
> problem that the concept of "scrollability" and "has scrollbars" are
> intermingled.  Now that we have forked I think we should fix the issue at the
> root by separating the concepts instead of adding even more workarounds. 
> Xianzhu, you've already run into the problem, any thoughts on this?

+trchen who is working on refactoring ScrollableArea/ScrollView/FrameView.

I agree to fix the issue of "scrollability" and "has scrollbars".

FYI I ever created a patch (https://codereview.chromium.org/14874002/) to allow
scaled-up root frame to scroll even if the body or html element has
overflow:hidden, but finally we decided to resolve the issue in the compositor
instead of in ScrollView
(https://docs.google.com/a/chromium.org/document/d/1KewSqexq4Pd99RUFB-yVoUIJoc...).

[aelias_OOO_until_Jul13, 2013-06-07 19:29:54]

OK, closing this issue since I think the solution we'll end up with will look
different from this patch.