1724358 - [Fission] Moving cursor over twitter embed causes page to not respond to scrolling

Status: RESOLVED FIXED

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

Description

[Kestrel]

STR:

1. Enable Fission.
2. Visit https://www.troyhunt.com/the-internet-of-things-is-a-complete-mess-and-how-to-fix-it/.
3. Wait for the first twitter embed to load and move the mouse cursor over it.
4. Scroll with the mousewheel or touchpad.

Expected:

Page scrolls normally like with Fission disabled.

Actual:

Nothing happens.

If the cursor is held still while scrolling, it will continue to work while passing over the embed. Scrolling only stops working when the cursor moves while over the embed.

Comment 1

[Timothy Nikkel (:tnikkel)]

Regression range

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1e3244e602fc0373508049b6fe544332f800f033&tochange=3c70f36ad62c9c714db3199fc00e60800ee82bde

From may 2019 and includes

Bug 1544811 - Use web processes on a per-site basis for fission-enabled windows
and
Bug 1533673 - Allow APZ to send fission matrices with the GPU process

So not really useful.

Comment 2

[Alice0775 White]

Regression window(using local cached build):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1e3244e602fc0373508049b6fe544332f800f033&tochange=f0566d1a9ab14fec0dc9e570fa138de14c535ccf
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=adb1c1a79a40102f634a419b6c5d1245de474d67&tochange=25bc2e11f12fa6b52e8783dd468f72cfeda3f025

So, Suspect: Bug 1544811

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: pretty much minimal testcase

Comment 4

[Timothy Nikkel (:tnikkel)]

The twitter document inside the iframe has "overscroll-behavior-y: none;", if you remove that the difference between fission and non-fission goes away.

Comment 5

[Chris Peterson [:cpeterson]]

Tentatively tracking as a soft blocker for Fission MVP.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

(In reply to Timothy Nikkel (:tnikkel) from comment #4)

The twitter document inside the iframe has "overscroll-behavior-y: none;", if you remove that the difference between fission and non-fission goes away.

Okay, in Fission we have APZCs (for iframes) which are not scrollable, thus we'd need to tweak Axis::OverscrollBehaviorAllowsHandoff to allow scroll handoff in the case where the APZC is not scrollable.

Note that overscroll-behavior properties are only applied to scroll container, so it's the right thing to do.

That said, it makes me wonder if the iframe is only scrollable in the horizontal direction and if the iframe's document has overscroll-behavior-y: none, then the overscroll-behavior-y should be recpected? I believe it should be.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

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

That said, it makes me wonder if the iframe is only scrollable in the horizontal direction and if the iframe's document has overscroll-behavior-y: none, then the overscroll-behavior-y should be recpected? I believe it should be.

With this fact, tweaking Axis::OverscrollBehaviorAllowsHandoff is a wrong way to fix this problem, we'd need to handle it in AsyncPanZoomController.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

I am going to tweak AsyncPanZoomController::GetAllowedHandoffDirections, and leave AsyncPanZoomController::GetOverscrollableDirections since the latter will regress bug 1712874.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1724358 - Don't apply overscroll-behavior properties on non scrollable APZCs for scrolling handoff. r?botond

Comment 10

[Botond Ballo [:botond]]

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

Okay, in Fission we have APZCs (for iframes) which are not scrollable, thus we'd need to tweak Axis::OverscrollBehaviorAllowsHandoff to allow scroll handoff in the case where the APZC is not scrollable.

Note that overscroll-behavior properties are only applied to scroll container, so it's the right thing to do.

I'm a bit confused by this. Since an iframe is a document, isn't its viewport always a scroll container, even if it has no scroll range?

Comment 11

[Botond Ballo [:botond]]

Also, I think the patch as written will make it such that overscroll-behavior is ignored on scroll frames which are overflow:hidden (in the relevant direction). If I'm not mistaken, that is undesirable as well.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

(In reply to Botond Ballo [:botond] from comment #10)

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

Okay, in Fission we have APZCs (for iframes) which are not scrollable, thus we'd need to tweak Axis::OverscrollBehaviorAllowsHandoff to allow scroll handoff in the case where the APZC is not scrollable.

Note that overscroll-behavior properties are only applied to scroll container, so it's the right thing to do.

I'm a bit confused by this. Since an iframe is a document, isn't its viewport always a scroll container, even if it has no scroll range?

I am assuming the situation is only applicable to the top level document? I mean the viewport means the visual viewport, isn't it? I am not 100% sure but given that Chrome doesn't respect overscroll-behavior in the case in comment 0, it's reasonable and actually it would be intuitive for web developers I think.

Some additional background what I've naively understood are; if iframe's document element has overflow: clip it's not a scroll container since the clipped area can't be reachable at all. Also I did intentionally ignore overflow: hidden cases here, actually the overflowed area can be reachable by JS APIs, but I assume most of web developers don't differentiate overflow:hidden and overflow:clip so probably it doesn't much matter at this moment. To be exact, we should respect overscroll-behavior properties in overflow:hidden cases, but I haven't checked the behavior in the cases yet.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to Botond Ballo [:botond] from comment #11)

Also, I think the patch as written will make it such that overscroll-behavior is ignored on scroll frames which are overflow:hidden (in the relevant direction). If I'm not mistaken, that is undesirable as well.

You are right, I did ignore the case here. Do you think we should care the overflow:hidden case exactly?

Comment 14

[Botond Ballo [:botond]]

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

(In reply to Botond Ballo [:botond] from comment #10)

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

Okay, in Fission we have APZCs (for iframes) which are not scrollable, thus we'd need to tweak Axis::OverscrollBehaviorAllowsHandoff to allow scroll handoff in the case where the APZC is not scrollable.

Note that overscroll-behavior properties are only applied to scroll container, so it's the right thing to do.

I'm a bit confused by this. Since an iframe is a document, isn't its viewport always a scroll container, even if it has no scroll range?

I am assuming the situation is only applicable to the top level document? I mean the viewport means the visual viewport, isn't it?

I don't think the distinction between visual and layout viewport is meaningful for iframes. (Though, to be honest, I'm also not sure if the spec has adopted the same usage of these terms as Gecko.)

Anyways, my understanding is that a "scroll container" corresponds more or less to an nsIScrollableFrame in Gecko, and since all iframes have a root scroll frame, therefore all iframes have a scroll container.

At the least this hould be the case for iframes whose <html> element has overflow: scroll or overflow: hidden in some direction (as in the page in comment 0, where the iframe has overflow-y: scroll).

(The last paragraph of https://drafts.csswg.org/css-overflow-3/#overflow-propagation, which says:

If visible is applied to the viewport, it must be interpreted as auto. If clip is applied to the viewport, it must be interpreted as hidden.

makes me think the iframe has a scroll container even in the clip and visible cases, though it's possible I'm missing something.)

I am not 100% sure but given that Chrome doesn't respect overscroll-behavior in the case in comment 0, it's reasonable and actually it would be intuitive for web developers I think.

It may be helpful to understand the logic behind Chrome's behaviour, so it can help guide our behaviour here.

(For example, is the logic that we should only apply overscroll-behavior to scroll containers which have a non-zero scroll range? That would settle the question of what to do with overflow: hidden.)

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Found a relevant spec issue; https://github.com/w3c/csswg-drafts/issues/5129, and I've commented there.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

I seems to forget pasting other relevant text from the CSSOM; from https://drafts.csswg.org/cssom-view-1/#viewport

Elements and viewports have an associated scrolling box if the element or viewport has a scrolling mechanism, or it overflows its content area and the used value of the overflow-x or overflow-y property is not hidden or clip. [CSS3-BOX]

This sentence uses scrolling box. So from what I understand is;

1. viewport seems to be always a scroll container as Botond said
2. viewport will be a scrolling box its overflow property is not hidden or clip

To me, it's quite confusing overscroll-behavior properties should be respected in overflow:hidden scroll container cases. I've raised another spec issue; https://github.com/w3c/csswg-drafts/issues/6523

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Chrome has changed their behavior since https://bugs.chromium.org/p/chromium/issues/detail?id=759209. Since then they have been ignoring overscroll-behavior properties on non scrollable node and viewports which are not scrollable.

Note that why they explained the body case specifically is Chrome has a bug that overscroll-behavior properties are not propagated from documentElement (https://bugs.chromium.org/p/chromium/issues/detail?id=954423).

Comment 18

[Botond Ballo [:botond]]

Some notes on this for reference:

• If a scroll frame is not actually APZ-scrollable (meaning, overflow:scroll/auto and has a scroll range), it does not get an APZC (WantAsyncScroll() returns false). This is the case even with Fission's "activate all scroll frames" logic.
• However, as a special case, the root scroll frame in a process always gets an APZC (so there is something to target the events to the process).
• If a scroll frame does not get an APZC, we will not respect overscroll-behavior for it (since the logic to respect it is inside APZC).

The above means that the status quo behaviour in Firefox, prior to Fission, is that overscroll-behavior is not respected for overflow:hidden scroll frames, or overflow:scroll/auto scroll frames with no scroll range. It appears this is also Chrome's behaviour.

With Fission, this behaviour is regressed in the case the root scroll frame of a nested content process (such as a twitter embed) does not have an APZ scroll range (because such a scroll frame now gets an APZC). I now see that pretty clearly as a bug, and the patch fixes it, so let's take the patch.

Separately, there is a question of whether the status quo behaviour is desirable. The spec currently seems to suggest overscroll-behavior should apply to all scroll containers (which includes hidden etc.), and there may be some motivating use cases (Hiro suggested a comment box which is focused but not yet scrollable may not want to hand off to the enclosing page). These considerations can be discussed in the spec issue that Hiro filed, and we can implement its resolution once it has one.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6df43bd2a7bc
Don't apply overscroll-behavior properties on non scrollable APZCs for scrolling handoff. r=botond

Comment 20

[Hiroyuki Ikezoe (:hiro)]

There are a couple of JUnit tests (I wrote before) being affected by this change. :/

Comment 21

[Alexandru Michis [:malexandru]]

Backed out changeset 6df43bd2a7bc (Bug 1724358) for causing gv-junit failures in testOverscrollBehaviorNoneAuto
Backout link: https://hg.mozilla.org/integration/autoland/rev/2e74756a7120483870e36b4ca7be3a8d0f2c6f7b
Push with failures, failure log.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

The JUnit tests are interesting test cases. The the top level document is non scrollable but has overscroll-behavior: none on the root element. This is similar to the comment box case in comment 18. And I believe in this particular case, the web site really wants to disallow scroll handoff, I mean, wants to disallow pull-to-refresh feature or some such. Actually what twitter does is really this case. So I've added additional isRoot checks in D122199. That said, Chrome also allows pull-to-refresh in this case, I am not sure Chrome's browser ignores the overscroll-behavior: none or Blink doesn't tell the property value to the browser properly.

Botond, what do you think about this case? Respecting overscroll-behavior property on the top level root document element even if it's not scrollable is actually inconsistent with what we are going to do (what we've been doing) for other non-scrollable frame cases, but as far as I can tell it's the best thing, the best compromise we can do right now. I'd hope the spec will clarify these cases clearly.

Comment 23

[Botond Ballo [:botond]]

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

Respecting overscroll-behavior property on the top level root document element even if it's not scrollable is actually inconsistent with what we are going to do (what we've been doing) for other non-scrollable frame cases, but as far as I can tell it's the best thing, the best compromise we can do right now.

+1, I think it's reasonable to allow a top-level document to prevent "boundary default actions" like pull to refresh even if it's not scrollable.

(It may be worth commenting on the spec issue to draw attention to this case.)

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Thanks! I added a comment there.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d83d7058c66
Don't apply overscroll-behavior properties on non scrollable APZCs for scrolling handoff. r=botond

Comment 26

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/0d83d7058c66

Comment 27

[Chris Peterson [:cpeterson]]

Setting status-firefox92=wontfix because I don't think we need to uplift this Fission bug fix to Beta 92. Fission is disabled for users who are not in the Beta or Release 92 experiments and, IIUC, the bug only affects page scrolling when mousing over an embedded tweet. The page will scroll correctly if the user moves the mouse off the tweet.