Closed Bug 1598145 Opened 5 years ago Closed 5 years ago

longines.com does not adjust to the screen size in portrait mode

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla77
Webcompat Priority ?
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- wontfix
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- fixed

People

(Reporter: ksenia, Assigned: bradwerth)

References

(Blocks 1 open bug, Regression, )

Details

(Keywords: regression)

Attachments

(5 files, 6 obsolete files)

626 bytes, text/html
Details
319 bytes, text/html
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

As reported in https://github.com/webcompat/web-bugs/issues/44650

Steps to reproduce:

  1. Visit https://www.longines.com/ in Firefox Preview / Preview Nightly and observe the site

Expected:
Site adjusts to the screen size

Actual:
Site does not adjust to the screen size in portrait mode

If I rotate my phone to landscape and then back to portrait mode, then it starts working.

From mozregression:

19:53.31 INFO: Narrowed inbound regression window from [f4137f96, cc691c0e] (3 builds) to [f7eafb0d, cc691c0e] (2 builds) (~1 steps left)
19:53.31 INFO: No more inbound revisions, bisection finished.
19:53.31 INFO: Last good revision: f7eafb0d4ad6afd7eeb0b02a6db892ff97aa8163
19:53.31 INFO: First bad revision: cc691c0e8775f8b6c24b9e641a164e8d82afa924
19:53.31 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f7eafb0d4ad6afd7eeb0b02a6db892ff97aa8163&tochange=cc691c0e8775f8b6c24b9e641a164e8d82afa924

Brad, could you please take a look?

Flags: needinfo?(bwerth)
Priority: -- → P2

Based on the regression range, 70 and later are affected.

(In reply to Ksenia Berezina [:ksenia] from comment #1)

Brad, could you please take a look?

I'm sorry it's taken me so long to investigate. I'm able to replicate the bug and I'll see what I can do about modifying the changes made in Bug 1523844 to make sites like this render correctly.

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

It seems that part of what makes longines.com different is that the first call to MobileViewportManager::UpdateResolution is triggered by a JavaScript call to getClientRects. I don't know the JS callstack. That seems unusual, or at least not anticipated by the logic in MVM. That does -- I think -- explain why it behaves differently in the Responsive Design Mode, because that viewport has already been laid out before this JS call would be triggered.

(In reply to Brad Werth [:bradwerth] from comment #5)

I don't know the JS callstack.

Note, you can get a JS callstack by adding a call to DumpJSStack() to UpdateResolution(). (You may need to add a forward declaration of that function to MobileViewportManager.cpp for the call to compile.) You can also call DumpJSStack() directly from the debugger if you're stopped at a breakpoint at the place of interest.

Getting a bit further. I've discovered that the bug is reproducible in Responsive Design Mode after a reload. So initial view is correct resolution, reload is shrunken as it is on GeckoView. The problematic resolutions are coming through scripted changes to the meta tag. It's in minified JS code:

1 X(e = "[object Object]", t = [function], i = ""content"", n = ""width=320, user-scalable=no"", s = "true") ["https://www.longines.com/js/minify-nomagento.js":1:36384]

That fires a DOM_META_CHANGED event which reaches MVM::RefreshViewportSize. The first call to UpdateResolution in that function is for the viewport size, and it's setting a reasonable resolution. The second call to UpdateResolution comes from the later call to ShrinkToDisplaySizeIfNeeded, and it sets the smaller resolution -- which is the intrinsic scale calculated at that point. The reason why this calling pattern worked before Bug 1523844 landed is that once mRestoreResolution is set, then UpdateResolution no longer snaps to the intrinsic scale for content updates. But the core issue is that the intrinsic scale computation is shrunken at the point of this final update.

Attachment #9115563 - Attachment is obsolete: true

Currently, when a content update specifies a large scrollable rect,
if the MVM does not already have a restore resolution, the resolution
will be reduced to fit at least one axis of the scrollable rect
within the viewport. That seems to be a not useful outcome, given
that the scrollable rect is scrollable and doesn't need to be shown
in its entirety. This change makes a MVM resolution change from a
viewport update also serve as the restore resolution, and makes that
decision independent of whether or not the viewport has been painted.

Attachment #9116907 - Attachment is obsolete: true

Currently, when a content update specifies a large scrollable rect,
if the MVM does not already have a restore resolution, the resolution
will be reduced to fit at least one axis of the scrollable rect
within the viewport. That seems to be a not useful outcome, given
that the scrollable rect is scrollable and doesn't need to be shown
in its entirety. This change makes a forced MVM resolution change
for a viewport update also serve as the restore resolution. Forced
resolution changes happen when the viewport has already been painted
or when a meta viewport tag is added or changed.

I have a new insight into what is going on with longines.com. The scrollable rect and intrinsic scale are being calculated consistently in all cases. There's no difference in these values between the tiny and intended display states. The site appears tiny because its scrollable rect is much wider than the viewport and the content is being shrunk to fit the viewport. The only thing that was preventing this from happening -- before Bug 1523844 landed -- was that the restore resolution was being set. There is no reason for the restore resolution to be set from simply loading the page. Before Bug 1523844 landed, the restore resolution was set during page load.

There are other things that can set the restore resolution and therefore cause the site to appear as intended. If you reproduce this bug in Responsive Design Mode, you can get it to trigger by loading the page, toggling on RDM, and then reloading the page. In this state, restore resolution has not been set. Now, if you scroll the page vertically, and then reload it, it will appear at its intended resolution. What's going on? Well, during layout, we hit this stack:

#0 MobileViewportManager::SetRestoreResolution(float) at layout/base/MobileViewportManager.cpp:741
#1 MobileViewportManager::ResolutionUpdated(mozilla::ResolutionChangeOrigin) at layout/base/MobileViewportManager.cpp:120
#2 mozilla::PresShell::SetResolutionAndScaleTo(float, mozilla::ResolutionChangeOrigin) at layout/base/PresShell.cpp:5138
#3 mozilla::ScrollFrameHelper::RestoreState(mozilla::PresState*) at layout/generic/nsGfxScrollFrame.cpp:6601
#4 nsHTMLScrollFrame::RestoreState(mozilla::PresState*) at layout/generic/nsGfxScrollFrame.h:1071
#5 nsFrameManager::RestoreFrameStateFor(nsIFrame*, nsILayoutHistoryState*) at layout/base/nsFrameManager.cpp:222
#6 nsCSSFrameConstructor::InitAndRestoreFrame(nsFrameConstructorState const&, nsIContent*, nsContainerFrame*, nsIFrame*, bool) at layout/base/nsCSSFrameConstructor.cpp:4618
#7 nsCSSFrameConstructor::BeginBuildingScrollFrame(nsFrameConstructorState&, nsIContent*, mozilla::ComputedStyle*, nsContainerFrame*, mozilla::PseudoStyleType, bool, nsContainerFrame*&) at layout/base/nsCSSFrameConstructor.cpp:4171
#8 nsCSSFrameConstructor::SetUpDocElementContainingBlock(nsIContent*) at layout/base/nsCSSFrameConstructor.cpp:2602
#9 nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*) at layout/base/nsCSSFrameConstructor.cpp:2164
#10 nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsILayoutHistoryState*, nsCSSFrameConstructor::InsertionKind) at layout/base/nsCSSFrameConstructor.cpp:6971
#11 nsCSSFrameConstructor::ContentInserted(nsIContent*, nsILayoutHistoryState*, nsCSSFrameConstructor::InsertionKind) at layout/base/nsCSSFrameConstructor.cpp:6888
#12 mozilla::PresShell::Initialize() at layout/base/PresShell.cpp:1738
#13 nsContentSink::StartLayout(bool) at dom/base/nsContentSink.cpp:1142

And the resolution that is set during that call is the tiny resolution. This is the resolution that makes the site look bad. Setting any restore resolution, even NaN, will make the page appear as intended due to the MVM::UpdateResolution ContentType logic.

So I see three options:

  1. Treat the behavior as correct, and fix RDM to show the tiny version on first load. The page just does have a large scrollable rect, due to unknown CSS styling issues (probably overflow styles.) We can chase down these styling issues further and propose a change to the page, but the idea that a site that intended to have a large scrollable rect would not shrink to fit content if the user took an action to set a restore resolution feels very fragile.
  2. Make a carveout to set a restore resolution during some step hit during loading of the longines page. This is what the proposed patch does. It's not very satisfying. It's particularly unsatisfying that the actual restore resolution doesn't matter.
  3. Change the MVM::UpdateResolution ContentType update logic to be less aggressive and not shrink in as many cases. Perhaps the logic should consider the value of the restore resolution, and whether or not the ViewportType update adopted that value. I'll think on this some more and attempt to build a patch to address this.

Happy to hear any other proposals on how to solve this bug.

(In reply to Brad Werth [:bradwerth] from comment #12)

Happy to hear any other proposals on how to solve this bug.

Do we have a resolution to the question that came up during review:

Do we have an understanding of why MVM sees the page's scrollable rect as being so large? I opened the page in RDM, refreshed to trigger the shrunken rendering as described in the bug comments, and then printed document.scrollingElement.scrollWidth -- and it printed 320, the desired narrower width. If MVM is picking up a value different from document.scrollingElement.scrollWidth, I think we should investigate why.

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

Do we have an understanding of why MVM sees the page's scrollable rect as being so large? I opened the page in RDM, refreshed to trigger the shrunken rendering as described in the bug comments, and then printed document.scrollingElement.scrollWidth -- and it printed 320, the desired narrower width. If MVM is picking up a value different from document.scrollingElement.scrollWidth, I think we should investigate why.

I don't have a good understanding of this. Here's what I know:

The large content size is calculated in MVM::ShrinkToDisplaySize, which goes through nsLayoutUtils::CalculateScrollableRectForFrame and grabs a stored overflow rect in ScrollFrameHelper::GetScrolledRect. The large overflow rect is set via:

#0 nsIFrame::SetOverflowAreas(nsOverflowAreas const&) at layout/generic/nsFrame.cpp:9185
#1 nsIFrame::FinishAndStoreOverflow(nsOverflowAreas&, nsSize, nsSize*, nsStyleDisplay const*) at layout/generic/nsFrame.cpp:9632
#2 nsHTMLScrollFrame::PlaceScrollArea(mozilla::ScrollReflowInput&, nsPoint const&) at layout/generic/nsGfxScrollFrame.cpp:872
#3 nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) at layout/generic/nsGfxScrollFrame.cpp:1160
#4 nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, nsIFrame::ReflowChildFlags, nsReflowStatus&, nsOverflowContinuationTracker*) at layout/generic/nsContainerFrame.cpp:948
#5 mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) at layout/generic/ViewportFrame.cpp:299
#6 mozilla::PresShell::DoReflow(nsIFrame*, bool, mozilla::OverflowChangedTracker*) at layout/base/PresShell.cpp:9159
#7 mozilla::PresShell::ProcessReflowCommands(bool) at layout/base/PresShell.cpp:9332
#8 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) at layout/base/PresShell.cpp:4114
#9 mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) at dist/include/mozilla/PresShell.h:1452
#10 mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) at dom/base/Document.cpp:9969
#11 mozilla::dom::Document::FlushPendingNotifications(mozilla::FlushType) at dom/base/Document.cpp:9890
#12 mozilla::PresShell::SimpleResizeReflow(int, int, mozilla::ResizeReflowOptions) at layout/base/PresShell.cpp:1866
#13 mozilla::PresShell::ResizeReflowIgnoreOverride(int, int, mozilla::ResizeReflowOptions) at layout/base/PresShell.cpp:1894
#14 mozilla::GeckoMVMContext::Reflow(mozilla::gfx::SizeTyped<mozilla::CSSPixel, float> const&) at layout/base/GeckoMVMContext.cpp:195
#15 MobileViewportManager::RefreshViewportSize(bool) at layout/base/MobileViewportManager.cpp:619
#16 MobileViewportManager::HandleEvent(mozilla::dom::Event*) at layout/base/MobileViewportManager.cpp:134

I don't know why it has such a large overflow rect value. I also can't tell you why document.scrollingElement.scrollWidth is not returning the same value. The call to Element::ScrollWidth does go through ScrollFrameHelper::GetScrolledRect, but doesn't get the same value. I'll try to figure that out.

Later in page load, the overflow area gets set to a width that matches the scrollport. It looks like it happens in response to JS call to retrieve the offsetWidth of some parent element, which triggers reflow. The JS stack goes through the minified magento library and it doesn't seem useful to reproduce it here.

So, a possible fix might be forcing a call to MVM::RefreshViewportSize in response to changes to the overflow area. I'm not sure how to trigger that.

(In reply to Brad Werth [:bradwerth] from comment #15)

So, a possible fix might be forcing a call to MVM::RefreshViewportSize in response to changes to the overflow area. I'm not sure how to trigger that.

PresShell::ResizeReflow calls MVM::RequestReflow. We'd want something similar that triggered off a change to overflow property of the root element of the RDM scroll pane (I think).

This patch introduces a new flag that is set when the root scroll
area is changed, and triggers MobileViewportManager shrink-to-fit
behavior when reflow is finished. This is necessary to correctly
size the viewport when overflow properties are changed after page
load.

Attachment #9117085 - Attachment is obsolete: true
Attachment #9118448 - Attachment description: Bug 1598145 Part 1: Make the MVM shrink-to-fit in response to a change in the root scroll area. → Bug 1598145 Part 1: Make the MVM shrink-to-fit in response to a change in the root scroll area size.

:bwerth, can you give a short feedback on the status of that patch? We are still tracking this bug for FF74. Thank you!

Flags: needinfo?(bwerth)

The patch has not been accepted by reviewers (see the comments at https://phabricator.services.mozilla.com/D58582) and I haven't taken on the work of the requested investigation. I have been prioritizing other bugs that are in my component (devtools Responsive Design Mode). I intend to return to this bug within the next few weeks. If it's being tracked for 74, someone else will need to pick it up.

Flags: needinfo?(bwerth)
See Also: → 1626080

I'm replying to hiro's reviewer Phab comment on D58582.

I've ignored this for awhile. I just checked and it seems that for this content, UpdateMinimumScaleSize exits early because of "user-scalable=no" in the meta viewport tag. That was added in the patch for Bug 1525805. Are these two ideas in conflict?

Conceptually, it seems that user scalability shouldn't affect the user agent's ability to choose a reasonable size before any user input is applied. The spec seems to say this is the intent: https://www.w3.org/TR/css-device-adapt-1/#user-zoom-desc. It says "The user cannot interactively change the zoom factor."

My new proposed solution is to rollback the code changes in the Bug 1525805 patch D20206, which should make this case work, and then find a different approach to solve Bug 1525805.

Flags: needinfo?(hikezoe.birchill)

do'h! I am sorry I missed the last comment in the Phabricator.

Posted my comment here too

As per the comment in bug 1525805 comment 15, Chrome doesn't apply the minimum scale in case of ""user-scalable=no", so if the site in question is rendered properly on Chrome, the site is doing something depending on UA string? Or maybe there is a race condition, I mean, the "user-scalable=no" is injected at some point during page loading and it causes this issue?

I am very interested in what's going on the site.

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

do'h! I am sorry I missed the last comment in the Phabricator.

Posted my comment here too

As per the comment in bug 1525805 comment 15, Chrome doesn't apply the minimum scale in case of ""user-scalable=no", so if the site in question is rendered properly on Chrome, the site is doing something depending on UA string? Or maybe there is a race condition, I mean, the "user-scalable=no" is injected at some point during page loading and it causes this issue?

I am very interested in what's going on the site.

Which means I am interested in what's going on Chrome too.

I started suspecting this is a dup of bug 1552713.

Specifically see bug 1552713 comment 2

Flags: needinfo?(hikezoe.birchill)
Attachment #9118448 - Attachment is obsolete: true

In an attempt to create a synthetic testcase for this bug, I was able to build this testcase that is not quite the same, but is clearly related to the issues central to this bug. Fixing this testcase will likely result in a fix to this bug.

Steps to Reproduce:

  1. Load the attached file in RDM.
  2. Set the display size to 300 x 600.
  3. Turn on Touch Simluation.
  4. Reload, then reload again.

Expected Results: The page should look much the same as it did on initial load.
Actual Results: The page appears zoomed in. Reloading again will zoom in further.

I'm closer to understanding the root cause of the new testcase attachment 9141436 [details]. In my testing, I'm using a 300 x 600 display area (I have updated the Steps to Reproduce to reflect this). When the meta viewport tag is added dynamically, it shrinks the viewport width from 980px (the default) to 300px, as specified in the tag. The code in MVM::UpdateResolution handles the case of an added meta viewport tag with this logic: preserve the proportion of the content shown in the display area. So in this case, if the display area was showing 300px of a 980px viewport, that's 30.6% of the content area in view. When the viewport width changes to 300px, this math attempts to change the zoom level to keep the same proportion visible, setting a 3.27x zoom level. Unfortunately, the newly-added viewport tag doesn't permit the user to change the zoom again ("user-scalable=no"), so the setting sticks.

I'm not clear yet why the APZ scrolling is an important part of the testcase. So many of these MVM transformations involving setting zoom due to the viewport settings, then changing it due to the content size, etc. My best guess is that one of these other paths is affected by whether or not the page has ever been scrolled. I'm also still confused about why the first reload doesn't trigger the problem; it has something to do with the point where mRestoreResolution is set, which changes the code paths.

I'm also not certain what to do about this issue. I can imagine scenarios where a changed or added meta viewport tag should maintain the proportion of displayed content. I'll probably just disable the zoom scaling when "user-scalable=no" and see what tests break.

(In reply to Brad Werth [:bradwerth] from comment #25)

I'll probably just disable the zoom scaling when "user-scalable=no" and see what tests break.

This change fixes the testcase, but does not fix longines.com. More nuance needed. I'll run the test suite anyway to see what fails.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5ea08a6754184d2cc1395e85ad97a8f0be53c5e&selectedJob=298536177

This change prevents the UpdateType::ViewportSize updates from forcing the
zoom level to maintain proportionality to the viewport size when a new
viewport tag is added with user-scalable=no. This is detected by only checking
for user scalability when the displaySizeChangeRatio is 1.0f. This prevents
site content from being zoomed in a way that doesn't match its initial zoom
level if the viewport tag is added dynamically later.

With meta viewport active, Firefox displays this page dramatically differently than it is displayed in Chrome. Firefox shows the page zoomed out, showing much more than its viewport of 320px, but Chrome shows 320px wide content with scrollbars. This reflects the core issue for the longines.com site.

So the test case makes me think that Chrome doesn't apply auto scale value if user-scalable=no is specified? What I did check is the minimum scale case, so I missed the case at that time.

I will try to debug Chromium with the test case in comment 28 to see what's going on there.

Flags: needinfo?(hikezoe.birchill)

I am still tracking down what's going on in Chrome, but as of now, from what I can tell is Chrome DOES apply auto-calculated scale value which is 1.125 on a 360px width device. You can check the value by using visualViewport.scale (I should have checked it first). So now I am confident that we need to apply the auto calculated scale value even if user-scalable=no is specified.

Flags: needinfo?(hikezoe.birchill)

Though I haven't further dug into detail about the Chrome's behavior, probably what we need to do is what Chrome does, they clamp the max/min values to the initial scale value in cases of user-scalable=no, like this;

diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp
--- a/dom/base/Document.cpp
+++ b/dom/base/Document.cpp
@@ -9830,6 +9830,10 @@ nsViewportInfo Document::GetViewportInfo
         size.height = std::max(size.height, displaySize.height);
       }
-
+      if (effectiveZoomFlag == nsViewportInfo::ZoomFlag::DisallowZoom) {
+        scaleMaxFloat = scaleMinFloat = scaleFloat;
+      }
+
       return nsViewportInfo(
           scaleFloat, scaleMinFloat, scaleMaxFloat, size, sizeFlag,
           mValidScaleFloat ? nsViewportInfo::AutoScaleFlag::FixedScale

With this change, two test cases in this bug look fine in RDM at least.

Attachment #9141901 - Attachment is obsolete: true

This provides web compatability with Chrome's interpretation of the spec at
https://drafts.csswg.org/css-device-adapt/#user-zoom-desc. This changes makes
us treat "the user cannot interactively change the zoom factor" as inclusive
of ANY method of the user agent changing zoom levels, regardless of whether or
not the user initiated an action.

So, in addition to preventing things like double-tap-to-zoom and pinch-zoom,
this change prevents zoom changes to fit all content into the viewport, or to
maintain visible content proportionality if the viewport is resized dynamically.

This test checks several replaced meta viewport tags, exercising the code in
MobileViewportManager that changes resolution to maintain the proportional
amount of content visible in the display area as the viewport size changes.
It also checks the shrink-to-fit behavior of pages with user-scalable=no.

Depends on D72004

To reduce intermittents in RDM tests, setTouchAndMetaViewportSupport and
spawnViewportTask now await reflow before resolving. Additionally an awaited
reflow in setViewportSizeAndAwaitReflow was changed to use the standard
method.

Depends on D72004

Attachment #9143812 - Attachment description: Bug 1598145 Part 2: Add a test of changed meta viewport tags. → Bug 1598145 Part 3: Add a test of changed meta viewport tags.

This asserts that content width should not be a factor for resolution when
user-scalable=no.

Depends on D72762

Attachment #9142466 - Attachment description: Bug 1598145 Part 1: For user-scalable=no, force min and max zoom to match initial zoom. → Bug 1598145 Part 1: For user-scalable=no, force min and max zoom to match initial zoom, add WPT.
Attachment #9144176 - Attachment is obsolete: true
Pushed by bwerth@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8981f5187921 Part 1: For user-scalable=no, force min and max zoom to match initial zoom, add WPT. r=hiro https://hg.mozilla.org/integration/autoland/rev/483d819e3580 Part 2: Make some more RDM test functions await reflow. r=mtigley,hiro https://hg.mozilla.org/integration/autoland/rev/12014da3d9f1 Part 3: Add a test of changed meta viewport tags. r=hiro
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/23355 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Upstream PR merged by moz-wptsync-bot
Regressions: 1638773
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: