Closed Bug 1718012 Opened 4 years ago Closed 4 years ago

Zoom out and triggering full screen will break DOM rendering

Categories

(Core :: Panning and Zooming, defect)

Product:
Core
Component:
Panning and Zooming
Version:
Firefox 89
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
92 Branch
Tracking Flags:
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- wontfix
firefox89 --- wontfix
firefox90 --- wontfix
firefox91 --- wontfix
firefox92 --- fixed

People

(Reporter: meefox, Assigned: hiro)

References

(Regressed 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Screenshot 2021-06-24 at 10.51.30.png
3.27 MB, image/png
Details
Bug 1718012 - Call UpdateSizesBeforeReflow only for resize reflow cases and set MobileViewportManager::mMobileViewportSize even if mDisplaySize hasn't been changed in UpdateSizesBeforeReflow. r?tnikkel
48 bytes, text/x-phabricator-request
Details | Review

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.77 Safari/537.36

Steps to reproduce:

1 go to youtube (just easier)
2 zoom out using cmd + -
3 trigger full screen video

Actual results:

4 observe horizontal/vertical scrolls bars (scrollable)

Expected results:

4 no horizontal/vertical scrolls bars

I could not reproduce this issue on macOS Big Sur 11.4 using the latest Nightly 91.0a1 or the latest Firefox 89.0.2.

Meefox, could you please create a screen recording that exemplifies your issue? Also, to eliminate certain custom settings or the influence of some add-ons, could you please check if the issue is reproducible when opening Firefox in safe mode?
http://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode

Yes it is reproducible on 2 of my computers with fresh Firefox and as well in troubleshooting mode.

video: https://www.dropbox.com/s/sfr69bb6dxzajed/ff-bug.mp4?dl=0

Meefox, thanks for the screencast, I'm setting this issue to New as I was able to reproduce it.

Narrowed regression window to:
Last good revision: 55ecb48a05043c97f77dcfcb1e3ed4c66671523f
39:52.46 INFO: First bad revision: 9505095bc965807c39cf71f27c3eba4a9e6fa9b2
39:52.46 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=55ecb48a05043c97f77dcfcb1e3ed4c66671523f&tochange=9505095bc965807c39cf71f27c3eba4a9e6fa9b2

Severity: -- → S3
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox89: --- → affected
status-firefox90: --- → affected
status-firefox91: --- → affected
status-firefox-esr78: --- → unaffected
Component: Untriaged → Panning and Zooming
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
Regressed by: 1647034
Flags: needinfo?(hikezoe.birchill)

What I've found so far are;

  1. It doesn't often happen on my Linux box, it's actually hard to reproduce, I haven't found any clues to reproduce this issue reliably
  2. It does happen both on WebRender/non-WebRender
  3. The fullscreen element on Youtube is the top level <html> element, so it's not position:fixed, it's not -moz-top-layer

Presumably there's a long standing issue for fullscreen-ed html element that bug 1647034 revealed.

This was indeed regressed by bug 1647034, specifically this early return.

From the change set;

+void MobileViewportManager::UpdateSizesBeforeReflow() {
   if (Maybe<LayoutDeviceIntSize> newDisplaySize =
           mContext->GetContentViewerSize()) {
-    // Note that we intentionally don't short-circuit here if the display size
-    // (in LD units) is unchanged, because a resize-reflow may also be triggered
-    // by a change in the CSS/LD pixel ratio which would affect GetZoom() and
-    // therefore the computed visual viewport.
+    if (mDisplaySize == *newDisplaySize) {
+      return;
+    }
+

Reflowing triggered by full-zoom doesn't change the display size, but it needs to change MobileViewportManager::mMobileViewportSize since the mMobileViewportSize is in CSS units, it's affected by the full-zoom value.

Flags: needinfo?(hikezoe.birchill)
Assignee: nobody → hikezoe.birchill
Status: NEW → ASSIGNED

In the case of reflowing caused by full zoom changes, the mDisplaySize isn't
changed, whereas mMobileViewportSize should be changed.

Attachment #9231842 - Attachment description: Bug 1718012 - Set MobileViewportManager::mMobileViewportSize even if mDisplaySize hasn't been changed. r?tnikkel → Bug 1718012 - Call UpdateSizesBeforeReflow only for resize reflow cases and set MobileViewportManager::mMobileViewportSize even if mDisplaySize hasn't been changed in UpdateSizesBeforeReflow. r?tnikkel
Pushed by hikezoe.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1b29d7a6d15c Call UpdateSizesBeforeReflow only for resize reflow cases and set MobileViewportManager::mMobileViewportSize even if mDisplaySize hasn't been changed in UpdateSizesBeforeReflow. r=tnikkel

Hmm, ./mach try run didn't run those tests unfortunately...

Looks like we also need to call the UpdateSizesBeforeReflow before the initial reflow.

Flags: needinfo?(hikezoe.birchill)
Pushed by hikezoe.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/21ac9225dd4b Call UpdateSizesBeforeReflow only for resize reflow cases and set MobileViewportManager::mMobileViewportSize even if mDisplaySize hasn't been changed in UpdateSizesBeforeReflow. r=tnikkel

Four unexpected PASS and one failure.

The failure is

[task 2021-07-28T21:31:00.660Z] 21:31:00     INFO - TEST-UNEXPECTED-FAIL | /css/css-values/viewport-units-css2-001.html | vh length applied to border-top-width - assert_equals: expected 114 but got 113

Looks like a precision issue?

(Though I didn't expect that the change makes some tests pass)

Okay, the viewport-units-css2-001.html has been already marked as intermittent on Win and Mac. Now the behavior on Android matches them, I believe it's a good sign. I will modify the annotation.

Flags: needinfo?(hikezoe.birchill)

There are more unexpected-PASS on Android; https://treeherder.mozilla.org/jobs?repo=try&author=hikezoe.birchill%40mozilla.com&selectedTaskRun=bFgKKRA4SbSTsA4KN0P2hw.0

I did trigger all wpt tests there to see there's still similar tests.

Note that the reason why the change makes some tests pass is, I am assuming, with the change the test cases are properly layout in the first place, thus it fixed race conditions on Android.

I've modify all UNEXPECTED-PASS test cases. Also note that there's an unexpected failure with the original failure, that's disable-fontinfl-on-mobile.html. The failure reason was that if we call Document::GetViewportInfo before parsing <!DOCTYPE> we can't properly handle old fashion mobile cases. Emilio told me on Matrix that Document::SetCompatibilityMode should get called when a doctype is set, so I just added mViewportType = Unknown in Document::SetCompatibilityMode to make sure we re-evaluate the viewport type even if we called GetViewportInfo before parsing the doctype.

Pushed by hikezoe.birchill@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/295399ab06a8 Call UpdateSizesBeforeReflow only for resize reflow cases and set MobileViewportManager::mMobileViewportSize even if mDisplaySize hasn't been changed in UpdateSizesBeforeReflow. r=tnikkel

https://hg.mozilla.org/mozilla-central/rev/295399ab06a8

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox92: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 92 Branch
status-firefox-esr91: --- → wontfix
Flags: in-testsuite+
Regressions: 1780559
Regressions: 1803631
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: