[Android] Disable pull-to-refresh with overflow:hidden

[Android] Disable pull-to-refresh with overflow-y:hidden

Currently, the only way to disable the pull-to-refresh effect is to
explictly preventDefault the causal touches or suppress their default
actions with touch-action: none. However, the latter prevents
any kind of accelerated scrolling, and the former can be to get right
with composed or nested scrollable content.

Use the overflow-y:hidden property on the root element as a signal to
disable the effect. This dovetails with how this property both prevents
hiding of the top controls and suppresses the overscroll glow effect.

BUG=456515, 456300
Committed: https://crrev.com/2afdbf72bcfa0a2e30d31a37ca6499dcb99a62aa
Cr-Commit-Position: refs/heads/master@{#315938}

[jdduke (slow), 2015-02-10 22:37:49]

jdduke@chromium.org changed reviewers:
+ aelias@chromium.org

[jdduke (slow), 2015-02-10 22:37:50]

aelias@: PTAL. The alternative here as suggested by Rick is using the content
size to suppress the feature, i.e., if the (outer) viewport isn't scrollable,
disable the effect. My concern with that is how it makes the effect
hyper-sensitive to content length, which can vary and for any given page at any
given time. It would be slightly more consistent with overscroll glow, but as
that's purely cosmetic so there's still a functional difference.

[Rick Byers, 2015-02-10 22:48:10]

rbyers@chromium.org changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2015-02-10 22:48:11]

I like this, lgtm

A couple notes on the description: it's not true that preventDefault is the only
way to suppress this today, 'touch-action: none' is typically an easier way (if
you don't mind suppressing scrolling and zooming of the root layer).  But may
app-like sites already set body/html overflow:hidden, so disabling by default
for them makes sense.

Also, from the code I assume that 'overflow-y: hidden' is all you need to
disable the effect (eg. you don't have to suppress horizontal scrolling to),
might be worth making that more clear in the CL description.

[jdduke (slow), 2015-02-10 23:19:59]

Thanks for review!

On 2015/02/10 22:48:11, Rick Byers wrote:
> I like this, lgtm
> 
> A couple notes on the description: it's not true that preventDefault is the
only
> way to suppress this today, 'touch-action: none' is typically an easier way
(if
> you don't mind suppressing scrolling and zooming of the root layer).  But may
> app-like sites already set body/html overflow:hidden, so disabling by default
> for them makes sense.

You're absolutely right, added a note mentioning touch-action:none.

> Also, from the code I assume that 'overflow-y: hidden' is all you need to
> disable the effect (eg. you don't have to suppress horizontal scrolling to),
> might be worth making that more clear in the CL description.

Yup, adjusted the wording.

[aelias_OOO_until_Jul13, 2015-02-10 23:32:58]

On 2015/02/10 at 22:48:11, rbyers wrote:
> A couple notes on the description: it's not true that preventDefault is the
only way to suppress this today, 'touch-action: none' is typically an easier way
(if you don't mind suppressing scrolling and zooming of the root layer).  But
may app-like sites already set body/html overflow:hidden, so disabling by
default for them makes sense.

Hmm, you're implying that "touch-action: none" doesn't affect scrollable
subelements, allowing them to scroll on the impl thread even if the body element
has touch-action: none?  How is that defined exactly?  Does it depend on
stacking contexts?

If that's already sufficient, I'm not sure we actually should do anything
special with "overflow: hidden" anymore.  It's not really obvious that
"overflow: hidden" should do anything with pull-to-refresh.  But pull-to-refresh
is indeed a "default touch action" so it's logical that touch-action: none
should disable it.  And the current behavior is friendlier to a hypothetical
developer who wants to disable scrolling, but likes the default refresh
behavior.

[jdduke (slow), 2015-02-11 00:09:15]

On 2015/02/10 23:32:58, aelias wrote:
> On 2015/02/10 at 22:48:11, rbyers wrote:
> > A couple notes on the description: it's not true that preventDefault is the
> only way to suppress this today, 'touch-action: none' is typically an easier
way
> (if you don't mind suppressing scrolling and zooming of the root layer).  But
> may app-like sites already set body/html overflow:hidden, so disabling by
> default for them makes sense.
> 
> Hmm, you're implying that "touch-action: none" doesn't affect scrollable
> subelements, allowing them to scroll on the impl thread even if the body
element
> has touch-action: none?  How is that defined exactly?  Does it depend on
> stacking contexts?

Sorry, I didn't mean to imply that, the new wording is poor.

> 
> If that's already sufficient, I'm not sure we actually should do anything
> special with "overflow: hidden" anymore.  It's not really obvious that
> "overflow: hidden" should do anything with pull-to-refresh.  But
pull-to-refresh
> is indeed a "default touch action" so it's logical that touch-action: none
> should disable it.  And the current behavior is friendlier to a hypothetical
> developer who wants to disable scrolling, but likes the default refresh
> behavior.

It's not sufficient for the general case of allowing scrollable subelements.
touch-action: none will prevent pull-to-refresh however.

[Rick Byers, 2015-02-11 04:51:03]

On 2015/02/10 23:32:58, aelias wrote:
> On 2015/02/10 at 22:48:11, rbyers wrote:
> > A couple notes on the description: it's not true that preventDefault is the
> only way to suppress this today, 'touch-action: none' is typically an easier
way
> (if you don't mind suppressing scrolling and zooming of the root layer).  But
> may app-like sites already set body/html overflow:hidden, so disabling by
> default for them makes sense.
> 
> Hmm, you're implying that "touch-action: none" doesn't affect scrollable
> subelements, allowing them to scroll on the impl thread even if the body
element
> has touch-action: none?  How is that defined exactly?  Does it depend on
> stacking contexts?

Ah, crap - you're right.  The "effective touch-action" depends on the
touch-action of the hit element and it's ancestors up to the nearest scroller. 
So setting touch-action: none on the document won't prevent the document from
getting scroll chaining (again we need scroll extensibility to block chaining),
and so won't fully prevent pull to refresh.  Sorry for the confusion.

That's a good argument for why this should be coupled to overflow.

> If that's already sufficient, I'm not sure we actually should do anything
> special with "overflow: hidden" anymore.  It's not really obvious that
> "overflow: hidden" should do anything with pull-to-refresh.  But
pull-to-refresh
> is indeed a "default touch action" so it's logical that touch-action: none
> should disable it.  And the current behavior is friendlier to a hypothetical
> developer who wants to disable scrolling, but likes the default refresh
> behavior.

[aelias_OOO_until_Jul13, 2015-02-11 06:43:38]

lgtm

[jdduke (slow), 2015-02-11 16:55:02]

I just realized I'm going to have to tweak this a bit. This reports scrolls
beyond an overflow:hidden as "consumed", but that will effectively prevent our
async -> sync touchmove transition when overscrolling.

I think I can do this by restoring the old "consumed" semantics, and using the
overscroll signal as an input to the pull-to-refresh logic. Stay tuned.

[jdduke (slow), 2015-02-11 19:05:35]

New patchsets have been uploaded after l-g-t-m from
rbyers@chromium.org,aelias@chromium.org

[jdduke (slow), 2015-02-11 19:19:50]

jdduke@chromium.org changed reviewers:
+ sievers@chromium.org

[jdduke (slow), 2015-02-11 19:19:51]

OK, the latest version accommodates overflow:hidden without affecting the
InputEventAckState. In short, we listen to the  overscroll notification, rather
than just the ack state, to determine whether or not to start the effect.
Overscroll is suppressed when the root axes aren't user-scrollable, or as soon
as any scrolling occurs.

My only worry here is that the effect now doesn't work for main thread
scrolling, but that should hopefully be resolved soon... (crbug.com/420207).

+sievers@ for content/renderer/gpu/render_widget_compositor.cc review.

[jdduke (slow), 2015-02-11 19:30:46]

On 2015/02/11 19:19:51, jdduke wrote:
> OK, the latest version accommodates overflow:hidden without affecting the
> InputEventAckState. In short, we listen to the  overscroll notification,
rather
> than just the ack state, to determine whether or not to start the effect.
> Overscroll is suppressed when the root axes aren't user-scrollable, or as soon
> as any scrolling occurs.
> 
> My only worry here is that the effect now doesn't work for main thread
> scrolling, but that should hopefully be resolved soon... (crbug.com/420207).
> 
> +sievers@ for content/renderer/gpu/render_widget_compositor.cc review.

The alternative here would be to distinguish between NOT_CONSUMED and
NO_CONSUMER_EXISTS. The latter would be reported for scenarios like
overflow:hidden.

[aelias_OOO_until_Jul13, 2015-02-11 20:42:05]

Hmm, would it be simpler to just plumb a bool about root overflow-y: hidden in
the CompositorFrameMetadata and disable the pull-to-refresh effect directly when
that value is true?

[jdduke (slow), 2015-02-11 20:44:45]

On 2015/02/11 20:42:05, aelias wrote:
> Hmm, would it be simpler to just plumb a bool about root overflow-y: hidden in
> the CompositorFrameMetadata and disable the pull-to-refresh effect directly
when
> that value is true?

Works for me, as long as we can do it per axis. Sound good?

[jdduke (slow), 2015-02-11 20:45:16]

On 2015/02/11 20:44:45, jdduke wrote:
> On 2015/02/11 20:42:05, aelias wrote:
> > Hmm, would it be simpler to just plumb a bool about root overflow-y: hidden
in
> > the CompositorFrameMetadata and disable the pull-to-refresh effect directly
> when
> > that value is true?
> 
> Works for me, as long as we can do it per axis. Sound good?

Oops, I misread your comment as overflow:hidden, yes overflow-y:hidden should be
sufficient.

[jdduke (slow), 2015-02-11 22:17:59]

jdduke@chromium.org changed reviewers:
+ dcheng@chromium.org
- sievers@chromium.org

[jdduke (slow), 2015-02-11 22:18:00]

OK, made the suggested change, much cleaner and should just work with main
thread scrolling.

+dcheng for content/common/cc_messages.h review.

[dcheng, 2015-02-11 22:19:23]

lgtm

[jdduke (slow), 2015-02-11 22:22:56]

https://codereview.chromium.org/910373002/diff/40001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/910373002/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2681: const LayerImpl*
layer_for_user_scrollable_testing =
Oops, I'll go ahead and revert this change, it's not necessary for the patch.

[jdduke (slow), 2015-02-11 22:30:08]

New patchsets have been uploaded after l-g-t-m from dcheng@chromium.org

[jdduke (slow), 2015-02-11 22:31:34]

Patchset #4 (id:60001) has been deleted

[aelias_OOO_until_Jul13, 2015-02-12 04:03:23]

lgtm

[jdduke (slow), 2015-02-12 04:50:11]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-02-12 04:50:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/910373002/80001

[commit-bot: I haz the power, 2015-02-12 06:28:29]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-02-12 06:29:11]

Patchset 4 (id:??) landed as
https://crrev.com/2afdbf72bcfa0a2e30d31a37ca6499dcb99a62aa
Cr-Commit-Position: refs/heads/master@{#315938}