Closed Bug 1724358 Opened 4 months ago Closed 3 months ago

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

Categories

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

Firefox 92
defect

Tracking

()

RESOLVED FIXED
93 Branch
Fission Milestone MVP
Tracking Status
firefox-esr78 --- disabled
firefox-esr91 --- disabled
firefox90 --- disabled
firefox91 --- disabled
firefox92 --- wontfix
firefox93 --- verified

People

(Reporter: ke5trel, Assigned: hiro)

References

(Blocks 1 open bug, )

Details

(Whiteboard: fission-soft-blocker)

Attachments

(2 files)

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.

Fission Milestone: --- → ?

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.

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

Tentatively tracking as a soft blocker for Fission MVP.

Fission Milestone: ? → MVP
Whiteboard: fission-soft-blocker

(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.

(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.

Assignee: nobody → hikezoe.birchill
Status: NEW → ASSIGNED
Attachment #9235526 - Attachment description: WIP: Bug 1724358 - Don't apply overscroll-behavior properties on non scrollable APZCs for scrolling handoff. → Bug 1724358 - Don't apply overscroll-behavior properties on non scrollable APZCs for scrolling handoff.
Attachment #9235526 - Attachment description: Bug 1724358 - Don't apply overscroll-behavior properties on non scrollable APZCs for scrolling handoff. → WIP: Bug 1724358 - Don't apply overscroll-behavior properties on non scrollable APZCs for scrolling handoff.
Attachment #9235526 - Attachment description: WIP: Bug 1724358 - Don't apply overscroll-behavior properties on non scrollable APZCs for scrolling handoff. → Bug 1724358 - Don't apply overscroll-behavior properties on non scrollable APZCs for scrolling handoff.

(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?

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.

(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.

(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?

(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.)

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

Severity: -- → S3
Priority: -- → P2

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

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).

Attachment #9235526 - Attachment description: Bug 1724358 - Don't apply overscroll-behavior properties on non scrollable APZCs for scrolling handoff. → Bug 1724358 - Don't apply overscroll-behavior properties on non scrollable APZCs for scrolling handoff. r?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.

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

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

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.

Flags: needinfo?(hikezoe.birchill)

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.

Flags: needinfo?(hikezoe.birchill) → needinfo?(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.)

Flags: needinfo?(botond)
Pushed by hikezoe.birchill@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d83d7058c66
Don't apply overscroll-behavior properties on non scrollable APZCs for scrolling handoff. r=botond
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 93 Branch

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.

You need to log in before you can comment on or make changes to this bug.