Enabling meta viewport breaks the viewport (-size)

VERIFIED FIXED in Firefox 68

Status

defect
P1
normal
VERIFIED FIXED
9 months ago
18 days ago

People

(Reporter: mbalfanz, Assigned: bradwerth)

Tracking

(Blocks 2 bugs, {regression})

64 Branch
Firefox 68
Desktop
All
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox64 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 wontfix, firefox68 verified)

Details

(Whiteboard: [dt-q])

Attachments

(20 attachments, 14 obsolete attachments)

654.09 KB, video/mp4
Details
59.72 KB, image/png
Details
781.01 KB, image/gif
Details
1.30 MB, video/mp4
Details
47 bytes, text/x-phabricator-request
Details | Review
176.74 KB, video/mp4
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
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
85.46 KB, image/png
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
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Posted video demo.mp4 (obsolete) —
STR:
- visit any page
- enable RDM
- change the viewport size
- enable touch simulation

The size of the actual viewport does not match the "displayed viewport" anymore.  Notice in the attached video also the behavior when you try to resize again.
Posted video demo2.mp4 (obsolete) —
Actually, no need to resize anything. Activating touch simulation messes things up already.
Assignee: nobody → bwerth
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [dt-q]
I am unable to reproduce this. Martin, is there any other nuance in how you are reproducing this? Your video picks up after step 2 -- already on the page, RDM already on. Alternatively, would you please see if you can find some way where this is NOT happening for you? Perhaps that will reveal what I'm doing that prevents me from reproducing the issue.
Flags: needinfo?(mbalfanz)
Posted video full.mp4
It always happens to me and I can't find a case where it doesn't.  Maybe the key is that the RDM viewport size has to be bigger than 1024x768 to actually see this.

In the video, you see the STR:

1. I run Firefox from a fresh profile, latest m-c
2. On the default page, I enter RDM through the keyboard shortcut cmd+opt+m
3. I enable touch simulation < note that the layout changes here, so something is already up
4. I increase the viewport size until I see the mismatch clearly (background color as indicator)

I noticed the warning about refreshing the page, but it doesn't change anything if I do.

Hope that helps :)
Attachment #9019693 - Attachment is obsolete: true
Attachment #9019695 - Attachment is obsolete: true
Flags: needinfo?(mbalfanz)
Posted image offset.png
Something like what Martin experienced here just happened to me outside of RDM.
I just had a simple page opened, with devtools opened too (and the flexbox highlighter on), and I was playing with various values of the layout.css.devPixelsPerPx pref.
I set it to 2, to 1, to 0.9, 1.2, etc. just to test something in devtools, and after that I set it back to its default -1.0 value.

Once I did that, the viewport size started not matching the size of the tab anymore.
It looked as if the entire UI of Firefox went back to a -1.0 devPixelsPerPx ratio, but the content page did not. It remained at 1.2, which caused the page to appear smaller.
Posted image offset.gif
And here's a GIF to see it in action.
Note that I was on Windows 10, with my PC connected to an external high-density monitor.
See Also: → 1514779
OS: Unspecified → All
Hardware: Unspecified → Desktop
Version: unspecified → 64 Branch
Thanks for the regression range Martin. This confirms the suspicion that this is due to bug 1290420 which landed in 64.
So unfortunately, we now have this regression in release.

Brad is really the one who can work on this. He has already started pushing patches to another related bug (bug 1504659).
I think those patches will fix the current bug too.

Realistically, this won't get fixed before January. When this happens, we'll need to carefully evaluate the risk of uplifting to at least beta. But I have a feeling this might be a lot of code changes, and we might have to live with this regression for a couple releases.
Blocks: 1290420
See Also: → 1504659
Marking wontfix for 64 based on comment 7.
Duplicate of this bug: 1500314
Depends on D16100
Blocks: rdm-ux
Posted video mc-vs-nightly.mp4

I applied D16100 locally and gave it a shot. Unfortunately, it didn't fix the problem.

Please see the video attached. On the left hand side you see m-c with the patch applied, on the right hand side the latest Nightly. Both behave the same for me.

(In reply to Martin Balfanz [:mbalfanz] from comment #13)

Created attachment 9036651 [details]
mc-vs-nightly.mp4

I applied D16100 locally and gave it a shot. Unfortunately, it didn't fix the problem.

Please see the video attached. On the left hand side you see m-c with the patch applied, on the right hand side the latest Nightly. Both behave the same for me.

Okay, I can reproduce this on my macOS machine, without an external monitor. With a wide-enough browser window, I see the same things that you're seeing in your video. I'll try to figure it out with what I can replicate here.

Okay, here's what's going on with the example in attachment 9036651 [details]. That is a Bugzilla page, and it has a meta viewport tag: <meta name="viewport" content="width=1024">. That tag states that when the meta viewport is respected, the layout viewport should be no wider than 1024. When we turn on touch simulation, we also turn on the meta viewport, since as of Bug 1290420 those features travel together to better simulate the device experience.

So, that explains why the viewport maxxes out at 1024. I argue this is correct since the layout viewport should NOT be wider than that in this case. If we want to make this more clear, two options I can think of are to center the layout viewport within the RDM panel, or we could constrain the RDM panel from resizing more than this amount. If we decide to do one of these, that should be a seperate dependent bug.

However, that's not the only problem shown in the video. The height is being oddly clamped also. That is happening because the code at https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/dom/base/Document.cpp#6792 clamps the height to match the aspect ratio of the pre-clamped RDM panel size. I'll add something to the patch to fix this.

Attachment #9035454 - Attachment description: Bug 1501665 Part 1: Restore assignment of initial viewport width when viewport width is auto. → Bug 1501665 Part 2: Restore assignment of initial viewport width when viewport width is auto.
Attachment #9035455 - Attachment description: Bug 1501665 Part 2: Update test expectations. → Bug 1501665 Part 3: Update test expectations.

Martin, try applying Parts 1 and 2 and see if they resolve the problem for you. This patch stack is still not landable because it causes tests to fail for the font scaling issue that Part 2 reverts.

Flags: needinfo?(mbalfanz)

Thanks Brad! I can confirm that the patch fixes the issue with the clamped height. And I understand why this behavior might be correct, though the user experience is weird. I will file new bugs for this an another oddity I noticed while testing.

Flags: needinfo?(mbalfanz)

This change also affects the way that zoom ratios are calculated when
resizing the visual viewport. The changes are constrained to also consider
the layout viewport, maintaining the invariant that a visual viewport
narrower than the visual viewport should not zoom.

Depends on D16100

Attachment #9035455 - Attachment description: Bug 1501665 Part 3: Update test expectations. → Bug 1501665 Part 4: Update test expectations.
Duplicate of this bug: 1520814

Martin, please try the new Parts 1 through 3. They implement the approach you requested in Bug 1520814 -- to zoom in when the visual viewport is made larger than the layout viewport. Thank you for filing that bug. I have marked Bug 1520814 as a duplicate of this bug.

These patches are not complete. They cause bad zoom behavior when loading a page into a small RDM panel with touch simulation on. If that happens in your testing, please resize the RDM panel and reload.

Thanks Brad! I tested it, and as you described it causes some false behavior on smaller viewports, but it works quiet nicely on 1024px width and above. So good progress! I also think the overall behavior feels right this way.

Posted video google-scrollbar.mp4

Brad, I'm not sure if it's related but I just discovered this in meta viewport handling:

  • Visit google.com
  • enter RDM
  • choose a device
  • refresh the page
  • increase the viewport size

The scrollbars stay in place.

Meta viewport is: <meta content="width=device-width,initial-scale=1.0" name="viewport">

This starts to become a more general question on how RDM should behave on resizing, esp. when you select a predefined device. Chrome, for example, does not allow you to resize the viewport when you select a preset device. I feel we should do the same. I'll set up a meeting to discuss the details.

No longer blocks: 1521934

Once Bug 1521814 lands, this issue will need the pref dom.meta-viewport.enabled set to true in order to properly test it.

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

Once Bug 1521814 lands, this issue will need the pref dom.meta-viewport.enabled set to true in order to properly test it.

Update: the relevant pref is devtools.responsive.metaViewport.enabled.

No longer blocks: rdm-ux

Instead of checking for platform APZ capabilities, instead check if the
viewportInfo reports that zooming should be allowed or not.

Depends on D17999

Attachment #9037716 - Attachment description: Bug 1501665 Part 3: Allow resolution zoom to be used on non-APZ platforms, to better support Responsive Design Mode. → Bug 1501665 Part 6: Change the way we calculate device width change ratios to avoid zooming out too far.

This is to respect the intent of the meta viewport tag, which is that the
content should not be shrunk below a certain size.

Depends on D17041

Attachment #9035455 - Attachment description: Bug 1501665 Part 4: Update test expectations. → Bug 1501665 Part 9: Update test expectations.

Botond, I need your help. Something that landed in Bug 521644 is constraining whether the RDM pane will show zoomed-in content. Parts 1 through 3 of this patch should force a fixed width meta viewport document to render zoomed in inside the RDM pane if the pane is wider than the specified width. It was doing that before Bug 521644 landed. I'm guessing that the final part https://hg.mozilla.org/integration/autoland/rev/998c765c5a96 is overriding the PresShell resolution for platforms where APZAllowZooming is false.

Anyway, I'm not sure how to integrate with this recent change and I'd appreciate any insight. What is necessary to get non APZ platforms to use the pres shell resolution, like I believe we need for the RDM pane?

Flags: needinfo?(botond)

And I have confirmed that setting the pref 'layout.scroll.root-frame-containers' to 1 allows the RDM pane to show zoomed content. But again, I want to integrate with this containerless scrolling feature, not override it.

(Comment 32 is intending to reference bug 1521644.)

You're right, bug 1521644 did move some code related to processing of the pres shell resolution behind the condition that apz.allow_zooming=true. That wasn't exactly intentional, but I guess a background assumption here is that we only set a pres shell resolution in the first place if zooming is allowed.

Thinking about this a bit, based on our experiences in bug 1504659 and what you're pointing out here, I think what would make the most sense is to tie all zooming activity, including RDM with meta viewport tags, to apz.allow_zooming=true (or at least the codepaths that are currently conditional on it).

We're already planning to enable apz.allow_zooming=true on desktop (tracked in bug 1461360 and its dependencies), but RDM doesn't need to wait for that to happen, as a lot of the dependencies of that do not apply to RDM. For example, bug 1523405 is about rendering layout (non-overlay) scrollbars, but RDM uses overlay scrollbars so it's not applicable. So, I think we can try to enable APZ zooming support for RDM only.

This might be a concrete way to do that:

  • Introduce a new function, AllowZoomingForDocument(document), which would return gfxPrefs::APZAllowZooming() || <document is inside RDM> (or possibly <document is inside RDM and touch simulation is enabled>).
  • Replace most call sites of gfxPrefs::APZAllowZooming() with AllowZoomingForDocument(document).

This would mean we would use the APZ zooming codepaths for documents inside RDM, without having to set apz.allow_zooming=true on desktop (which would affect regular desktop pages, not just RDM).

How does that sound? It seems like it would simplify the work in this bug, make the first patch in bug 1504659 unnecessary, and solve bug 1520320 (since we can then just enable apz.allow_zooming=true in those tests). It is also possible that it surfaces new issues, but they would be issues we'd want to fix in the near future anyways to allow desktop zooming.

Flags: needinfo?(botond)

Thanks for this thorough answer. This sounds like a good plan.

Attachment #9037108 - Attachment description: Bug 1501665 Part 1: Fix an edge case in Document::GetViewportInfo to return css scaled sizes. → Bug 1501665 Part 5: Fix an edge case in Document::GetViewportInfo to return css scaled sizes.
Attachment #9035454 - Attachment description: Bug 1501665 Part 2: Restore assignment of initial viewport width when viewport width is auto. → Bug 1501665 Part 6: Restore assignment of initial viewport width when viewport width is auto.
Attachment #9039967 - Attachment description: Bug 1501665 Part 4: Allow MVM::RequestReflow to adjust resolution, and do so when destroying the MVM. → Bug 1501665 Part 7: Allow MVM::RequestReflow to adjust resolution, and do so when destroying the MVM.

What's important is that we track whether the width is specified, not if
it has or hasn't been resolved to an auto-like value.

Depends on D17999

Default zoom for specified width viewports should be 100% zoom. The
MobileViewportManager can adjust that as necessary later.

Depends on D19241

Attachment #9037716 - Attachment description: Bug 1501665 Part 6: Change the way we calculate device width change ratios to avoid zooming out too far. → Bug 1501665 Part 10: Change the way we calculate device width change ratios to avoid zooming out too far.
Attachment #9039965 - Attachment is obsolete: true
Attachment #9039968 - Attachment is obsolete: true
Attachment #9039985 - Attachment is obsolete: true
Attachment #9039969 - Attachment is obsolete: true
Attachment #9035455 - Attachment description: Bug 1501665 Part 9: Update test expectations. → Bug 1501665 Part 11: Update test expectations.
Attachment #9035455 - Attachment description: Bug 1501665 Part 11: Update test expectations. → Bug 1501665 Part 8: Update test expectations.
Attachment #9042666 - Attachment is obsolete: true
Attachment #9042669 - Attachment is obsolete: true
Attachment #9037716 - Attachment is obsolete: true
Posted image metaview300.png

Botond, I'm bogged down again on this and I would appreciate some help. With this patch stack applied, we get the right layout viewport scaling behavior with meta viewport on, but the scrollbar sizing and placement is wrong. It seems clear to me that this is due to the scrollbar dimensions being based off of the unscaled layout viewport size, instead of off the visual viewport size. I'm working through the callstack that reaches ScrollFrameHelper::LayoutScrollbars(), but if you have an insight into how to resolve this quickly, I'd love to hear it.

The testcase URL is www.bradleyjwerth.com/metaview300.html

Flags: needinfo?(botond)

I'm unlikely to be able to investigate this until I'm back from the standards meeting I'll be at next week, but one thought in the meantime:

If you enable apz.allow_zooming=true and dom.meta-viewport.enabled=true and restart, do you see the same issues with scrollbar rendering on regular desktop pages (controlling for the expected side effect of dom.meta-viewport.enabled=true that most pages will render into a width of 980px rather than the browser window width)? I'm assuming here you're testing on Mac, and therefore have overlay scrollbars.

If so, the issue is probably easier to debug on the regular desktop page than in RDM.

Flags: needinfo?(botond)

I looked into this a little on the plane. It looks like correct rendering of scrollbars with zooming requires taking codepaths guarded by the overlayScrollBarsWithZoom variable in LayoutScrollbars(), but that variable is false with RDM. I guess we need to tweak the initializer of that variable to include checking for RDM.

Attachment #9037108 - Attachment description: Bug 1501665 Part 5: Fix an edge case in Document::GetViewportInfo to return css scaled sizes. → Bug 1501665 Part 6: Fix an edge case in Document::GetViewportInfo to return css scaled sizes.
Attachment #9035454 - Attachment description: Bug 1501665 Part 6: Restore assignment of initial viewport width when viewport width is auto. → Bug 1501665 Part 7: Restore assignment of initial viewport width when viewport width is auto.
Attachment #9039967 - Attachment description: Bug 1501665 Part 7: Allow MVM::RequestReflow to adjust resolution, and do so when destroying the MVM. → Bug 1501665 Part 9: Allow MVM::RequestReflow to adjust resolution, and do so when destroying the MVM.
Attachment #9035455 - Attachment description: Bug 1501665 Part 8: Update test expectations. → Bug 1501665 Part 10: Update test expectations.
Attachment #9045479 - Attachment description: Bug 1501665 Part 6: Change UpdateShouldBuildAsyncZoomContainer to check if document is zoomable. → Bug 1501665 Part 5: Change UpdateShouldBuildAsyncZoomContainer to check if document is zoomable.

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

https://treeherder.mozilla.org/#/
jobs?repo=try&revision=640f2ae6bb84523e4b1a5bd3fc6e5772acfcb2a8

To reiterate my Phabricator comment: I've tried the Android APK from this Try run, and it seems that this patch series breaks rendering of non-mobile-friendly pages on a real mobile device.
Those (i.e. pages without an explicit <meta> viewport tag) have traditionally been rendering at browser.viewport.desktopWidth even when not forcing desktop viewport mode (through "Request desktop site"), and there's a very good reason for that: With this patch, it looks like those pages end up with something that looks like an implicit width=device-width, which, given the typical display widths on a phone in portrait mode, causes frequent layout breakage on even moderately complex pages.

Now admittedly browser.viewport.desktopWidth (i.e. 980 px) zoomed to fit a portrait mode phone screen (300-something px or so?) doesn't lead to very readable text, but the layout breakage is why to solve the font size issue, we ended up with font inflation instead of simply reducing the viewport width for those pages. I.e. we keep rendering non-mobile-friendly pages at 980 px to keep the layout intact and then selectively increase font sizes for targetted parts of the content to restore readability, which normally (there are admittedly some corner cases) causes less layout breakage than reducing the viewport width does.

Especially in view of the patches for bug 1501665, which seem to have some-
what misunderstood the reason for the choice of viewport width here.

(In reply to Jan Henning [:JanH] from comment #51)

To reiterate my Phabricator comment: I've tried the Android APK from this Try run, and it seems that this patch series breaks rendering of non-mobile-friendly pages on a real mobile device.
Those (i.e. pages without an explicit <meta> viewport tag) have traditionally been rendering at browser.viewport.desktopWidth even when not forcing desktop viewport mode (through "Request desktop site"), and there's a very good reason for that: With this patch, it looks like those pages end up with something that looks like an implicit width=device-width, which, given the typical display widths on a phone in portrait mode, causes frequent layout breakage on even moderately complex pages.

Thanks for the detailed explanation. I'll rethink the relevant part of the patches and see what can be done to preserve the current desired behavior but also fixing the unwanted RDM impacts.

Maybe you could just detect being loaded inside the RDM environment and make your viewport handling conditional on that?

Although in a way it would of course be nice if the RDM could more accurately reflect what actually happens in our mobile browsers (or other's, too), though I suppose that would be somewhat more complicated...

(In reply to Jan Henning [:JanH] from comment #54)

Maybe you could just detect being loaded inside the RDM environment and make your viewport handling conditional on that?

Although in a way it would of course be nice if the RDM could more accurately reflect what actually happens in our mobile browsers (or other's, too), though I suppose that would be somewhat more complicated...

I think the solution will be noting that the viewport tag is empty as a new enum in https://searchfox.org/mozilla-central/rev/b10ae6b7a50d176a813900cbe9dc18c85acd604b/dom/base/Document.h#4449, and choosing the current behavior (using the desktopWidth pref value) as an alternate path when the enum is set to that value. That will ensure that RDM is reflective of actual device behavior, without actually changing the behavior on those devices. I'm preparing a patch that does that now.

Attachment #9045864 - Attachment is obsolete: true
Attachment #9035455 - Attachment description: Bug 1501665 Part 10: Update test expectations. → Bug 1501665 Part 12: Update test expectations.
Summary: Enabling touch simulation breaks the viewport (-size) → Enabling meta viewport breaks the viewport (-size)
Attachment #9035454 - Attachment is obsolete: true

This file has existing issues with clang-format that made later
functional changes difficult to diff cleanly.

Depends on D21617

Attachment #9045479 - Attachment description: Bug 1501665 Part 5: Change UpdateShouldBuildAsyncZoomContainer to check if document is zoomable. → Bug 1501665 Part 7: Change UpdateShouldBuildAsyncZoomContainer to check if document is zoomable.
Attachment #9037108 - Attachment description: Bug 1501665 Part 6: Fix an edge case in Document::GetViewportInfo to return css scaled sizes. → Bug 1501665 Part 8: Fix an edge case in Document::GetViewportInfo to return css scaled sizes.
Attachment #9045481 - Attachment description: Bug 1501665 Part 8: Add a new function to allow visual viewport size to be un-set. → Bug 1501665 Part 9: Add a new function to allow visual viewport size to be un-set.
Attachment #9039967 - Attachment description: Bug 1501665 Part 9: Allow MVM::RequestReflow to adjust resolution, and do so when destroying the MVM. → Bug 1501665 Part 10: Allow MVM::RequestReflow to adjust resolution, and do so when destroying the MVM.
Attachment #9046877 - Attachment description: Bug 1501665 Part 10: Early exit around an unnecessary call to UpdateVisualViewportSize. → Bug 1501665 Part 11: Early exit around an unnecessary call to UpdateVisualViewportSize.
Attachment #9046884 - Attachment description: Bug 1501665 Part 11: Modify viewport resize zoom scaling to account for clamped zoom levels. → Bug 1501665 Part 12: Modify viewport resize zoom scaling to account for clamped zoom levels.
Attachment #9035455 - Attachment description: Bug 1501665 Part 12: Update test expectations. → Bug 1501665 Part 13: Update test expectations.
See Also: → 1500314
Attachment #9035455 - Attachment is obsolete: true
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/159753062cfa
No bug: Add comment with rationale behind choice of default viewport. r=dbaron

Passing needinfo to the creator of the patch. The patch may be easier to reason about in a new bug.

Flags: needinfo?(bwerth) → needinfo?(jh+bugzilla)
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4f88eb70887a
No bug: Add comment with rationale behind choice of default viewport. r=dbaron DONTBUILD CLOSED TREE

Jan, it looks like your "no bug" patch[1] ended up getting associated with this bug[2], which is why it appeared here and landed with this bug ID in its commit message.

This probably happened because the extended commit message mentioned this bug number, and it's probably worth considering that auto-linkage to be a bug in some part of the process.

Would you mind filing that as a bug, with a description of whatever process you used to generate your "no bug" phabricator page? It'd probably go under...
https://bugzilla.mozilla.org/enter_bug.cgi?product=Conduit&component=Review%20Wrapper
...if you used moz-phab, or under...
https://bugzilla.mozilla.org/enter_bug.cgi?product=Conduit&component=Phabricator
...if not.

[1] https://phabricator.services.mozilla.com/D20950
[2] It has "Bugzilla Bug ID 1501665" in a header near the top of the page, even though the bug number isn't mentioned in the title.

Sorry for the confusion, and yes, it seems our systems aren't really set up for that case.
I think the main problem in this case would be Pulsebot, though, so it would have happened even if I manually landed this on inbound, wouldn't it?

I used the hg phabsend-fork for the patch, which indeed misattribute the bug ID, too, although I subsequently deleted it from Phabricator again. Kris's phabsend fork seems to use our commit parser for getting the bug ID, though.

Flags: needinfo?(jh+bugzilla)

There are two viewports: the layout (or content) viewport, and the visual
viewport. This change ensures that we are both setting and checking the
visual viewport size, which is the size shown in the RDM control bar.
It is still possible to get the layout viewport with getContentSize().

Depends on D22436

This test uses a Document with a fixed-width meta viewport. This test
simulates resizing the RDM pane from a wide landscape size to a small
square size, and back again. It does this again with meta viewport
support turned on.

Depends on D22437

Attachment #9045479 - Attachment description: Bug 1501665 Part 7: Change UpdateShouldBuildAsyncZoomContainer to check if document is zoomable. → Bug 1501665 Part 6: Change UpdateShouldBuildAsyncZoomContainer to check if document is zoomable.
Attachment #9037108 - Attachment description: Bug 1501665 Part 8: Fix an edge case in Document::GetViewportInfo to return css scaled sizes. → Bug 1501665 Part 7: Fix an edge case in Document::GetViewportInfo to return css scaled sizes.
Attachment #9045481 - Attachment description: Bug 1501665 Part 9: Add a new function to allow visual viewport size to be un-set. → Bug 1501665 Part 8: Add a new function to allow visual viewport size to be un-set.
Attachment #9039967 - Attachment description: Bug 1501665 Part 10: Allow MVM::RequestReflow to adjust resolution, and do so when destroying the MVM. → Bug 1501665 Part 9: Allow MVM::RequestReflow to adjust resolution, and do so when destroying the MVM.
Attachment #9046877 - Attachment description: Bug 1501665 Part 11: Early exit around an unnecessary call to UpdateVisualViewportSize. → Bug 1501665 Part 10: Early exit around an unnecessary call to UpdateVisualViewportSize.
Attachment #9046884 - Attachment description: Bug 1501665 Part 12: Modify viewport resize zoom scaling to account for clamped zoom levels. → Bug 1501665 Part 11: Modify viewport resize zoom scaling to account for clamped zoom levels.
Attachment #9049049 - Attachment description: Bug 1501665 Part 13: Add a new test of meta viewport with zero width. → Bug 1501665 Part 12: Add a new test of meta viewport with zero width.
Attachment #9049050 - Attachment description: Bug 1501665 Part 14: Update RDM viewport size reporting to make it consistent with zoomed presshells. → Bug 1501665 Part 13: Update RDM viewport size reporting to make it consistent with zoomed presshells.
Attachment #9049051 - Attachment description: Bug 1501665 Part 15: Add a test of Responsive Design Mode resizing of meta viewports. → Bug 1501665 Part 14: Add a test of Responsive Design Mode resizing of meta viewports.
Attachment #9047558 - Attachment is obsolete: true
Attachment #9049049 - Attachment description: Bug 1501665 Part 12: Add a new test of meta viewport with zero width. → Bug 1501665 Part 12: Add a new test of meta viewport with zero display width.

It appears to be attachment 9047556 [details] (Part 5 -- https://phabricator.services.mozilla.com/D21617) that is causing the Android reftest failures. I confess I didn't really the implications of your proposal to revert this code (https://phabricator.services.mozilla.com/D19239#578344). Has your opinion changed? Is it really correct to revert this part of the code?

Flags: needinfo?(botond)

Interesting. I figured that we'd no longer need that change, since thanks to the other patches, nsLayoutUtils::AllowZoomingForDocument(mDocument) would return true in the cases that motivated the change originally (and therefore the early-exit wouldn't be taken).

I don't think the revert is necessary for the correctness of the other patches, so I'm happy to drop it from this patch series if it's causing problems. We can file a follow-up bug to investigate in more detail why the revert is causing test failures.

Flags: needinfo?(botond)
Attachment #9045479 - Attachment description: Bug 1501665 Part 6: Change UpdateShouldBuildAsyncZoomContainer to check if document is zoomable. → Bug 1501665 Part 5: Change UpdateShouldBuildAsyncZoomContainer to check if document is zoomable.
Attachment #9037108 - Attachment description: Bug 1501665 Part 7: Fix an edge case in Document::GetViewportInfo to return css scaled sizes. → Bug 1501665 Part 6: Fix an edge case in Document::GetViewportInfo to return css scaled sizes.
Attachment #9045481 - Attachment description: Bug 1501665 Part 8: Add a new function to allow visual viewport size to be un-set. → Bug 1501665 Part 7: Add a new function to allow visual viewport size to be un-set.
Attachment #9039967 - Attachment description: Bug 1501665 Part 9: Allow MVM::RequestReflow to adjust resolution, and do so when destroying the MVM. → Bug 1501665 Part 8: Allow MVM::RequestReflow to adjust resolution, and do so when destroying the MVM.
Attachment #9046877 - Attachment description: Bug 1501665 Part 10: Early exit around an unnecessary call to UpdateVisualViewportSize. → Bug 1501665 Part 9: Early exit around an unnecessary call to UpdateVisualViewportSize.
Attachment #9046884 - Attachment description: Bug 1501665 Part 11: Modify viewport resize zoom scaling to account for clamped zoom levels. → Bug 1501665 Part 10: Modify viewport resize zoom scaling to account for clamped zoom levels.
Attachment #9049049 - Attachment description: Bug 1501665 Part 12: Add a new test of meta viewport with zero display width. → Bug 1501665 Part 11: Add a new test of meta viewport with zero display width.
Attachment #9049050 - Attachment description: Bug 1501665 Part 13: Update RDM viewport size reporting to make it consistent with zoomed presshells. → Bug 1501665 Part 12: Update RDM viewport size reporting to make it consistent with zoomed presshells.
Attachment #9049051 - Attachment description: Bug 1501665 Part 14: Add a test of Responsive Design Mode resizing of meta viewports. → Bug 1501665 Part 13: Add a test of Responsive Design Mode resizing of meta viewports.
Attachment #9047556 - Attachment is obsolete: true
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be1026de486b
Part 1: Add a webidl property to Document to track if it's in an RDM pane. r=smaug
https://hg.mozilla.org/integration/autoland/rev/8e0afe4a041a
Part 2: Set the inRDMPane property on Documents as they enter/leave RDM. r=gl
https://hg.mozilla.org/integration/autoland/rev/731d7ee06d86
Part 3: Add a new function to determine if a document can use resolution zooming. r=botond
https://hg.mozilla.org/integration/autoland/rev/cf42ea4e8443
Part 4: Use the new function as a replacement for APZAllowZooming. r=botond
https://hg.mozilla.org/integration/autoland/rev/6a84e97d0e62
Part 5: Change UpdateShouldBuildAsyncZoomContainer to check if document is zoomable. r=botond
https://hg.mozilla.org/integration/autoland/rev/9eebe767ef20
Part 6: Fix an edge case in Document::GetViewportInfo to return css scaled sizes. r=botond
https://hg.mozilla.org/integration/autoland/rev/178210eb72ba
Part 7: Add a new function to allow visual viewport size to be un-set. r=smaug
https://hg.mozilla.org/integration/autoland/rev/088dc24eabc7
Part 8: Allow MVM::RequestReflow to adjust resolution, and do so when destroying the MVM. r=botond
https://hg.mozilla.org/integration/autoland/rev/3542bf2b89dd
Part 9: Early exit around an unnecessary call to UpdateVisualViewportSize. r=botond
https://hg.mozilla.org/integration/autoland/rev/520dd24a73fc
Part 10: Modify viewport resize zoom scaling to account for clamped zoom levels. r=botond
https://hg.mozilla.org/integration/autoland/rev/5bdf0ad9dc66
Part 11: Add a new test of meta viewport with zero display width. r=botond
https://hg.mozilla.org/integration/autoland/rev/afaf26d7df42
Part 12: Update RDM viewport size reporting to make it consistent with zoomed presshells. r=gl
https://hg.mozilla.org/integration/autoland/rev/2fa518cb0dfc
Part 13: Add a test of Responsive Design Mode resizing of meta viewports. r=gl
Backout by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/af46b1e88998
Backed out 13 changesets for failing a11y tests in accessible/tests/mochitest/relations/test_tabbrowser.xul CLOSED TREE
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c37181ff94ab
Part 1: Add a webidl property to Document to track if it's in an RDM pane. r=smaug
https://hg.mozilla.org/integration/autoland/rev/b159f05c2d8e
Part 2: Set the inRDMPane property on Documents as they enter/leave RDM. r=gl
https://hg.mozilla.org/integration/autoland/rev/c947824bd2d8
Part 3: Add a new function to determine if a document can use resolution zooming. r=botond
https://hg.mozilla.org/integration/autoland/rev/7a7781e568c2
Part 4: Use the new function as a replacement for APZAllowZooming. r=botond
https://hg.mozilla.org/integration/autoland/rev/05efd157254b
Part 5: Change UpdateShouldBuildAsyncZoomContainer to check if document is zoomable. r=botond
https://hg.mozilla.org/integration/autoland/rev/7d0c48d43cbd
Part 6: Fix an edge case in Document::GetViewportInfo to return css scaled sizes. r=botond
https://hg.mozilla.org/integration/autoland/rev/67b1eb6b5087
Part 7: Add a new function to allow visual viewport size to be un-set. r=smaug
https://hg.mozilla.org/integration/autoland/rev/c8bf0f59a60c
Part 8: Allow MVM::RequestReflow to adjust resolution, and do so when destroying the MVM. r=botond
https://hg.mozilla.org/integration/autoland/rev/22aac830b13a
Part 9: Early exit around an unnecessary call to UpdateVisualViewportSize. r=botond
https://hg.mozilla.org/integration/autoland/rev/8824d55c1b62
Part 10: Modify viewport resize zoom scaling to account for clamped zoom levels. r=botond
https://hg.mozilla.org/integration/autoland/rev/916cbe96f1d3
Part 11: Add a new test of meta viewport with zero display width. r=botond
https://hg.mozilla.org/integration/autoland/rev/1468489e29bf
Part 12: Update RDM viewport size reporting to make it consistent with zoomed presshells. r=gl
https://hg.mozilla.org/integration/autoland/rev/572526065fb2
Part 13: Add a test of Responsive Design Mode resizing of meta viewports. r=gl

Marking this as FIXED, which didn't seem to happen automatically in this case.

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED

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

Marking this as FIXED, which didn't seem to happen automatically in this
case.

To avoid that in the future, clear the leave-open keyword before landing.

Duplicate of this bug: 1514779
Duplicate of this bug: 1513165
Flags: qe-verify+

Confirmed the issue with 65.0a1 (2018-10-24) on Windows 10.
Fix verified with 68.0b7 on Windows 10, macOS 10.11, Ubuntu 18.04.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.