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.

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.

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.

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

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.

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.

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.

Depends on D16100

Depends on D17997

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

Depends on D17999

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

Depends on D18001

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?

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.

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

Depends on D19235

Depends on D19237

Depends on D19238

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

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

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.

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.

Depends on D19239

Depends on D16100

Depends on D16101

(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.

Depends on D17999

Depends on D21287

Depends on D19239

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

Depends on D21617

Backed out 6 changesets (Bug 1525662, Bug 1501665) for geckoview failures at org.mozilla.geckoview.test.NavigationDelegateTest.loadData_noMimeType

Backout: https://hg.mozilla.org/integration/autoland/rev/10f3bcf2a0ed1a35764e8ac1a48d76042ba1ef78

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=1fa7091b4b4eb5d57319894a09728bd435136cb0&selectedJob=231977925

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=231977925&repo=autoland&lineNumber=2894

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

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.

https://hg.mozilla.org/mozilla-central/rev/4f88eb70887a

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.

Depends on D21291

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

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?

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.

https://hg.mozilla.org/mozilla-central/rev/c37181ff94ab
https://hg.mozilla.org/mozilla-central/rev/b159f05c2d8e
https://hg.mozilla.org/mozilla-central/rev/c947824bd2d8
https://hg.mozilla.org/mozilla-central/rev/7a7781e568c2
https://hg.mozilla.org/mozilla-central/rev/05efd157254b
https://hg.mozilla.org/mozilla-central/rev/7d0c48d43cbd
https://hg.mozilla.org/mozilla-central/rev/67b1eb6b5087
https://hg.mozilla.org/mozilla-central/rev/c8bf0f59a60c
https://hg.mozilla.org/mozilla-central/rev/22aac830b13a
https://hg.mozilla.org/mozilla-central/rev/8824d55c1b62
https://hg.mozilla.org/mozilla-central/rev/916cbe96f1d3
https://hg.mozilla.org/mozilla-central/rev/1468489e29bf
https://hg.mozilla.org/mozilla-central/rev/572526065fb2

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

(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.

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.