1889017 - Video controls at the bottom cannot be hit when viewing video in fullscreen mode in landscape orientation (affects Rotten Tomatoes, YouTube)

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

Found while playing around with the STR of bug 1875589 on an Android tablet (Samsung Galaxy Tab A9+).

Steps to reproduce

1. Orient the tablet in landscape mode
2. Load https://www.rottentomatoes.com/
3. Request desktop site
4. Tap a video in the "Something for everyone" section to play it
5. Double-tap the video to put it into fullscreen mode
6. Tap the "exit fullscreen" mode (at the right end of the bottom bar of video controls)

Expected results

The video exits fullscreen mode.

Actual results

The video pauses.

It appears as though we fail to hit the exit-fullscreen button, and instead the event targets the video element itself (where it's handled as play/pause).

Comment 1

[Botond Ballo [:botond]]

A workaround exists in this case (you can double-tap the video again to exit fullscreen mode), but the inability to successfully hit an element in the bottom right corner of the viewport seems concerning (one can imagine an underlying issue that potentially causes an S2 bug on a different website). Adding to APZ diagnosis backlog.

Comment 2

[Botond Ballo [:botond]]

This affects Youtube as well:

Steps to reproduce

1. Orient the tablet in landscape mode
2. Load https://www.youtube.com/watch?v=6Y8FVIiWyao
3. Request desktop site
4. Tap the button at the bottom right of the video to enter fullscreen mode
5. Try to tap the same button again to exit fullscreen mode

Expected results

The video exits fullscreen mode.

Actual results

The video plays or pauses.

Unlike on Rotten Tomatoes, double-tapping the video to exit full screen mode does not work as a workaround. Rotating the table to portrait mode does not seem to work either (the page remains in landscape orientation even after the rotation).

I have found some other workarounds for getting out of fullscreen mode but they are less discoverable:

• Turn the tablet's display off and back on (which shows the lock screen) and unlock it. This causes Firefox to leave fullscreen mode.
• With the table physically oriented in portrait mode, activate the application switcher. This seems to force Firefox to change orientation to portrait mode.

Comment 4

[Botond Ballo [:botond]]

It's actually worse than just the "exit fullscreen" button not working: none of the buttons in the bottom bar work (tapping in that area always has the effect of palying/pausing the video, suggesting the event targets the video element itself).

(Previously, I tested the first button, which is play/pause, and thought it was working because the video did indeed play or pause. However, now that I check the other buttons and the effect is always play/pause, it's clear that the first button was also only "working" via the event targeting the video element.)

Comment 6

[Botond Ballo [:botond]]

Interesting observation made in bug 1884589 comment 1:

This bug is only triggered when the tablet is in landscape mode. [...] If the video is in portrait mode at first, and then clicked to make the video full screen, the automatic horizontal screen will not trigger this bug.

Comment 7

[Mr. Beedell, Roke Julian Lockhart]

https://bugzilla.mozilla.org/show_bug.cgi?id=1889017#c6

[e-mail], does it occur in vertical videos, like https://youtu.be/Wj2es4JH8g4?si=tOVRXVRBjMbJIXiZ&t=0?

Comment 8

[Botond Ballo [:botond]]

(In reply to {third: "Beedell", first: "Roke"}{.JSON5} from comment #7)

does it occur in vertical videos, like https://youtu.be/Wj2es4JH8g4?si=tOVRXVRBjMbJIXiZ&t=0?

If the tablet is in the portrait orientation (in which case, for this video the page contents also remain in portrait orientation in fullscreen mode), the buttons work fine.

If the tablet is in the landscape orientation, the bug occurs as with other videos.

Comment 9

[Botond Ballo [:botond]]

We discussed this in our planning meeting. Bumping severity to S2 because the only workarounds we're aware of are not very discoverable.

(We're not sure if this is an APZ issue, but will start by ruling out APZ issues and then move components as appropriate.)

Comment 10

[Botond Ballo [:botond]]

I've been thinking about how to investigate this.

I think a starting point might be to reproduce the issue with event.retarget logging enabled. This will log the touch events that arrive at the content process when you tap, including their:

1. coordinates
2. initial event target (based on their coordinates)
3. final (re-targeted) event target

(The frame tree, dumped here only at the verbose logging level (level 5), is particularly helpful to understand where in the document the elements targeted at steps 2 and 3 are.)

We can then correlate this information with the document structure viewed in the Inspector using about:debugging to understand at which point the observed results start to diverge from expectations.

Comment 11

[Botond Ballo [:botond]]

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

(The frame tree, dumped here only at the verbose logging level (level 5), is particularly helpful to understand where in the document the elements targeted at steps 2 and 3 are.)

An important addition here is that this does not work in a release or nightly build, only in a local build built with either --enable-debug or --enable-dump-painting.

Comment 12

[Botond Ballo [:botond]]

I've tried getting a frame dump on rottentomatoes.com in a build with --enable-dump-painting, but the frame dump is so large that it takes 5+ seconds just to print it. Under those circumstances, I haven't been able to trigger the STR (the script on the page seems to become unresponsive due to the high latency of event processing).

Comment 13

[Botond Ballo [:botond]]

Attachment: event.retarget logging including frame dump (rottentomatoes.com)

With some hacks in place to only get the frame dump to be printed for the event of interest, I managed to get event.retarget logging including a frame dump. I haven't analyzed the logs yet but I'm attaching them for reference.

Comment 14

[Botond Ballo [:botond]]

Some observations from the log:

• There is no re-targeting of the event happening here (the final target is the same as the initial target). However, the initial target is itself incorrect.
• The target frame is Block(div)(0)@7024a79908. This is a descendant of the HTMLVideo frame (presumably in the <video> element's anonymous subtree).
• The frame that should be targeted is FlexContainer(div)(15)@7024a576c8 (or a descendant of it). This is the frame for the <div class="jw-icon-fullscreen ..."> element (the "exit fullscreen" button). This is a descendant of a <div class="jw-controls ..."> which is a later sibling of <div class="jw-media ..."> which contains the <video> element. Basically, <div class="jw-controls"> is an overlay over the video.
• The overlay has pointer-events: none, but its descendant <div clas="jw-controlbar">, which only occupies the bottom row, overrides that with pointer-events: all. This is how events over the bottom bar (should) target elements inside that bottom bar, while events above the bottom bar go "through" the overlay and hit the video element underneath.
• On my device, the height of the viewport in fullscreen mode is 720 CSS pixels. The bottom bar occupies the bottom 60 pixels, i.e. it starts at y=660.
• The coordinates of the touchstart event captured in the log are (51542,31917) in app units relative to the viewport frame. This is (859, 532) in CSS pixels.
  • These coordinates seem to be incorrect. Note in particular the y=532, when the bottom bar starts y=660. (The x-coordinate is also too small, given the width of the viewport is 1152 CSS pixels and the exit-fullscreen button is at the end of the bottom bar.) These incorrect coordinates explain why the bottom bar is not being hit: the event instead hits the part of the overlay above the bottom bar, and falls through to the video element underneath.

Comment 15

[Botond Ballo [:botond]]

The event coordinates look correct in the parent process. In the content process, they seem to be scaled by an unexpected factor, as though we think we're pinch-zoomed in when we're actually not.

This scaling only happens when the video is in fullscreen mode, and only when the page is in desktop mode.

I'm removing the [apz-needsdiagnosis] tag because based on the above findings, this seems like it is indeed an APZ issue.

Comment 16

[Botond Ballo [:botond]]

Looking at various metrics on my tablet, one thing I see that's unusual is that there is a full zoom of 1.11, in addition to a widget scale of 1.5, for a total css to device scale of 1.67. I think some codepaths may not be expecting this.

Comment 17

[Botond Ballo [:botond]]

The full zoom of 1.11 appears to be coming from here, and can be disabled by setting browser.display.os-zoom-behavior=0 (so that now the css to device pixel scale is 1.5). However, the bug still occurs, so this is not the cause.

Comment 18

[Botond Ballo [:botond]]

The reason the input coordinates are transformed as though we were pinch-zoomed in is that we are pinch-zoomed in, as far as the state stored in PresShell::mResolution is concerned.

When the page is in desktop mode but the video is not yet fullscreen, the CSS viewport (initial containing block) width is 980 CSS pixels (that's our desktop-mode viewport width), and the content width of the page when reflowed into that ICB is 1100 CSS pixels. This does not fill the screen width at unit resolution (1920 device pixels / 1.5 device pixel scale = 1280 > 1100), so we render at a resolution of 1.16 to scale up the contents to fit the screen width.

When we enter fullscreen mode:

• the CSS viewport width is recomputed to fit the screen at unit resolution (i.e. in this case to 1280 CSS pixels), by taking this branch in Document::GetViewportInfo() rather than this branch
  • this behaviour was introduced in bug 1696717
• the resolution is updated in Document::ApplyFullscreen()

The intention of the code in ApplyFullscreen() is to update the resolution to the "intrinsic resolution", which is the resolution that makes the ICB width fit the screen width. The code pre-dates bug 1696717, and so doesn't assume that this resolution is necessarily 1 (prior to bug 1696717, we would have kept the ICB width at 980px in fullscreen mode, and required the resolution to be at e.g. 1.31 to fit the screen).

The bug occurs by running into the following failure mode (which happens most of the time): ApplyFullscreen() fails to compute the correct intrinsic resolution, because it reads the ICB size from MobileViewportManager::mMobileViewportSize before that has been updated to reflect the new ICB size in fullscreen mode (i.e. it's still at 980px, not 1280px). We therefore set the resolution to an unintended value (such as 1.31).

A tweak to ComputeIntrinsicResolution() to use the up-to-date GetViewportInfo().GetSize() rather than the stored mMobileViewportSize fixes the bug for me locally.

One thing I do not yet understand is: if the resolution is 1.31 when the video is in fullscreen mode and we have reflowed the page into a 1280 CSS pixel ICB, why isn't the video visually zoomed in (such that its right and bottom parts, including the bottom bar, are offscreen)? That's what I would expect the rendering to be in this state (and if that were the rendering, the underlying bug would be much more obvious from the get-go).

Comment 19

[Botond Ballo [:botond]]

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

One thing I do not yet understand is: if the resolution is 1.31 when the video is in fullscreen mode and we have reflowed the page into a 1280 CSS pixel ICB, why isn't the video visually zoomed in (such that its right and bottom parts, including the bottom bar, are offscreen)? That's what I would expect the rendering to be in this state (and if that were the rendering, the underlying bug would be much more obvious from the get-go).

The reason for this is that we skip building an async zoom container if the document is in fullscreen mode. This was also introduced in bug 1696717.

It's a bit of a footgun that there exists a set of conditions under which we ignore PresShell::mResolution for rendering purposes (by not building an async zoom container), but still use it for hit-testing purposes. We may want to consider at least detecting and asserting if that ever happens when mResolution != 1.0. (Such an assertion would have caught this bug and made the problem obvious.)

Comment 20

[Botond Ballo [:botond]]

Here is a minimal testcase demonstrating the bug: https://theres-waldo.github.io/mozilla-testcases/bug1889017.html

The STR for it is:

1. Load the testcase
2. Orient the tablet in landscape mode
3. Request desktop site
4. Tap the yellow element to enter fullscreen mode
5. Try to tap the green element to exit fullscreen mode

The bug also reproduces on a phone with the pref browser.viewport.desktopWidth set to a sufficiently small value. (It needs to be smaller than the width of the screen in landscape mode divided by the device scale; for example, on a phone with a screen height of 1920px and a device scale of 3.0, the pref value needs to be smaller than 1920 / 3 = 640.)

It does not reproduce 100% of the time but it does fairly often.

Comment 21

[Botond Ballo [:botond]]

Attachment: Bug 1889017 - MobileViewportManager logging improvements. r=hiro

The log statement in ComputeIntrinsicScale() was quite noisy because one
of its callers was called on every paint. The patch moves it to the
non-noisy call sites, and details whether the intrinsic scale is being
computed based on the viewport size or content size.

Also add a couple of new log statements.

Comment 22

[Botond Ballo [:botond]]

Attachment: Bug 1889017 - Use the up-to-date CSS viewport size in ComputeIntrinsicResolution(). r=hiro

Depends on D216902

Comment 23

[Botond Ballo [:botond]]

Attachment: Bug 1889017 - Add a mochitest. r=hiro

Depends on D216902

Comment 24

[Botond Ballo [:botond]]

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

It's a bit of a footgun that there exists a set of conditions under which we ignore PresShell::mResolution for rendering purposes (by not building an async zoom container), but still use it for hit-testing purposes. We may want to consider at least detecting and asserting if that ever happens when mResolution != 1.0. (Such an assertion would have caught this bug and made the problem obvious.)

Filed bug 1915051 for this.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07653767b63f
MobileViewportManager logging improvements. r=hiro
https://hg.mozilla.org/integration/autoland/rev/2598d4f8fc98
Add a mochitest. r=hiro
https://hg.mozilla.org/integration/autoland/rev/2856adb4fd44
Use the up-to-date CSS viewport size in ComputeIntrinsicResolution(). r=hiro

Comment 26

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/07653767b63f
https://hg.mozilla.org/mozilla-central/rev/2598d4f8fc98
https://hg.mozilla.org/mozilla-central/rev/2856adb4fd44