Closed Bug 1561227 Opened 1 year ago Closed 1 year ago

RDM should correctly handle full-page zoom and resolution zooming together.

Categories

(DevTools :: Responsive Design Mode, defect, P1)

defect

Tracking

(firefox67 unaffected, firefox68 wontfix, firefox69 wontfix, firefox70 wontfix, firefox71 wontfix, firefox72 verified)

VERIFIED FIXED
Firefox 72
Tracking Status
firefox67 --- unaffected
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- verified

People

(Reporter: alin.ilea, Assigned: bradwerth)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [rdm-mvp])

Attachments

(11 files, 10 obsolete files)

57.70 KB, image/jpeg
Details
53.41 KB, image/jpeg
Details
192 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
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

Affected versions:

Nightly 69.0a1 (2019-06-24) and Beta 68.0b13
Affected platforms:

Windows 10, Mac OS 10.14 and Ubuntu 18.04

Prerequisites:

Access FF and enter in about:config
Type devtools.responsive.metaViewport.enabled and set the value to "true"
Steps:

  1. Open FF and access: https://sketchfab.com
  2. Hit Ctrl + Shift + M in order to start Responsive Design Mode.
  3. Change the zoom level of viewport to 30%.
  4. Reset the zoom level to 100%.

Expected result:
The page should be displayed correctly after reset zoom level to 100%

Actual result:
The page is still zoomed and the user is redirected to the top of the page.

Note: The issue occurs just with touch simulation enabled.
On release version the user cannot zoom just the viewport content, ctrl and "+" or ctrl and mouse wheel will change the whole browser zoom level. In Beta and Nightly the user can zoom just the viewport content with ctrl and "+" command.

Attached image Rdm before zoom.jpg
Assignee: nobody → bwerth
Blocks: rdm-zoom
Priority: -- → P3
Whiteboard: [rdm-mvp]
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [rdm-mvp] → [rdm-reserve]
Whiteboard: [rdm-reserve] → [rdm-mvp]

I can reproduce this too on macOS with Firefox nightly 70.

While reproducing, I noticed that if I turned Touch Simulation / Meta Viewport off after the last step, I get the same clipped viewport seen in Bug 1569626. These may have the same root cause.

See Also: → 1569626

This is a much simpler testcase with the same Steps to Reproduce.

Still sorting this out but the key issue seems to be that, with this meta viewport tag, decreasing zoom values changes resolution directly, but increasing zoom values do not. Ideally, we don't want any special resolution processing with this meta viewport tag even with meta viewport support turned on -- full page zoom should behave the same. One way to achieve that is to make sure that the same code paths are followed whether or not meta viewport support is on.

One solution to the problem is to make the changes proposed for Bug 1523844. Those changes are necessary, so I'm marking that bug as a blocker for this bug.

Depends on: 1523844

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression

I do not think this is a regression because it only occurs with <meta viewport> enabled which is a feature that we have not shipped yet.

Keywords: regression

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

One solution to the problem is to make the changes proposed for Bug 1523844. Those changes are necessary, so I'm marking that bug as a blocker for this bug.

The patches for Bug 1523844 have changed a bit and they no longer wholly solve this problem. The changed part is that those patches used to remove a call to ShrinkToDisplaySizeIfNeeded from RefreshViewportSize at https://searchfox.org/mozilla-central/source/layout/base/MobileViewportManager.cpp#581. That call is required in some cases so it is being left alone in the Bug 1523844 patches. However, the presence of that call also causes THIS bug. So more work will need to be done, but I'll keep Bug 1523844 as a blocker and do the additional work in this bug.

I've found another way to solve this bug; possibly a better way. Full-page zoom is triggering an unnecessary resolution change in ShrinkDisplaySizeIfNeeded->UpdateResolution due to aViewportInfo.IsDefaultZoomValid() being true at https://searchfox.org/mozilla-central/source/layout/base/MobileViewportManager.cpp#389. IsDefaultZoomValid() is true because initial-scale is specified. But that shouldn't make content-fitting behave differently after the initial display of the content. So checking IsDefaultZoomValid() here is probably incorrect and we need something more specific for the intended cases.

Botond, I need help with this one... We need to find some way for full-page zoom to not attempt to shrink content to fit the display size, since the viewport already has the correct resolution.

In MVM::UpdateResolution, for aType == UpdateType::ContentSize, there are two paths that set zoom to intrinsicScale. Our challenge is that we don't want EITHER of them to occur in this Steps To Reproduce. For ContentSize updates, if the zoom level is less than the intrinsicScale, we WILL set zoom to the intrinsicScale in either path. And that's what's happening here.

Specifically, the calling path of nsPresContext::SetFullZoom ... PresShell::ResizeReflow ... MVM::RefreshViewportSize is tasked with resizing the viewport and the content. It does this by expanding the viewport, which sets a lower zoom level, and then calls ShrinkToDisplaySizeIfNeeded and provides the OLD size of the content by calculating a scrollable rect at https://searchfox.org/mozilla-central/source/layout/base/MobileViewportManager.cpp#600. When UpdateResolution is called, the intrinsicScale is calculated as fitting the old content to the display size resulting in an intrinsicScale that's greater than the zoom level. That's the bad situation referenced above.

Flags: needinfo?(botond)

Just to clarify the STR here a bit, based on a conversation with Brad:

(In reply to Alin Ilea from comment #0)

  1. Change the zoom level of viewport to 30%.

This is meant to via done via "Hamburger menu -> Zoom out".

  1. Reset the zoom level to 100%.

And this, presumably, via "Hamburger menu -> Reset zoom level".

Trying to zoom via Ctrl+Minus/Plus has a very different effect, likely triggering other (unrelated) bugs.

Per a conversation with Botond, we agreed that this bug is a side-effect of our current UI decision to keep the RDM pane the same size under full-page zoom. If the pane itself is ALSO sized as zoom changes (not just the contents being sized), then these scaling factors should balance out again. I will be creating a new bug as a blocker for this bug.

Flags: needinfo?(botond)
Depends on: 1572840

I've thought about this a bit. I think part of the solution here is to amend this condition in UpdateResolution() to also include "the user hasn't changed the full zoom".

Basically, if the user has zoomed, we don't want to auto-adjust the zoom level any further. This is currently captured in the !mContext->IsResolutionUpdatedByApz() condition, which basically means "the user hasn't pinch-zoomed", but I think we want to exclude cases where the user has changed the full-zoom as well.

(I'm not sure whether bug 1572840 would also fix this or not, but I feel like we should decide whether we want bug 1572840 independently, based on what behaviour we want for the RDM controls from a UX point of view.)

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

Basically, if the user has zoomed, we don't want to auto-adjust the zoom level any further. This is currently captured in the !mContext->IsResolutionUpdatedByApz() condition, which basically means "the user hasn't pinch-zoomed", but I think we want to exclude cases where the user has changed the full-zoom as well.

That makes sense and I'll explore that in a proposed patch.

(I'm not sure whether bug 1572840 would also fix this or not, but I feel like we should decide whether we want bug 1572840 independently, based on what behaviour we want for the RDM controls from a UX point of view.)

My patches for Bug 1572840 are posted, and they don't make this problem magically go away. Those changes are still the right thing to do, and we may have more complicated issues besides this one (as you mention in comment 13).

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

I've thought about this a bit. I think part of the solution here is to amend this condition in UpdateResolution() to also include "the user hasn't changed the full zoom".

That still leaves the possibility that the resolution could be changed in the else block.

This is a tricker one, because it's a situation that's unique to the combination of mobile viewport sizing and full-zoom.

On desktop, the ICB size is always determined by the size of the browser window's content area. If the user decreases the full-zoom, making the content area larger in CSS pixel terms, the ICB size also gets larger in CSS pixel terms. This is true even with desktop zooming.

Once we bring mobile viewport sizing into the mix, you can now get an ICB size which is fixed in CSS pixel terms (e.g. with <meta name="viewport" content="width=1000">). If the following are true:

  • the page has a fixed ICB width specified in the meta-viewport tag
  • the RDM pane's physical size does not change when decreasing the full-zoom
  • the user decreases the full-zoom enough that the ICB width underflows the RDM pane width
  • the page's content width is no larger than the ICB width (or it is larger, but the user decreases the full-zoom even further such that the content width also underflows the RDM pane width)

then we get into a situation where the page's content is narrower than the RDM pane and part of the RDM pane is just showing blank space. This "showing blank space" scenario is what the else block in question is trying to prevent.

So, possible solutions for this one are:

  • Decreasing the full-zoom does decrease the RDM pane's width (i.e. bug 1572840)
  • We allow MobileViewportManager to "veto" full-zoom-out events if they would cause the content width to underflow the RDM pane width
  • We allow the "showing blank space" scenario to happen. Hiro's work in bug 1508177 suggests that there isn't any technical difficulty with this, but we'd still want to be careful not to allow it on actual phones.

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

So, possible solutions for this one are:

  • Decreasing the full-zoom does decrease the RDM pane's width (i.e. bug 1572840)

The patches for Bug 1572840 seem plausible and likely to land. I'll check this bug against a build with those patches applied.

  • We allow MobileViewportManager to "veto" full-zoom-out events if they would cause the content width to underflow the RDM pane width

I think there are additional reasons to consider a veto. By doing a full-zoom too far, with patches for Bug 1572840 applied, it's trivial to make the RDM pane exceed the bounds of the browser window. It's such a bad user experience, particularly with the horizontal excess, that we'll probably want to prevent this from happening.

Huh. With the patches for Bug 1572840 applied, we still have a problem, but I think it is an order-of-operations problem. What we want is for the correct display size to be made available when UpdateResolution is called. Instead, first those UpdateResolution calls happen with the old display size:

nsPresContext::SetFullZoom
nsViewManager::SetWindowDimensions
nsViewManager::DoSetWindowDimensions
mozilla::PresShell::ResizeReflow
MobileViewportManager::RequestReflow
MobileViewportManager::RefreshViewportSize

and then later this call stack updates to the new display size:

mozilla::dom::BrowserChild::RecvUpdateDimensions
nsWebBrowser::SetPositionAndSize
nsDocShell::SetPositionAndSize
nsDocumentViewer::SetBoundsWithFlags

and then eventually does another call to MVM::RefreshViewportSize. If these two calls were made in the other order, that would fix this bug. I don't know enough about the BrowserChild message receiving and what triggers that, and can we get it to happen before the other call stack.

I guess the problem is that conceptually, changing the full-zoom and the RDM pane size are an atomic operation, but MVM is processing the two changes independently.

I notice SetFullZoom() sets a flag called mSuppressResizeReflow, which causes us to not actually reflow. Perhaps that flag should also suppress the RefreshViewportSize (or at least the UpdateResolution)?

Summary: Reset zoom to 100% after zoom out to 30% in responsive design mode does not restore viewport content correctly → RDM should correctly handle full-page zoom and resolution zooming together.
Attachment #9085852 - Attachment is obsolete: true
Attachment #9090096 - Attachment is obsolete: true
Attachment #9090098 - Attachment is obsolete: true
Attachment #9090102 - Attachment description: Bug 1561227 Part 3: Make MVM::UpdateResolution only care about initial-scale on first paint. → Bug 1561227 Part 1: Make MVM::UpdateResolution only care about initial-scale on first paint.
Attachment #9090104 - Attachment description: Bug 1561227 Part 4: Rename an existing RDM zoom test in anticipation of adding related tests. → Bug 1561227 Part 2: Rename an existing RDM zoom test in anticipation of adding related tests.
Attachment #9090105 - Attachment description: Bug 1561227 Part 5: Add a test of meta viewport resizing with full zoom. → Bug 1561227 Part 3: Add a test of meta viewport resizing with full zoom.
Attachment #9090096 - Attachment is obsolete: false
Attachment #9090098 - Attachment is obsolete: false
Attachment #9090102 - Attachment description: Bug 1561227 Part 1: Make MVM::UpdateResolution only care about initial-scale on first paint. → Bug 1561227 Part 3: Make MVM::UpdateResolution only care about initial-scale on first paint.
Attachment #9090104 - Attachment description: Bug 1561227 Part 2: Rename an existing RDM zoom test in anticipation of adding related tests. → Bug 1561227 Part 4: Rename an existing RDM zoom test in anticipation of adding related tests.
Attachment #9090105 - Attachment description: Bug 1561227 Part 3: Add a test of meta viewport resizing with full zoom. → Bug 1561227 Part 5: Add a test of meta viewport resizing with full zoom.

I'm starting to work through the test failures triggered by these changes. One of the interesting failures is the test added by Bug 1539687, which checks that APZ can target scroll points beyond the bounds of the layout viewport. The meta viewport tag in this test contains an "initial-scale=2.0". I can't get the test to fail locally, but it's clear that if the initial-scale is ignored during the test, it would cause the error seen the in the test log. Since https://phabricator.services.mozilla.com/D44502 adds an additional constraint to initial-scale that it will only be respected during mIsFirstPaint, I assume this is causing the problem in the test harness.

So, it's possible that either mIsFirstPaint is the wrong condition for respecting initial-scale, or mIsFirstPaint is not persisting through all of the many calls to UpdateResolution that happen on initial load. The root cause here will also need to be addressed as part of this patch.

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

or mIsFirstPaint is not persisting through all of the many calls to UpdateResolution that happen on initial load.

Note, mIsFirstPaint will only be true during the following RefreshViewportSize calls:

  • the first of the load or before-first-paint events

  • possibly additional times for DOMMetaAdded events that occur after the above call, but before the document finishes loading

Conceptually, the goal of changing full zoom with an MVM is that the final resolution should be nearly the same at the initial resolution. So a page that starts with initial-zoom=2.0 should stay near that resolution through full page zooms, no matter what the device-width setting might be. This patch is not respecting that; instead it does a shrink-to-fit as soon as zoom is changed. Need to rethink.

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

I notice SetFullZoom() sets a flag called mSuppressResizeReflow, which causes us to not actually reflow. Perhaps that flag should also suppress the RefreshViewportSize (or at least the UpdateResolution)?

I am now aligned on this goal, to make full zoom changes have no effect on resolution (as they do without an MVM) but mSuppressResizeReflow isn't enough to get it done. The call from nsPresContext::SetFullZoom (where mSuppressResizeReflow is set) does not actually make any calls to UpdateResolution, because it's stopped in MVM::RefreshViewportSize.

The unwanted calls to UpdateResolution come later, through BrowserChild::RecvUpdateDimensions only because of the SetBoundsWithFlags changes in these patches. I'll see if I can craft a solution that omits that part, while adding a piece to make sure that the resolution makes content snug to the viewport without shrinking to the intrinsic scale.

The necessary calls to UpdateResolution will occur from the RDM pane being
resized. There's no longer a need to listen to this event separately.

Depends on D46672

Attachment #9090104 - Attachment description: Bug 1561227 Part 4: Rename an existing RDM zoom test in anticipation of adding related tests. → Bug 1561227 Part 5: Rename an existing RDM zoom test in anticipation of adding related tests.
Attachment #9090105 - Attachment description: Bug 1561227 Part 5: Add a test of meta viewport resizing with full zoom. → Bug 1561227 Part 6: Add a test of meta viewport resizing with full zoom.
Attachment #9090102 - Attachment is obsolete: true
Attachment #9090098 - Attachment is obsolete: true
Attachment #9090096 - Attachment is obsolete: true

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

The unwanted calls to UpdateResolution come later, through BrowserChild::RecvUpdateDimensions

This is getting closer to correct behavior. The patches still fail with full zoom of documents with initial-scale != 1.0, and would probably also fail with documents that had undergone APZ zoom and then were full zoomed, though I don't have a test that explores this. I believe the reason for the failure is that preventing the call to ResizeReflow when nsPresContext::SuppressingResizeReflow() is necessary for both the browser parent and the browser child, but the changes in https://phabricator.services.mozilla.com/D46672 are only effective on the parent.

One way to make the child also suppress resize reflow would be to make the message received by BrowserChild::RecvUpdateDimensions carry a SuppressingResizeReflow() flag supplied by the parent. That's tricky but doable. Emilio, do you think this is a reasonable way to proceed?

Flags: needinfo?(emilio)

One way to make the child also suppress resize reflow would be to make the message received by BrowserChild::RecvUpdateDimensions carry a SuppressingResizeReflow() flag supplied by the parent. That's tricky but doable. Emilio, do you think this is a reasonable way to proceed?

I don't understand, why would that be desirable? suppressing the reflow is only an optimization, shouldn't have side effects. Could you elaborate on how things go wrong?

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #39)

I don't understand, why would that be desirable? suppressing the reflow is only an optimization, shouldn't have side effects. Could you elaborate on how things go wrong?

That's a good point. It should be an optimization only and I hadn't been thinking that way. This is my understanding of what is causing the undesirable side effects. When the viewport is zoomed, MVM::UpdateResolution gets an UpdateType::ViewportSize update with the display size of the viewport converted into CSS units, and a ratio of the new display size relative to the old display size. Unfortunately, the display size actually has changed but it has changed only proportionally to the change in AppUnitsPerDevPixel maintained by the nsPresContext. So to make UpdateResolution do better math, an option might be to pass in the old AppUnitsPerDevPixel value, or cache the value last used value, more likely. If we had that value, we could further tweak the "adjustment" of the aDisplaySizeChangeRatio to compensate for the changing AppUnitsPerDevPixel. I'll see if I can get that working.

This ensures that full zoom changes to viewport size won't make
resolution changes.

Depends on D46673

Attachment #9090104 - Attachment description: Bug 1561227 Part 5: Rename an existing RDM zoom test in anticipation of adding related tests. → Bug 1561227 Part 6: Rename an existing RDM zoom test in anticipation of adding related tests.
Attachment #9090105 - Attachment description: Bug 1561227 Part 6: Add a test of meta viewport resizing with full zoom. → Bug 1561227 Part 7: Add a test of meta viewport resizing with full zoom.

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

So to make UpdateResolution do better math, an option might be to pass in the old AppUnitsPerDevPixel value, or cache the value last used value, more likely. If we had that value, we could further tweak the "adjustment" of the aDisplaySizeChangeRatio to compensate for the changing AppUnitsPerDevPixel. I'll see if I can get that working.

This seems to be working. MVMContext already had a CSSToDevPixelScale method, so I used that.

Attachment #9090098 - Attachment is obsolete: false
Attachment #9090098 - Attachment is obsolete: true

Brad, just FYI, I just submitted bug 1583534 which should make your stuff much easier to implement.

It adds a way of explicitly opting out of reflow from ResizeReflow{,IgnoreOverride}, so I'd expect MVM to use it like:

ResizeReflowIgnoreOverride(..., SuppressReflow);
presShell->GetDocument()->FlushPendingNotifications(FlushType::InterruptibleLayout);

wdyt? That should simplify the patches here I think.

Flags: needinfo?(bwerth)
Attachment #9094326 - Attachment is obsolete: true
Attachment #9094327 - Attachment is obsolete: true
Attachment #9094713 - Attachment description: Bug 1561227 Part 5: Make MVM::UpdateResolution consider changes to CSSToDev scale when handling viewport updates. → Bug 1561227 Part 3: Make MVM::UpdateResolution consider changes to CSSToDev scale when handling viewport updates.

This event is useful for tests that resize the RDM pane and need to
know when all resolution adjusting effects are complete.

Depends on D46845

Blocks: 1547101

The change to await snapshotWindow is something that should have been
done in Bug 1573254.

Depends on D44504

(In reply to Emilio Cobos Álvarez (:emilio) from comment #44)

Brad, just FYI, I just submitted bug 1583534 which should make your stuff much easier to implement.

Thanks, Emilio. The updated patches no longer attempt to "optimize" the SuppressResizeReflow case. Once it was clear that MVM::UpdateResolution needed to function correctly even when it was called during a full zoom change, I changed my efforts towards that instead of preventing the call from happening in the first place.

Flags: needinfo?(bwerth)
Attachment #9094323 - Attachment is obsolete: true

Getting the fix right is quite involved. I have been trying to make MVM::UpdateResolution always do the right thing, which during full zoom changes is "nothing" and that's very difficult. The key challenge is that the calling pattern starting at nsDocumentViewer::SetFullZoom. It roughly follows these steps:

  1. nsDocumentViewer::SetFullZoom sets the app units per dev pixel, which resizes the window.
  2. MVM::UpdateResolution interprets this change and ideally makes no changes to resolution.
  3. The front end rescales the window to match the new zoom level, triggering another resize of the window.
  4. MVM::UpdateResolution interprets this change and ideally makes no changes to resolution.

The problem is that during step 2, for a document with a fixed-width viewport, Document::GetViewportInfo will ensure that the viewport is expanded to minimally fill the display size. If in step 1 we set the app units per dev pixel to a very small number (perhaps by going from 100% zoom to 30% zoom), then the viewport size is stretched and therefore changes at the check in MVM::RefreshViewportSize and MVM::UpdateResolution gets called. Under these conditions, it's very contrived to make the call have no effect since the cssToDevScale, the aViewportOrContentSize, and the aDisplayWidthChangeRatio have all changed. Indeed if this call wasn't being followed up by the call in step 4 it really should force a resolution change to ensure that content fits in the display area.

So perhaps it will be necessary to just prevent the call in step 2 from happening at all. An earlier version of the patch did this by keying off of nsPresContext::SuppressingResizeReflow, which is intended to only indicate an opportunity for optimization, not as a signal for a behavior change. But it's seeming increasingly necessary and I'm tempted to go back to this approach.

The only other way I could see to resolve this is for the MVM to save and restore resolution, spanning from the initial call to nsDocumentViewer::SetFullZoom through to the final resizing done by BrowserChild::RecvUpdateDimensions, but this would be a whole new pathway for setting resolution that would essentially undo the intermediate steps being done by MVM::UpdateResolution which feels inefficient.

Emilio, what approach would you prefer here?

Flags: needinfo?(emilio)

So, I'm a bit confused about what the root cause of the problem is here, but maybe because I don't have a clear idea of everything that's going on.

Could you provide me some concrete STR where we get the behavior wrong if we do what I suggested here? I'm happy to dig a bit using rr to try figure out what is going wrong.

Should changing full zoom really do nothing to the resolution? If so, why is it factored in Document::GetViewportInfo? Should it be? I assume the case where we factor fullZoom into the equation is the one that's causing trouble?

Ideally, the final viewport we end up with does not depend on the ordering or presence of steps here. I don't know why coming up with which viewport size / resolution we end up with should be stateful, given the same input.

That sounds definitely bogus, and jumping through some of the steps to not hit one of those issues seems just a wallpaper to me.

Flags: needinfo?(emilio) → needinfo?(bwerth)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #52)

So, I'm a bit confused about what the root cause of the problem is here, but maybe because I don't have a clear idea of everything that's going on.

Could you provide me some concrete STR where we get the behavior wrong if we do what I suggested here? I'm happy to dig a bit using rr to try figure out what is going wrong.

That code just ensures that updatetype::contentsize updates do not tweak resolution when they shouldn't. Your proposed change is a cleaner version of https://phabricator.services.mozilla.com/D46670. That problem seems solved. The problem I am encountering in comment 52 is for updatetype::viewportsize calls to MVM::UpdateResolution. I'll provide updated STR at the end of this.

Should changing full zoom really do nothing to the resolution? If so, why is it factored in Document::GetViewportInfo? Should it be? I assume the case where we factor fullZoom into the equation is the one that's causing trouble?

The cases where full zoom is used in Document::GetViewportInfo are both legacy cases not hit in this bug. One of those cases is for documents with NO meta viewport information, and is used in a heuristic to determine an appropriate viewport size in that case. It's possible that those legacy cases will eventually go away, but in the meantime, they aren't part of the root cause here.

Ideally, the final viewport we end up with does not depend on the ordering or presence of steps here. I don't know why coming up with which viewport size / resolution we end up with should be stateful, given the same input.

That sounds definitely bogus, and jumping through some of the steps to not hit one of those issues seems just a wallpaper to me.

The steps laid out in comment 52 are not ordered by the user -- they are the internal calls made when a user does the single action of setting the full zoom level. Unfortunately, the RDM panel has to resize in response to the full zoom level changing, and that resize is done as a front-end action after the full zoom changes happen in the platform. We really don't want the viewport to be recalculated or the resolution to be changed in the middle of that process, and finding an appropriate way to fix that is what this bug is all about.

Clearer Steps to Reproduce:

  1. about:config, set the "devtools.responsive.metaViewport.enabled" pref to true.
  2. Open attachment 9083135 [details].
  3. Start Responsive Design Mode. Change the width of the RDM pane to 600 pixels wide.
  4. Turn on Touch Simulation (the hand icon in the center-right of the RDM toolbar). This also turns on meta viewport handling.
  5. Note that the green div appears within the bounds of the RDM pane, particularly the right-hand side has a gap between the div and the edge of the pane.
  6. Set full zoom to 90%. Note that the green div now overflows the RDM pane on the right.

What has happened is there have been a series of calls to MVM::UpdateResolution that have adjusted the resolution. The updatetype::contentsize ones are addressed by https://phabricator.services.mozilla.com/D46670 . The problem remains with the updateype::viewportsize calls, which need to be changed to ignore the transitory case where the appUnitsPerDevPixel have shrunk but the display area has not yet shrunk.

I'll put together a new version of my patches that interprets mSuppressingSizeReflow as a signal to in MVM::RefreshViewportSize with a detailed comment.

Flags: needinfo?(bwerth)

We need to listen to this event to detect when full zoom changes
are complete.

Depends on D48621

Attachment #9096779 - Attachment description: Bug 1561227 Part 5: Define a helper function for setting RDM zoom and use it in existing tests. → Bug 1561227 Part 4: Define a helper function for setting RDM zoom and use it in existing tests.
Attachment #9090104 - Attachment description: Bug 1561227 Part 6: Rename an existing RDM zoom test in anticipation of adding related tests. → Bug 1561227 Part 5: Rename an existing RDM zoom test in anticipation of adding related tests.
Attachment #9090105 - Attachment description: Bug 1561227 Part 7: Add a test of meta viewport resizing with full zoom. → Bug 1561227 Part 6: Add a test of meta viewport resizing with full zoom.
Attachment #9097041 - Attachment description: Bug 1561227 Part 8: Update expectations for existing tests. → Bug 1561227 Part 7: Update expectations for existing tests.
Attachment #9096777 - Attachment is obsolete: true
Attachment #9094713 - Attachment is obsolete: true
Attachment #9094324 - Attachment is obsolete: true
See Also: → 1588177
Attachment #9096777 - Attachment is obsolete: false
Attachment #9096777 - Attachment description: Bug 1561227 Part 4: Define and fire an internal event when BrowserChild has finished resizing. → Bug 1561227 Part 2: Define and fire an internal event when BrowserChild has finished resizing.

These initial messages appear to be unnecessary, and they confuse the
ZoomChild that is newly created to handle the Responsive Design Mode
pane.

Depends on D48624

Attachment #9096779 - Attachment description: Bug 1561227 Part 4: Define a helper function for setting RDM zoom and use it in existing tests. → Bug 1561227 Part 5: Define a helper function for setting RDM zoom and use it in existing tests.
Attachment #9090104 - Attachment description: Bug 1561227 Part 5: Rename an existing RDM zoom test in anticipation of adding related tests. → Bug 1561227 Part 6: Rename an existing RDM zoom test in anticipation of adding related tests.
Attachment #9090105 - Attachment description: Bug 1561227 Part 6: Add a test of meta viewport resizing with full zoom. → Bug 1561227 Part 7: Add a test of meta viewport resizing with full zoom.
Attachment #9097041 - Attachment description: Bug 1561227 Part 7: Update expectations for existing tests. → Bug 1561227 Part 8: Update expectations for existing tests.
Attachment #9099727 - Attachment is obsolete: true
Attachment #9102644 - Attachment description: Bug 1561227 Part 4: Prevent ZoomChild from sending events when receiving initial values. → Bug 1561227 Part 4: Make ZoomChild respond to a new message to cache its full zoom level.
Attachment #9102644 - Attachment description: Bug 1561227 Part 4: Make ZoomChild respond to a new message to cache its full zoom level. → Bug 1561227 Part 4: Make the ZoomChild actor cache its initial fullZoom and textZoom values.
Attachment #9102644 - Attachment description: Bug 1561227 Part 4: Make the ZoomChild actor cache its initial fullZoom and textZoom values. → Bug 1561227 Part 4: Make RDM force the UI ZoomChild Actor to cache its zoom level.
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/849e1cf51a8f
Part 1: Make nsDocumentViewer send a new event before setting full zoom levels on the pres contexts. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f8b8e593316f
Part 2: Define and fire an internal event when BrowserChild has finished resizing. r=botond,emilio
https://hg.mozilla.org/integration/autoland/rev/677da957ddcb
Part 3: Make RDM UI save and restore resolution when responding to a full zoom change. r=mtigley
https://hg.mozilla.org/integration/autoland/rev/1b63d555c6c1
Part 4: Make RDM force the UI ZoomChild Actor to cache its zoom level. r=mconley,gl
https://hg.mozilla.org/integration/autoland/rev/257bc09f49af
Part 5: Define a helper function for setting RDM zoom and use it in existing tests. r=botond
https://hg.mozilla.org/integration/autoland/rev/693b0dd88f2b
Part 6: Rename an existing RDM zoom test in anticipation of adding related tests. r=botond
https://hg.mozilla.org/integration/autoland/rev/635534927ffc
Part 7: Add a test of meta viewport resizing with full zoom. r=botond
https://hg.mozilla.org/integration/autoland/rev/8117cafb3a20
Part 8: Update expectations for existing tests. r=botond

Backed out for failures on browser_viewport_resizing_scrollbar.js

backout: https://hg.mozilla.org/integration/autoland/rev/bb2d2f3832bf3e55d623e27aba3166ce2ddddf01

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8117cafb3a205311137ee2a294765fed2a4ed967&group_state=expanded&selectedJob=272865153

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=272865153&repo=autoland&lineNumber=12322

[task 2019-10-24T23:25:30.639Z] 23:25:30 INFO - Waiting for viewport-resize to 300 x 600
[task 2019-10-24T23:25:30.640Z] 23:25:30 INFO - GECKO(3574) | console.log: "[DISPATCH] action type:" "RESIZE_VIEWPORT"
[task 2019-10-24T23:25:30.640Z] 23:25:30 INFO - Got viewport-resize to 300 x 600
[task 2019-10-24T23:25:30.656Z] 23:25:30 INFO - TEST-INFO | started process screentopng
[task 2019-10-24T23:25:31.089Z] 23:25:31 INFO - TEST-INFO | screentopng: exit 0
[task 2019-10-24T23:25:31.089Z] 23:25:31 INFO - TEST-UNEXPECTED-FAIL | devtools/client/responsive/test/browser/browser_viewport_resizing_scrollbar.js | Snapshot canvases are not the same size: 300x600 vs. 600x300 -
[task 2019-10-24T23:25:31.089Z] 23:25:31 INFO - Stack trace:
[task 2019-10-24T23:25:31.089Z] 23:25:31 INFO - chrome://mochikit/content/browser-test.js:test_ok:1297
[task 2019-10-24T23:25:31.089Z] 23:25:31 INFO - chrome://mochikit/content/tests/SimpleTest/WindowSnapshot.js:compareSnapshots:24
[task 2019-10-24T23:25:31.089Z] 23:25:31 INFO - chrome://mochitests/content/browser/devtools/client/responsive/test/browser/browser_viewport_resizing_scrollbar.js:null:104
[task 2019-10-24T23:25:31.090Z] 23:25:31 INFO - chrome://mochitests/content/browser/devtools/client/responsive/test/browser/head.js:addRDMTask/<:148
[task 2019-10-24T23:25:31.090Z] 23:25:31 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1067
[task 2019-10-24T23:25:31.090Z] 23:25:31 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1102
[task 2019-10-24T23:25:31.090Z] 23:25:31 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:930
[task 2019-10-24T23:25:31.090Z] 23:25:31 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:805
[task 2019-10-24T23:25:31.091Z] 23:25:31 INFO - TEST-PASS | devtools/client/responsive/test/browser/browser_viewport_resizing_scrollbar.js | Window snapshots should match. -
[task 2019-10-24T23:25:31.091Z] 23:25:31 INFO - Meta Viewport ON setting meta viewport support.
[task 2019-10-24T23:25:31.091Z] 23:25:31 INFO - Reload is needed -- waiting for it.
[task 2019-10-24T23:25:31.091Z] 23:25:31 INFO - Current size: 300 x 600, set to: 300 x 600
[task 2019-10-24T23:25:31.091Z] 23:25:31 INFO - Current size: 300 x 600, set to: 600 x 300
[task 2019-10-24T23:25:31.092Z] 23:25:31 INFO - Waiting for viewport-resize to 600 x 300
[task 2019-10-24T23:25:31.094Z] 23:25:31 INFO - GECKO(3574) | console.log: "[DISPATCH] action type:" "RESIZE_VIEWPORT"
[task 2019-10-24T23:25:31.095Z] 23:25:31 INFO - Got viewport-resize to 600 x 300
[task 2019-10-24T23:25:31.095Z] 23:25:31 INFO - Current size: 600 x 300, set to: 300 x 600
[task 2019-10-24T23:25:31.095Z] 23:25:31 INFO - Waiting for viewport-resize to 300 x 600
[task 2019-10-24T23:25:31.095Z] 23:25:31 INFO - GECKO(3574) | console.log: "[DISPATCH] action type:" "RESIZE_VIEWPORT"
[task 2019-10-24T23:25:31.095Z] 23:25:31 INFO - Got viewport-resize to 300 x 600
[task 2019-10-24T23:25:31.095Z] 23:25:31 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-10-24T23:25:31.097Z] 23:25:31 INFO - TEST-UNEXPECTED-FAIL | devtools/client/responsive/test/browser/browser_viewport_resizing_scrollbar.js | Snapshot canvases are not the same size: 300x600 vs. 600x300 -
[task 2019-10-24T23:25:31.097Z] 23:25:31 INFO - Stack trace:
[task 2019-10-24T23:25:31.097Z] 23:25:31 INFO - chrome://mochikit/content/browser-test.js:test_ok:1297
[task 2019-10-24T23:25:31.098Z] 23:25:31 INFO - chrome://mochikit/content/tests/SimpleTest/WindowSnapshot.js:compareSnapshots:24
[task 2019-10-24T23:25:31.098Z] 23:25:31 INFO - chrome://mochitests/content/browser/devtools/client/responsive/test/browser/browser_viewport_resizing_scrollbar.js:null:104
[task 2019-10-24T23:25:31.098Z] 23:25:31 INFO - chrome://mochitests/content/browser/devtools/client/responsive/test/browser/head.js:addRDMTask/<:148
[task 2019-10-24T23:25:31.099Z] 23:25:31 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1067
[task 2019-10-24T23:25:31.099Z] 23:25:31 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1102
[task 2019-10-24T23:25:31.099Z] 23:25:31 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:930
[task 2019-10-24T23:25:31.099Z] 23:25:31 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:805
[task 2019-10-24T23:25:31.100Z] 23:25:31 INFO - TEST-PASS | devtools/client/responsive/test/browser/browser_viewport_resizing_scrollbar.js | Window snapshots should match. -

Flags: needinfo?(bwerth)
No longer blocks: 1547101
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97be65338edf
Part 1: Make nsDocumentViewer send a new event before setting full zoom levels on the pres contexts. r=emilio
https://hg.mozilla.org/integration/autoland/rev/cfcf79794049
Part 2: Define and fire an internal event when BrowserChild has finished resizing. r=botond,emilio
https://hg.mozilla.org/integration/autoland/rev/a3759e1a04e2
Part 3: Make RDM UI save and restore resolution when responding to a full zoom change. r=mtigley
https://hg.mozilla.org/integration/autoland/rev/aef3cfa2f539
Part 4: Make RDM force the UI ZoomChild Actor to cache its zoom level. r=mconley,gl
https://hg.mozilla.org/integration/autoland/rev/ac88bbefb95f
Part 5: Define a helper function for setting RDM zoom and use it in existing tests. r=botond
https://hg.mozilla.org/integration/autoland/rev/4776cec83ae8
Part 6: Rename an existing RDM zoom test in anticipation of adding related tests. r=botond
https://hg.mozilla.org/integration/autoland/rev/b22440a67078
Part 7: Add a test of meta viewport resizing with full zoom. r=botond
https://hg.mozilla.org/integration/autoland/rev/aa00b1b62ea7
Part 8: Update expectations for existing tests. r=botond
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c212479e5ca
Part 1: Make nsDocumentViewer send a new event before setting full zoom levels on the pres contexts. r=emilio
https://hg.mozilla.org/integration/autoland/rev/0d1ad0f881d1
Part 2: Define and fire an internal event when BrowserChild has finished resizing. r=botond,emilio
https://hg.mozilla.org/integration/autoland/rev/0ff7eb5f45fd
Part 3: Make RDM UI save and restore resolution when responding to a full zoom change. r=mtigley
https://hg.mozilla.org/integration/autoland/rev/70c9f888057a
Part 4: Make RDM force the UI ZoomChild Actor to cache its zoom level. r=mconley,gl
https://hg.mozilla.org/integration/autoland/rev/99fce0ce5375
Part 5: Define a helper function for setting RDM zoom and use it in existing tests. r=botond
https://hg.mozilla.org/integration/autoland/rev/855980cca497
Part 6: Rename an existing RDM zoom test in anticipation of adding related tests. r=botond
https://hg.mozilla.org/integration/autoland/rev/068b231e041a
Part 7: Add a test of meta viewport resizing with full zoom. r=botond
https://hg.mozilla.org/integration/autoland/rev/1ec2654d55f8
Part 8: Update expectations for existing tests. r=botond

Hi,

We tried to verify the fix in latest Nightly 72, but we cannot check the fix because now we are not able to zoom just the viewport content. The zoom is applied to the entire viewport window not just to the content into it. Can someone take a look and let us know if this is the intended behavior now?

Thanks.

(In reply to Alin Ilea from comment #64)

Hi,

We tried to verify the fix in latest Nightly 72, but we cannot check the fix because now we are not able to zoom just the viewport content. The zoom is applied to the entire viewport window not just to the content into it. Can someone take a look and let us know if this is the intended behavior now?

Full zoom will now change the RDM viewport size. That should changes the content of the RDM pane proportionally at each step. So any changes to full zoom should result in a similar-looking RDM content, just smaller or bigger.

Flags: needinfo?(bwerth)

Verified - fixed on latest Nightly 72.0a1 (Build id: 20191028215046), considering the updates from comment 65.

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