1814886 - Dynamic Toolbar misbehaves when panning a child element

Status: NEW

(Core :: Panning and Zooming, defect, P3)

Description

[Alex Jakobi [:ajakobi]]

Found during implementation of Bug #1805851

Summary

When scrolling a child element that has set overscroll-behavior: none and no room to scroll the Dynamic Toolbar is hidden and revealed.

Reproduction

See the attached test page and reproduction video.
Try to scroll the green child element vertically.

Observed Behavior

The Dynamic Toolbar is hidden and revealed.

Expected Behavior

Scrolling should have no effect.

Comment 1

[Alex Jakobi [:ajakobi]]

Attachment: Test page

Attached a test page for reproduction. This can also be found here:
https://denvercoder21.github.io/firefox-debugging/bugs/1814886.html

Comment 2

[Alex Jakobi [:ajakobi]]

Attachment: Reproduction Chrome

• No scrolling of the parent triggered
• No pull to refresh triggered
• No hide/reveal of dynamic toolbar triggered

Comment 3

[Alex Jakobi [:ajakobi]]

Attachment: Reproduction Firefox

• No scrolling of the parent is triggered
• No pull to refresh is triggered
• Hide/reveal of dynamic toolbar is triggered

Comment 4

[Botond Ballo [:botond]]

Interesting findings!

Having thought about this a bit more, I do think that not triggering pull-to-refresh should be the expected behaviour here: the reasoning being that pull-to-refresh is a boundary default action for the root, and overscroll-behavior: none on the child should mean nothing happens to the root (including no boundary actions being triggered).

This does lead me to wonder what the behaviour is if the root is not scrolled first: does trying to scroll the child up trigger pull-to-refresh then?

As for the dynamic toolbar behaviour, I would expect that it is not moved, for the same reason, so this is an issue we should probably fix. (But I'm not sure if the fix will be in ArePointerEventsConsumable or somewhere else.)

Comment 5

[Alex Jakobi [:ajakobi]]

I completely agree with your expectation of not triggering pull-to-refresh.

I re-tested on Android with Chrome and Firefox, this time without the initial scroll. The behavior is identical to the one with the initial scroll you see in the screen captures.

For 'completeness' sake: Safari on iOS performs a scroll on the parent.

Regarding the dynamic toolbar behavior, I also agree with you: There should be no movement. Should we address it within the scope of this ticket or create a new one?

Comment 6

[Botond Ballo [:botond]]

(In reply to Alex Jakobi from comment #5)

I re-tested on Android with Chrome and Firefox, this time without the initial scroll. The behavior is identical to the one with the initial scroll you see in the screen captures.

Thanks for checking!

I think this tells us that, while ArePointerEventsConsumable not checking for overscroll-behavior may technically be incorrect from the point of view of the internal API, it doesn't actually lead to any user-visible misbehaviour. That is, while it may return mHasRoom=true for a subframe with overscroll-behavior: none where the conceptually more correct value would be mHasRoom=false, the consequence of this is that pull-to-refresh is not triggered, and we don't want it to be triggered in this scenario anyways.

It's probably not worth making a change here given that it's not causing a user-visible bug.

Regarding the dynamic toolbar behavior, I also agree with you: There should be no movement. Should we address it within the scope of this ticket or create a new one?

I think we can re-purpose this bug to fix the dynamic toolbar behaviour. I'll leave it up to you whether to investigate that first or bug 1806218, since the dynamic toolbar issue may be unrelated to ArePointerEventsConsumable.

Comment 7

[Alex Jakobi [:ajakobi]]

(In reply to Botond Ballo [:botond] [away until 02/17] from comment #6)

It's probably not worth making a change here given that it's not causing a user-visible bug.

Thanks for the clarification, I agree there's no change necessary with the kind of behavior we see.

I think we can re-purpose this bug to fix the dynamic toolbar behaviour. I'll leave it up to you whether to investigate that first or bug 1806218, since the dynamic toolbar issue may be unrelated to ArePointerEventsConsumable.

Changed the title of the bug and edited/expanded the description with some useful information.

bug 1806218

I will look into that next.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

One thing I should comment is that if the child scroll container height is 100vh, i.e. it covers whole screen including under the dynamic toolbar area, we don't need to move the dynamic toolbar?

Comment 9

[Alex Jakobi [:ajakobi]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)

One thing I should comment is that if the child scroll container height is 100vh, i.e. it covers whole screen including under the dynamic toolbar area, we don't need to move the dynamic toolbar?

For that case I would say the dynamic toolbar should move. I'm thinking of the situation where the child container takes up almost all of the visible space and its content represents the main parts of the web page. Then I would expect the dynamic toolbar to show up when scrolling inside of that child element.

What do you think, Hiro and Botond?

Comment 10

[Hiroyuki Ikezoe (:hiro)]

When I was commenting comment 8, I would have said yes. But after thinking more, now my answer is it's hard to tell..

Given that on desktop versions Firefox, with a 100vh height child scrollable container, which entirely covers the browser rendering region, scrolling on the child container should hand off to the parent with overscroll-behavior: none style? I don't think so. That's said, on desktop versions of Firefox, there should be the vertical scrollbar on the root scroll container so that users can reach out rest of the contents by dragging the scrollbar. I believe on mobile versions there should also appear the scrollbar, so probably we should just respect the overscroll-behavior style on the target scroll container?

Comment 11

[Botond Ballo [:botond]]

It's an interesting edge case. I've raised a variation of it in https://github.com/w3c/csswg-drafts/issues/3267, and the feedback there seemed to suggest that implementations should not special-case the scenario where a child scroller takes up all of the viewport; if the page author makes the child scroller overscroll-behavior: none and as a result the user has no way of scrolling the root, then so be it, the user can't scroll the root.