Closed Bug 1938966 Opened 1 year ago Closed 1 year ago

fenix (firefox android) nightly: full screen button on youtube videos doesn't go full screen (only removes chrome)

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Version:
Firefox 135
Platform:
All
Android
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
136 Branch
Tracking Flags:
Tracking Status
firefox133 --- unaffected
firefox134 --- unaffected
firefox135 + wontfix
firefox136 + fixed

People

(Reporter: sschweinsteiger92, Assigned: edgar, NeedInfo)

References

Details

(Keywords: regression, Whiteboard: [fxdroid])

Attachments

(3 files, 1 obsolete file)

fullscreenbug.mp4
3.73 MB, video/mp4
Details
Bug 1938966 - Make GeckoView actor handle fullscreen request from OOP iframe properly; r?m_kato
48 bytes, text/x-phabricator-request
Details | Review
FixedBottomContentAfterExitingFullscreen.mp4
7.99 MB, video/mp4
Details

Steps to reproduce:

Go to a web page with an embedded youtube video (e.g. https://www.theverge.com/24322968/nosferatu-review-robert-eggers). Play video and hit the 'full screen' button on the embedded player.

Actual results:

The browser chrome disappears, but the video does not take up the full screen. Using Fenix Nightly v135.0a1 (Build #2016063207)

Expected results:

The video should take up the full screen. Verified this happens on the non-nightly Fenix v133.0.3 (build #2016060959).

Summary: fenix (firefox android) nighly: full screen button on youtube videos doesn't go full screen (only removes chrome) → fenix (firefox android) nightly: full screen button on youtube videos doesn't go full screen (only removes chrome)

I can confirm the bug, I could reproduce it on Fenix Nightly 135 (Samsung S24, Android 14).

Severity: -- → S2
Status: UNCONFIRMED → NEW
status-firefox133: --- → unaffected
status-firefox134: --- → unaffected
status-firefox135: --- → affected
Component: General → Media
Ever confirmed: true

hi! thanks for flagging. i'm struggling to reproduce this on an S24, wondering what is different about my environment. Is anyone who can reproduce it able to share a video of the issue please? and maybe any info about what settings you have etc.

Attached video fullscreenbug.mp4 —

This is a screencast showing the bug being reproduced

Keywords: regression
Regressed by: 1919488

:petru, since you are the author of the regressor, bug 1919488, could you take a look?

For more information, please visit BugBot documentation.

Flags: needinfo?(petru)

The bug is marked as tracked for firefox135 (nightly). We have limited time to fix this, the soft freeze is in 8 days. However, the bug still isn't assigned.

:titouan, could you please find an assignee for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Flags: needinfo?(tthibaud)

Hrm, I'm still not seeing proper fullscreen behavior in current Nightly builds with the backout of bug 1919488 included.

Seems then that bug 1919488 did not cause this.

Based on the recording from comment 3 the app enters in immersive mode and switches to landscape, is just that the video does not get in fullscreen - this seems more like a Gecko issue since the app reacts (correctly) to Gecko informing us that the page entered fullscreen, but the recording shows that it actually didn't.

Flags: needinfo?(petru)
No longer regressed by: 1919488

Don't think we can do anything to improve this, moving it to Core - AV for triage / investigation.
Not sure if that is the right component, feel free to move it to where it suits best.

Component: Media → Audio/Video
Flags: needinfo?(tthibaud)
Product: Fenix → Core

An important note is that up until now people reproduced it on a Samsung S24 (Android 14) and a Pixel 7 (Android 15) but others could not reproduce on same devices.

Wfm S24 with 2024-12-28T11:40:20.833229659

This is a reminder regarding comment #5!

The bug is marked as tracked for firefox135 (nightly). We have limited time to fix this, the soft freeze is in 2 days. However, the bug still isn't assigned.

I'm also on a Pixel 7 with Android v15. I've disabled all of my extensions and I still have the issue.

Please let me know what additional data I can provide.

Jim, can you please help get this on the appropriate engineer's radar? Thanks!

status-firefox136: --- → affected
tracking-firefox136: --- → +
Flags: needinfo?(jmathies)

I tried the example page on a S24 Ultra at a store and Firefox Nightly didn't exhibit the issue on the example page (this was on Friday, Jan 10). My Pixel 7 still had the issue then.

I just retested on my Pixel 7 with Nightly (version 136.0a1 (Build #2016067423)) and fullscreen is now working. Not sure what changed.

Some pages still have issues. Seeing the problem on https://hackaday.com/2025/01/14/head-to-head-servos-vs-steppers/.

Emilio told me that he can repro.

Flags: needinfo?(jmathies) → needinfo?(emilio)

Samuel, can you confirm that you're logged into m.youtube.com from your phone?

Whether I can repro on the page in comment 15 seems to be tied to that. E.g.,

  • Clean profile -> cannot repro.
  • Go to m.youtube.com, log in, retry -> can't repro
  • Close nightly, open it again, retry -> can repro
  • Log out of m.youtube.com, sign out, retry -> can repro
  • Go to m.youtube.com, click on the lockpad, clear cookies and site data, retry -> can't repro
Flags: needinfo?(emilio) → needinfo?(sschweinsteiger92)

I need to go for today, but given those STR, is there any chance someone from the mobile team or QA could try to bisect this? mozregression with --app fenix on-device wipes my nightly install.

Otherwise I can try tomorrow or so.

Flags: needinfo?(petru)

I unfortunately can't repro on the emulator either...

Oh, I can repro on the emulator, but you need to close the app between (2) and (3). I updated the comment.

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

I need to go for today, but given those STR, is there any chance someone from the mobile team or QA could try to bisect this? mozregression with --app fenix on-device wipes my nightly install.

Adding Oana to handle QA helping find out when this regressed.

Flags: needinfo?(petru) → needinfo?(ohorvath)

This is a reminder regarding comment #5!

The bug is marked as tracked for firefox135 (beta) and tracked for firefox136 (nightly). We have limited time to fix this, the soft freeze is in 14 days. However, the bug still isn't assigned.

The QA team was unable to reproduce this issue following the steps from Comment 17 on Samsung S24 Ultra (Android 14), Samsung S24 (Android 14) Samsung Galaxy A53 5G (Android 14) and Google Pixel 8 Pro (Android 14).

We hardly reproduced it twice, without being able to determine the exact steps using a Google Pixel 8 (Android 14).
We will investigate this issue further, in order to provide a valid regression range.

Flags: needinfo?(ohorvath)

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

Samuel, can you confirm that you're logged into m.youtube.com from your phone?

Whether I can repro on the page in comment 15 seems to be tied to that. E.g.,

  • Clean profile -> cannot repro.
  • Go to m.youtube.com, log in, retry -> can't repro
  • Close nightly, open it again, retry -> can repro
  • Log out of m.youtube.com, sign out, retry -> can repro
  • Go to m.youtube.com, click on the lockpad, clear cookies and site data, retry -> can't repro

Yes, I am logged into m.youtube.com.

I tested in a private tab, and fullscreen works normally. I think you are correct.

Flags: needinfo?(sschweinsteiger92)

I tried to bisect:

18:50.84 INFO: Narrowed nightly regression window from [2025-01-03, 2025-01-06] (3 days) to [2025-01-05, 2025-01-06] (1 days) (~0 steps left)
18:50.84 INFO: Got as far as we can go bisecting nightlies...
18:50.84 ERROR: Sorry, but mozregression was unable to get a repository - no pushlog url available.

Which is not amazing, because that doesn't match with comment 0 (which is end of december) :(

I can consistently repro with latest nightly using comment 17 fwiw. On the same nightly, on a private tab, I see the good behavior.

On both cases, the state on the Youtube iframe seems ok:

  • window.fullScreen = true
  • document.fullscreenElement = <div id="movie_player" class="html5-video-player ytp-e…-fullscreen paused-mode" tabindex="-1" data-version="/s/player/6e1dd460/player_ias.vflset/en_US/base.js" aria-label="YouTube Video Player in Fullscreen">
  • document.fullscreen = true

However! On the top page, the state seems wrong:

  • window.fullScreen is true on both, but...
  • document.fullscreen is true on the good case and false on the bad one.
  • document.fullscreenElement is <iframe title="Servo vs stepper: Servo …ed, Torque and Accuracy" width="800" height="450" src="https://www.youtube.com/…nO1F-AO9I?feature=oembed" frameborder="0" allow="accelerometer; autoplay;…e-in-picture; web-share" referrerpolicy="strict-origin-when-cross-origin" allowfullscreen=""> on the good one, but null on the bad one.

Edgar, are you familiar with this code by any chance? Do you know what might be going on?

I wonder if something about fission breaks / fixes this...

Component: Audio/Video → DOM: Core & HTML
Flags: needinfo?(echen)

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

I wonder if something about fission breaks / fixes this...

Yeah, so I confirmed my suspicion... This breaks with fission.webContentIsolationStrategy=1 on a private tab, and should make this reproducible across devices.

I think mobile/ is missing a call to remoteFrameFullscreenChanged for fission frames: https://searchfox.org/mozilla-central/search?q=.RemoteFrameFullscreenChanged&path=&case=false&regexp=false

Assignee: nobody → echen
Flags: needinfo?(echen)

Hi Samuel, do you still have this issue? Can you please confirm what is the value of fission.autostart pref in your about:config on your Nightly? Thanks

Flags: needinfo?(sschweinsteiger92)

Hey Vasilica, can you reproduce the issue if you enrol into the fission experiment that is currently running on Nightly? The experiment slug is android-fissionship-nightly-experiment-for-qa

Flags: needinfo?(vasilica.mihasca)
Blocks: gv-fission
Whiteboard: [fxdroid]

(In reply to [:owlish] 🦉 PST from comment #28)

Hi Samuel, do you still have this issue? Can you please confirm what is the value of fission.autostart pref in your about:config on your Nightly? Thanks

Yes, I am still experiencing this on both examples pages I've provided.

Yes, fission.autostart is set to true.

Thank you,
Samuel

Flags: needinfo?(sschweinsteiger92)

(In reply to [:owlish] 🦉 PST from comment #29)

Hey Vasilica, can you reproduce the issue if you enrol into the fission experiment that is currently running on Nightly? The experiment slug is android-fissionship-nightly-experiment-for-qa

Investigated a bit more and I confirm that this issue reproduces only when the "Android Fission + SHIP Nightly" nimbus experiment is enabled. Please let me know if I can help with any other details. Thanks!

Flags: needinfo?(vasilica.mihasca)

In addition to the origin actor where the fullscreen request originates, we also
need to notify the parent document, which resides in a different process, to
enter fullscreen mode.

I've managed to reproduce this bug on the geckoview example application as well as fenix running in an emulator:

Steps to reproduce:

I've tested the patch series Implement new Fullscreen API Architecture against this bug and while running in an emulator, I've verified locally that it fixes this bug (as was the point with that work, to not have to chase down these out-of-sync bugs in parallel on multiple platforms).

The only issue I've run into, is the patch series and fenix - a debug assertion in nsPresContext.cpp fires taking the content processess down with, when exiting from fullscreen. When this assertion is commented out, everything works just fine. The geckoview example application doesn't seem to hit this code path, so it never exhibits this behavior.

Yeah, we don't have the dynamic toolbar on GVE, it's very minimal.

Petru, as you have been looking at the dynamic toolbar recently - do you know what the trouble there might be with that assertion and how can we avoid hitting it? Feel free to redirect!

Flags: needinfo?(petru)

Thanks for the ping!
Trying to see what's different between GVE and Fenix when fullscreen changes it looks like GVE doesn't hide the address bar, maybe this is the detail that makes the difference
But I think Hiro will know better why we need that nsPresContext.cpp assertion and if toolbar related changes are needed to support the new Fullscreen API Architecture.

Flags: needinfo?(petru) → needinfo?(hikezoe.birchill)

The assertion is to make sure it's never allowed that the toolbar offset is greater than the toolbar height. For example, if the toolbar height is 100px, then if the offset value is, it's negative basically, less than -100px, then the assertion happens. So I presume there's a call site of the function where the offset is wrong at that moment. It might be a race condition, given that the assertion happens on existing fullscreen.

If I were you, I'd start poking at these call sites which change the offset.

Flags: needinfo?(hikezoe.birchill)

Thank you!
I'll try to see if playing with setVerticalClipping and setDynamicToolbarMaxHeight help avoid that assertion.
Tried today to do a full build but it just errored with multiple similar errors like

/APKOpen.cpp:114:44: error: cannot initialize a parameter of type 'void **' with an rvalue of type 'JNIEnv **' (aka 'JNIEnv_ **')
sJavaVM->AttachCurrentThreadAsDaemon(&env, nullptr) != JNI_OK) {

Will try a bit more.

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

The assertion is to make sure it's never allowed that the toolbar offset is greater than the toolbar height. For example, if the toolbar height is 100px, then if the offset value is, it's negative basically, less than -100px, then the assertion happens. So I presume there's a call site of the function where the offset is wrong at that moment. It might be a race condition, given that the assertion happens on existing fullscreen.

If I were you, I'd start poking at these call sites which change the offset.

Thank you, I think I have enough to investigate this issue further, I think it would be best for everyone if the new fullscreen API architecture (moves to) land asap. :petru and :hiro, is there any place I can reach you guys on matrix for instance, if I have further questions about this directly?

Flags: needinfo?(petru)
Flags: needinfo?(hikezoe.birchill)

#DOM on matrix would be the best channel to talk about this kind of DOM-ish API things. #APZ or #Layout should also work for me about dynamic toolbar related things. I am in these channels basically.

Flags: needinfo?(hikezoe.birchill)

Testing with a local Fenix build with artifacts I see that after exiting fullscreen with having the navbar enabled indeed the offset can be bigger than the height of the toolbar.
I've created bug 1943086 to address that.

@Simon Can you confirm if for the results from comment 33 you had the navbar enabled?
If so I think disabling it from secret settings

  • go to Settings -> About .. -> quickly tap 5 times on the Firefox logo
  • go back to Settings -> Secret Settings -> toggle off "Enable Navigation Toolbar"

should help in testing your patches in Fenix without being impacted by this nsPresContext.cpp assertion.

I'm not using Matrix but if I can help with anything related to Fenix's code you can ping me on slack / NI me here.

Flags: needinfo?(petru)
See Also: → 1943329

Added for bug 1943329 a quick patch to ensure GVE enters in immersive mode for fullscreen videos in the hope that this will help with the investigation, fix and testing of this issue.

(In reply to Petru-Mugurel Lingurar [:petru] from comment #40)

Testing with a local Fenix build with artifacts I see that after exiting fullscreen with having the navbar enabled indeed the offset can be bigger than the height of the toolbar.
I've created bug 1943086 to address that.

@Simon Can you confirm if for the results from comment 33 you had the navbar enabled?
If so I think disabling it from secret settings

  • go to Settings -> About .. -> quickly tap 5 times on the Firefox logo
  • go back to Settings -> Secret Settings -> toggle off "Enable Navigation Toolbar"

should help in testing your patches in Fenix without being impacted by this nsPresContext.cpp assertion.

I'm not using Matrix but if I can help with anything related to Fenix's code you can ping me on slack / NI me here.

I can indeed verify that this stops the assert from firing. I'm struggling to grapple with what that means though. It does seem to suggest that there is a bug, like you say, but it also means that my patch series is the one to expose the bug - does it do that, because my patch series (1913666) has broken some undocumented (or at least to me, unknown) invariant, or is it a "pure bug" in APZ?

Flags: needinfo?(petru)
Pushed by echen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/aad5ffd8f578 Make GeckoView actor handle fullscreen request from OOP iframe properly; r=m_kato,geckoview-reviewers

(In reply to Simon Farre from comment #42)

I can indeed verify that this stops the assert from firing. I'm struggling to grapple with what that means though. It does seem to suggest that there is a bug, like you say, but it also means that my patch series is the one to expose the bug - does it do that, because my patch series (1913666) has broken some undocumented (or at least to me, unknown) invariant, or is it a "pure bug" in APZ?

Thanks for checking and the update!

Your patches shown that Fenix sends wrong values about the dynamic toolbar to APZ.
This was probably not known before and does not cause an actual issue in the application currently because we do multiple updates to APZ with eventually correct details:

  • when exiting fullscreen we reinitialize / relayout the dynamic toolbar and update APZ with related details - these might be wrong if the navbar layout is not complete.
  • very soon after the new layout is complete and we update again the dynamic toolbar details in APZ with the correct details
  • we also update APZ with the dynamic toolbar details everytime something changes in the layout (any other layout update/scroll)

So for a very brief moment APZ will have incorrect details - and this nsPresContext.cpp assertion will fail but very soon after we'd be sending the "right" details and everything should work nicely.
Based on comment 33 I understand a graceful handling of this incorrect, then correct data is not possible - the content process crashes - so I'll put on a patch for bug 1943086 to prevent Fenix sending the wrong data.

Flags: needinfo?(petru)

Your patches shown that Fenix sends wrong values about the dynamic toolbar to APZ.
This was probably not known before and does not cause an actual issue in the application currently because we do multiple updates to APZ with eventually correct details:

  • when exiting fullscreen we reinitialize / relayout the dynamic toolbar and update APZ with related details - these might be wrong if the navbar layout is not complete.
  • very soon after the new layout is complete and we update again the dynamic toolbar details in APZ with the correct details
  • we also update APZ with the dynamic toolbar details everytime something changes in the layout (any other layout update/scroll)

Thanks for the in-depth explaination!

So for a very brief moment APZ will have incorrect details - and this nsPresContext.cpp assertion will fail but very soon after we'd be sending the "right" details and everything should work nicely.
Based on comment 33 I understand a graceful handling of this incorrect, then correct data is not possible - the content process crashes - so I'll put on a patch for bug 1943086 to prevent Fenix sending the wrong data.

Ok, cool!

Just a nit, I guess; since it's a MOZ_ASSERT, it would in release mode actually not take the content process down with it, so there it would gracefully handle it (by just "behaving as normal"). Do you think your patch will solve the issues for 1913666 as well?

thanks!

Flags: needinfo?(petru)

(In reply to Simon Farre from comment #45)

Just a nit, I guess; since it's a MOZ_ASSERT, it would in release mode actually not take the content process down with it, so there it would gracefully handle it (by just "behaving as normal").

Good catch! Not knowing fully how these toolbar height/offset values are used by APZ I could not say for sure but should be quick to test in an official local build or from try.

 

(In reply to Simon Farre from comment #45)

Do you think your patch will solve the issues for 1913666 as well?

If going by comment 33 the Fenix patch from bug 1943086 should ensure that the fullscreen patch from bug 1913666 will fix this issue in Fenix.
Agree that bug 1943086 should block bug 1943955 to enable the new Fullscreen API Architecture.

Thank you for organizing all the related work, looks like we're set to fix this and overall improve the fullscreen functionality.

Flags: needinfo?(petru)

https://hg.mozilla.org/mozilla-central/rev/aad5ffd8f578

Status: NEW → RESOLVED
Closed: 1 year ago
status-firefox136: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 136 Branch

Note that even though the assertion doesn't cause any crashes on release builds, there would be visual glitches or some thing like that since if we received incorrect values there, dvh units will be broken, the visual viewport size would be also incorrect.

The patch landed in nightly and beta is affected.
:edgar, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox135 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(echen)

In addition to the origin actor where the fullscreen request originates, we also
need to notify the parent document, which resides in a different process, to
enter fullscreen mode.

Original Revision: https://phabricator.services.mozilla.com/D234733

Attachment #9462716 - Flags: approval-mozilla-beta?

beta Uplift Approval Request

  • User impact if declined: vidoe in cross-origin iframe can not enter into fullscreen mode on Anroid when fission is enabled
  • Code covered by automated testing: yes
  • Fix verified in Nightly: yes
  • Needs manual QE test: no
  • Steps to reproduce for manual QE testing: None
  • Risk associated with taking this patch: Medium
  • Explanation of risk level: Patch changes the fullscreen IPC flow on Android for Fission case, but it should not affect the non-Fission case.
  • String changes made/needed: None
  • Is Android affected?: yes
Flags: needinfo?(echen)

I just told ryanvm that the change is kinda big so it would be nice to bake on nightly for a while, but Edgar is convinced that it's not so risky, I am okay. :)

I understand that this is an ugly bug that we'd want fixed asap.
It's also marked as depending on bug 1943086, and I'm not sure whether that will be merged in time for it to also be uplifted.
However, as discussed above, some visual glitches in webpage rendering could appear right after exiting fullscreen videos to portrait mode with the navigation bar feature enabled. These should however be immediately resolved by subsequent APZ updates that Fenix requests after exiting fullscreen and settling it's UI.

Based on these values web content might not be properly anchored to the bottom <-- these are the talked about glitches.
But sending wrong values then immediately after the right ones is not a new issue, it's been there from when we added the navbar.
And it doesn't result in a bug for users / in production because after finishing exiting fullscreen and settling the UI Fenix updates again APZ with the "right" values.
So yes, sending wrong probably result in glitches - miss aligned content.
But realistically this does not seem to happen - shown in the video from Youtube where it's bottom bar is correctly aligned to Fenix's bottom toolbar.

(I don't have strong opinion whether we should release this fix to 135. Since this affects Android only, I will let Android team to decide.)

FYI I just filed bug 1944834 that shows that it's not always 100% correct. It's still so much better than what we had before where full screen videos weren't working at all with this STR.

(padenot asked me to needinfo edgar)

Flags: needinfo?(echen)

My understanding is that this bug requires Fission to be enabled in order to reproduce. That seems to pretty significantly mitigate the risk of users hitting this in the wild and warrants letting this fix bake longer before we consider any uplifts.

Attachment #9462716 - Flags: approval-mozilla-release?
Attachment #9462716 - Flags: approval-mozilla-beta?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #57)

My understanding is that this bug requires Fission to be enabled in order to reproduce. That seems to pretty significantly mitigate the risk of users hitting this in the wild and warrants letting this fix bake longer before we consider any uplifts.

(In reply to Edgar Chen [:edgar] from comment #54)

(I don't have strong opinion whether we should release this fix to 135. Since this affects Android only, I will let Android team to decide.)

There has been a great effort and collaboration to get this but if Fission is needed to reproduce / force this larger bug I think we can check with the team handling the Fission rollout to have it postponed just a bit more in production while we assess all the implications of the new fullscreen api through indeed more time in Nightly.

Flags: needinfo?(bugzeeeeee)
Flags: needinfo?(echen)

AFAICT, we're not doing any Fission rollouts to Release 135, so it doesn't seem like this is worth backporting for next week's planned dot release. Let me know if I'm missing something, however.

status-firefox135: affected → wontfix
Attachment #9462716 - Attachment is obsolete: true
Attachment #9462716 - Flags: approval-mozilla-release? → approval-mozilla-release-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: