1788504 - IntersectionObserver not triggering to populate infinitite-scrolling comments on YouTube, in Fenix with dynamic toolbar (but does fire on Chrome, and in Fenix with always-present toolbar)

Status: RESOLVED FIXED

(Core :: Layout: Scrolling and Overflow, defect, P2)

Description

[Thomas Wisniewski [:twisniewski]]

As reported at webcompat.com, YouTube's desktop layout never reveals its comment area under videos on Fenix , when the user scrolls down to the bottom of the page. (This can be seen if viewing the desktop version of the page the usual way in Fenix).

It turns out that they rely on an IntersectionObserver to trip as a signal to show the comments:

this.observer = new IntersectionObserver(this.handleObserveEvent.bind(this), 
    // omitting some irrelevant code
    {
      rootMargin: '-0.1px'
    });

This never seems to fire on Fenix, but it does on Chrome. I'm not sure if this is specific to Fenix, so I'm filing this as a potential Gecko issue.

Comment 1

[Daniel Holbert [:dholbert]]

This is related to the Dynamic Toolbar -- if you turn that behavior off[1], then I get EXPECTED RESULTS. (vs. with the dynamic toolbar enabled, I can reproduce this bug regardless of whether the toolbar is at the top or the bottom.)

hiro, I think you've been triaging/working on dynamic toolbar stuff recently; do you know if this is a version of a known issue there?

[1] Fenix settings, Customize, "Scroll to hide toolbar: off"

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I haven't heard any bugs related to interactions between IntersectionObserver and dynamc toolbar. If the issue happens only on desktop mode, bug 1641166 is probably the same root cause. If it also happens on visualViewport.scale==1 cases, I am not sure where the problem is.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Oops, https://github.com/w3c/IntersectionObserver/issues/95 would be relevant.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Long story short, IntersectionObserver doesn't work with the visual viewport, and dynamic toolbar basically changes the visual viewport rather than the layout viewport. If Chrome's IntersectionObserver works with the visual viewport, it's surprising. If Chrome's dynamic toolbar changes the layout viewport, it's more surprising. I don't believe the latter is true.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: screencast of Chrome and then Firefox

Thanks! FWIW this bug doesn't reproduce in Chrome (they scroll just fine here). So they're doing something different; not sure what.

Here's a screencast of Chrome and then Fenix, viewing https://m.youtube.com/watch?v=4fezP875xOQ on my Pixel 4a with "Desktop site" enabled in the browser menu. Infinite-scroll works in Chrome (lazily populating more content) but does not work in Firefox, with dynamic toolbar disappearing in both browsers.

(For anyone else trying to test with disappearing dynamic toolbar in Chrome: note that their toolbar doesn't hide right away, but if you wait ~5 seconds after pageload, it becomes dynamic and auto-hides for scroll operations from that point on.)

Comment 6

[Dennis Schubert [:denschub]]

Attachment: Simple testcase for this issue

Well, I just re-diagnosed this very issue - and built a nice test case for it. While I have no idea why I didn't find this bug earlier, at least we have a testcase now..!

Comment 7

[Emilio Cobos Álvarez (:emilio)]

The rect we're using is the root scrollport rect here: https://searchfox.org/mozilla-central/rev/aa329cf7506ddd966542e642ec00223fd7461599/dom/base/DOMIntersectionObserver.cpp#568

Hiro, Botond, is it expected that that doesn't account for the dynamic toolbar somehow? I guess we could expand by the dynamic toolbar height? But I'd be surprised if that would be the only place where we'd need it...

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Yes, see the link to the spec issue in See Also; https://github.com/w3c/IntersectionObserver/issues/95.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

Hiro, Botond, is it expected that that doesn't account for the dynamic toolbar somehow? I guess we could expand by the dynamic toolbar height? But I'd be surprised if that would be the only place where we'd need it...

You're right, there's a function just for that nsLayoutUtils::ExpandHeightForDynamicToolbar

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1788504 - Account for current dynamic toolbar state in IntersectionObserver. r=hiro

I believe this is the most sensible behavior maybe even the only
reasonable behavior...

Hiro, do you know how to test this?

Comment 11

[Hiroyuki Ikezoe (:hiro)]

If we do follow the Chrome behavior, we need to clarify the behavior.

There are three possible behaviors what Chrome does.

1. Using the rect including the dynamic toolbar area even if the toolbar is visibile
2. Using the rect including the dynamic toolbar area only if the toolbar is completely hidden
3. Using the visual viewport(-ish) rect

D162475 is doing 1), and I don't think it's what Chrome does.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

I intentionally wasn't mentioning the minimum-scale size, but the original report is mentioning it's on desktop mode... So I suppose the test case in comment 6 doesn't replicate the original issue. And things are bit more complicated.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

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

D162475 is doing 1), and I don't think it's what Chrome does.

Ah, yeah, I naively thought that ExpandHeightForDynamicToolbar would expand by the current height (so using 3). That should be trivial to fix tho.

Comment 14

[Dennis Schubert [:denschub]]

I'm not sure if I'm the best person to investigate what exactly Chrome does. I'm happy to do that, but I'd probably need a lot longer than someone like Emilio, who already has more experience with viewport-related stuff. I'll cancel my ni? for now - but if you want me to spend some time properly investigating something, please do let me know!

Comment 15

[Hiroyuki Ikezoe (:hiro)]

The next step to move this issue forward is to clarify what Chrome does in this specific case.

If Chrome's IntersectionObserver uses the visual viewport, it's currently under discussion as a spec issue (https://github.com/w3c/IntersectionObserver/issues/95).

If Chrome's IntersectionObserve uses something viewport-ish metrics in comment 11, we need to clarify what it is.

Comment 16

[Raul Bucata]

Verified this issue and still reproduces on Firefox Nightly. Comment section is missing when visiting youtube in desktop mode

Tested with:

Browser / Version: Firefox Nightly 128.0a1 (2016022983-🦎128.0a1-20240525201917🦎)
Operating System: Google Pixel 3 (Android 12) -1080 x 2160 pixels, 18:9 ratio (~443 ppi density)
Operating System: Oppo Find X5 (Android 13) - 1080 x 2400 pixels, 20:9 ratio (~402 ppi density)

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

So the spec issue has had pretty much no action since before the pandemic. The last poll had most in favor of visual viewport, with 1/3 preferring it to be configurable. Since Chrome isn't currently configurable, I suggest we implement visual viewport (which I think is what Chrome is doing), since that will likely solve a number of webcompat, and use that work to poke the issue.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1788504 - Account for current dynamic toolbar state in IntersectionObserver. r=emilio

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case I used

Randell, would you mind providing test cases which make you think Chrome uses visual viewport? (Or pointing me out where the visual viewport is used in Chrome code?)

Attaching file is a test case I used to check the Chrome behavior. With

1. try to pinch zoom in as possible
2. scroll down a bit to be able to see a horizontal bar

I am seeing the bar is red even if the bar is inside the visual viewport, which is something wrong.

This may be just an edge case that Chrome doesn't handle properly with the dynamic toolbar.

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

I got red initially in Chrome I'm pretty sure, but on some more zooming and scrolling it became green and stayed that way.
On Reload, all I can get is green. Ditto after stopping and restarting chrome

Comment 21

[Hiroyuki Ikezoe (:hiro)]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #20)

I got red initially in Chrome I'm pretty sure, but on some more zooming and scrolling it became green and stayed that way.
On Reload, all I can get is green. Ditto after stopping and restarting chrome

After reloading the contents, Chrome restores the last zooming state, thus it's not something the test case wants to check.

Anyways, the new patch Emilio posted is not using the visual viewport for IntersectionObserver. If it fixes most cases, I will be happy to take the approach.

Dennis, would you mind listing up sites affected by this bug? Youtube is the only one site? Thanks!

Comment 22

[Dennis Schubert [:denschub]]

There's also bug 1720060, which I think is the same cause? Besides those two, I don't see anything.

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Thanks Dennis. Unfortunately I can't check bug 1720060 since it's not available from my country though, I realized Emilio's updated patch (D212920) doesn't fix the youtube case. And I start suspecting this bug may be unrelated to visual viewport. Let me take some time on it. I will figure out ways to fix the youtube case, it would be not simple.

Even though D212920 doesn't fix the original case, the change itself is pretty reasonable to me.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case that IntersectionObserver doesn't work with dynamic toolbar either on Firefox or on Chrome

STR is to scroll down carefully to move the dynamic toolbar you will see a red bar (NOTE: you don't need to use desktop mode)

Now I am very confused. If the reason why youtube with desktop mode works on Chrome is just because Chrome's intersection observer accounts for the dynamic toolbar state, this attaching test case should work. But in fact it doesn't work.

So there's definitely another reason why youtube with desktop mode works on Chrome.

On the other hand, I've written an additional change upon D212920 to make youtube with desktop works by using viewport-ish metrics for our intersection observer implementation. It was simpler than I thought, thanks to D212920.

So here are two options;

a) Re-investigate why youtube with desktop mode works on Chrome and align our behavior match Chrome's behavior
b) Fix this bug in a different way from what Chrome does

I think a) is preferable, but it will take some more amount of time, given that investigating Chrome's behavior is harder than investigating our behavior (implementation).

James, opinions?

Comment 25

[James Graham [:jgraham]]

No strong opinion.

If we think that the patch you've written is a strict improvement then it seems sensible to land that first and continue to investigate how to align with Chrome's behaviour in the meantime. On the other hand if we're concerned that it might break some other sites we might be better off holding back. One option might be to ask QA to test some sites where we've previously seen dynamic toolbar issues in a build with the patch applied, and see if it regresses anything.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Attachment: A new test case slightly different from attachment.cgi?id=9408338

Thanks, James. A good news is, in the meantime I was asking, I was doing further investigation behaviors on Chrome, with the further investigation, now I am pretty sure that Chrome doesn't use visual viewport for IntersectionObserver at all. They rather use bimodal layout viewport.

Attaching file is a test case I used to check it. If you;

1. scroll down slowly to hide the dynamic toolbar completely
2. lift your finger once from touchscreen
   3-1) if you've already seen a bar there, you scrolled too much, you have to scroll up and do 1) again
   3-2) if you haven't seen a bar there, scroll down more to see the bar

You will see the bar is green, not red.

So, on Chrome

a) once after the dynamic toolbar is completely hidden, then they use the layout viewport + the area where the dynamic toolbar was initially occupied.
b) while the dynamic toolbar is visible, they use the original layout viewport (which is not including the dynamic toolbar area)

I was relieved they didn't change IntersectionObserver to use visual viewport :)

I think we can do the same thing, actually I wrote it locally, unfortunately as of now the change causes a couple of test failures, so there need more work, but I can do it.

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1788504 - Add test cases working with the bimodal layout viewport IntersectionObserver uses. r?emilio

There are basically two types of test cases.

a) While the dynamic toolbar is visible, IntersectionObserver uses the layout
viewport not including the dynamic toolbar height
b) While the dynamic toolbar is completely hidden, IntersectionObserver uses
the layout viewport + the height that the dynamic toolbar was originally occupied

And there are two different test patterns for a) and b) respectively. One is
for normal mobile rendering, the other one is for "desktop mode", i.e. the
document is scaled by < 1.0 initially. The latter one is exactly the original
report case, youtube.com with desktop mode.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Attachment: The additonal change

Comment 29

[Hiroyuki Ikezoe (:hiro)]

Some tests hit an assertion in the additional change, I thought MobileViewportManager is always there but there are some cases it's not.

So, now rather than posting a new updated change here, I did steal D212920 and integrated the change into it. Hope it's a better approach here.

Comment 31

[Daniel Holbert [:dholbert]]

Alternate testcase from my dupe bug 1905441, in case it's helpful:
https://bug1905441.bmoattachments.org/attachment.cgi?id=9410248

EXPECTED RESULTS: The page should turn lime when you scroll to the end.
ACTUAL RESULTS (in Fenix with dynamic toolbar): The page remains pink.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d24f6a7a58ba
Account for current dynamic toolbar state in IntersectionObserver. r=emilio
https://hg.mozilla.org/integration/autoland/rev/111141cbf859
Add test cases working with the bimodal layout viewport IntersectionObserver uses. r=geckoview-reviewers,owlish

Comment 33

[Aaron Train [:aaronmt]]

Hello, this seems to be causing consistent tab crashes in debug Fenix in a couple of Espresso UI tests over (tracking bug 1908660).

Matrix: https://console.firebase.google.com/project/moz-fenix/testlab/histories/bh.66b7091e15d53d45/matrices/5420560761048072013/details?stepId=bs.49773e37249fabbd&testCaseId=3&tabId=logs

(You may need IAM access, please DM me on Slack to view the above)

There's not much to go by in the logcat, but it should be reproducible locally.
./mach try --preset firefox-android will schedule UI tests across {focus/fenix/A-C}`
https://firefox-source-docs.mozilla.org/mobile/android/fenix/UI-Tests.html

07-18 05:06:58.488 10383 10383 E mozac/CrashReporter: CrashHandlerService received native code crash
07-18 05:06:58.491   553   592 D CompatibilityChangeReporter: Compat change id reported: 136219221; UID 10178; state: ENABLED
07-18 05:06:58.495 10383 10434 I mozac/CrashReporter: Received crash: NativeCodeCrash(timestamp=1721304418494, minidumpPath=/data/user/0/org.mozilla.fenix.debug/files/mozilla/Crash Reports/pending/21c3049d-f6c0-050d-9d15-c154a97b2144.dmp, minidumpSuccess=false, extrasPath=/data/user/0/org.mozilla.fenix.debug/files/mozilla/Crash Reports/pending/21c3049d-f6c0-050d-9d15-c154a97b2144.extra, processType=FOREGROUND_CHILD, breadcrumbs=[], remoteType=web, runtimeTags={}, uuid=311f1804-34f2-4f27-9c84-0add22514a75)
07-18 05:06:58.514   553   592 D CompatibilityChangeReporter: Compat change id reported: 135634846; UID 10178; state: DISABLED
07-18 05:06:58.514   553   592 D CompatibilityChangeReporter: Compat change id reported: 135754954; UID 10178; state: ENABLED
07-18 05:06:58.514   553   616 D CompatibilityChangeReporter: Compat change id reported: 143937733; UID 10178; state: ENABLED
07-18 05:06:58.514 10383 10434 I mozac/CrashReporter: Invoking non-fatal PendingIntent
07-18 05:06:58.515   553   592 I ActivityTaskManager: START u0 {flg=0x14000000 cmp=org.mozilla.fenix.debug/org.mozilla.fenix.HomeActivity (has extras)} from uid 10178
07-18 05:06:58.515   553   597 D EventSequenceValidator: Transition from ACTIVITY_FINISHED to INTENT_STARTED
07-18 05:06:58.517   312   312 D goldfish-address-space: claimShared: Ask to claim region [0x1f6e7c000 0x1f738c000]
07-18 05:06:58.517   300   300 D Zygote  : Forked child process 10439
07-18 05:06:58.518   553   616 I ActivityManager: Start proc 10439:org.mozilla.fenix.debug:crashReportingProcess/u0a178 for service {org.mozilla.fenix.debug/mozilla.components.lib.crash.service.SendCrashTelemetryService}

Comment 34

[Aaron Train [:aaronmt]]

07-18 05:06:57.976 9776 9807 D GeckoViewContent: observe: oop-frameloader-crashed also in the logs right above

logcat

Comment 35

[Aaron Train [:aaronmt]]

STR:

• Visit data:text/html,123 or even wikipedia.org
• Click Main Menu -> Print, or Main Menu -> Share -> Save as PDF

native crashes recorded in about:crashes

Comment 36

[Daniel Holbert [:dholbert]]

The comment 35 STR generate a PDF just fine for me, with no crashes, using Firefox Nightly on Android 14 / Pixel 8. (I used Menu|Print with the data URI). No crash reports show up in about:crashes.

@aaronmt if you're seeing that reliably happen, maybe worth filing a new bug with more details and flagging it as a see-also for this one? Even if it ends up being a duplicate, it's probably cleaner to start with a new bug for now given that you've got STR and given that this is the first mention of printing on this bug.

Comment 37

[Daniel Holbert [:dholbert]]

(whoops, sorry, I didn't read too far up into recent comments when posting comment 36; it seems comment 35 is reporting a regression introduced by the patches that recently landed here in comment 32. My testing in comment 36 was just using latest Nightly which definitely wouldn't have the landed patches yet.)

Comment 38

[Emilio Cobos Álvarez (:emilio)]

Yeah I'm trying to debug this, but no luck so far (bug 1908713).

Comment 39

[Aaron Train [:aaronmt]]

As this manifests as a reproducible web content tab crash, not sure if that's something you'd like to back-out before it makes it into Nightly for July 19th?

Comment 40

[Emilio Cobos Álvarez (:emilio)]

For context, I'm actively investigating, and I got around what was preventing me from doing so (bug 1908713). I expect the fix to be trivial but I'll ask the sheriffs to back out if not.

Comment 41

[Emilio Cobos Álvarez (:emilio)]

For context, I finally managed to get the stack:

    frame #0: 0x0000714b126d982a libxul.so`mozilla::PresShell::GetMobileViewportManager() const [inlined] RefPtr<MobileViewportManager>::RefPtr(this=0x0000714b17ea0ae8, aSmartPtr=0x0000000000000170) at RefPtr.h:92:27
    frame #1: 0x0000714b126d982a libxul.so`mozilla::PresShell::GetMobileViewportManager(this=0x0000000000000000) const at PresShell.cpp:11148:10
  * frame #2: 0x0000714b1277fd3d libxul.so`nsPresContext::GetDynamicToolbarMaxHeightInAppUnits(this=0x0000714afbb8c800) const at nsPresContext.cpp:3071:51
    frame #3: 0x0000714b12776608 libxul.so`nsPresContext::AdjustSizeForViewportUnits(this=0x0000714afbb8c800) at nsPresContext.cpp:3010:30
    frame #4: 0x0000714b1277fc17 libxul.so`nsPresContext::SetVisibleArea(this=0x0000714afbb8c800, r=<unavailable>) at nsPresContext.cpp:2966:5
    frame #5: 0x0000714b12b453c7 libxul.so`nsPrintJob::ReflowPrintObject(this=0x0000714afbb39ba0, aPO=0x0000714afbb39c00) at nsPrintJob.cpp:1331:22
    frame #6: 0x0000714b12b44c08 libxul.so`nsPrintJob::ReflowDocList(this=0x0000714afbb39ba0, aPO=0x0000714afbb39c00) at nsPrintJob.cpp:959:3
    frame #7: 0x0000714b12b41c90 libxul.so`nsPrintJob::InitPrintDocConstruction(this=0x0000714afbb39ba0, aHandleError=false) at nsPrintJob.cpp:1000:5
    frame #8: 0x0000714b12b40d2c libxul.so`nsPrintJob::DoCommonPrint(this=0x0000714afbb39ba0, aIsPrintPreview=<unavailable>, aPrintSettings=0x0000714b1747a320, aWebProgressListener=0x0000000000000000, aDoc=0x0000714afbe70400) at nsPrintJob.cpp:446:3
    frame #9: 0x0000714b12b41e66 libxul.so`nsPrintJob::Print(mozilla::dom::Document&, nsIPrintSettings*, mozilla::layout::RemotePrintJobChild*, nsIWebProgressListener*) [inlined] nsPrintJob::CommonPrint(this=0x0000714afbb39ba0, aIsPrintPreview=false, aPrintSettings=0x0000714b1747a320, aWebProgressListener=0x0000000000000000, aSourceDoc=0x0000714afbe70400) at nsPrintJob.cpp:335:17

Will send a follow-up to fix the crash, but hiro can you look? I'm not sure we should treat print docs as having a dynamic toolbar?

Comment 42

[Emilio Cobos Álvarez (:emilio)]

Can you also check https://hg.mozilla.org/integration/autoland/rev/d24f6a7a58ba#l3.107? Pretty sure it should not call the InAppUnits() version of the method (or should not use the coordinate conversion)?

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab0e584321a9
Null-check presshell to fix Fenix UI tests.

Comment 44

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1788504 - Fix coordinate spaces of the original patch. r=hiro

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Thanks for the quick fix, Emilio!

(In reply to Emilio Cobos Álvarez (:emilio) from comment #41)

Will send a follow-up to fix the crash, but hiro can you look? I'm not sure we should treat print docs as having a dynamic toolbar?

In print docs, I don't think we need dynamic toolbar at all, which means we should also check IsPrintingOrPrintPreview(), I think.

Comment 46

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/d24f6a7a58ba
https://hg.mozilla.org/mozilla-central/rev/111141cbf859

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/601de5fecb3e
Fix coordinate spaces of the original patch. r=hiro

Comment 48

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/ab0e584321a9
https://hg.mozilla.org/mozilla-central/rev/601de5fecb3e

Comment 49

[Petru-Mugurel Lingurar [:petru]]

I think this also regressed bug 1910244 (based on git bisect)
Emilio can you please investigate and advise?

Comment 50

[Emilio Cobos Álvarez (:emilio)]

Will comment in there.