RDM should correctly handle full-page zoom and resolution zooming together.
Categories
(DevTools :: Responsive Design Mode, defect, P1)
Tracking
(firefox67 unaffected, firefox68 wontfix, firefox69 wontfix, firefox70 wontfix, firefox71 wontfix, firefox72 verified)
People
(Reporter: ailea, Assigned: bradwerth)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [rdm-mvp])
Attachments
(11 files, 10 obsolete files)
(deleted),
image/jpeg
|
Details | |
(deleted),
image/jpeg
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details | |
(deleted),
text/x-phabricator-request
|
Details |
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:
- Open FF and access: https://sketchfab.com
- Hit Ctrl + Shift + M in order to start Responsive Design Mode.
- Change the zoom level of viewport to 30%.
- 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.
Reporter | ||
Comment 1•5 years ago
|
||
Reporter | ||
Comment 2•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 3•5 years ago
|
||
I can reproduce this too on macOS with Firefox nightly 70.
Assignee | ||
Comment 4•5 years ago
|
||
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.
Assignee | ||
Comment 5•5 years ago
|
||
This is a much simpler testcase with the same Steps to Reproduce.
Assignee | ||
Comment 6•5 years ago
|
||
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.
Assignee | ||
Comment 7•5 years ago
|
||
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.
Comment 8•5 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
Comment 9•5 years ago
|
||
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.
Assignee | ||
Comment 10•5 years ago
|
||
(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.
Assignee | ||
Comment 11•5 years ago
|
||
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.
Assignee | ||
Comment 12•5 years ago
|
||
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.
Comment 13•5 years ago
|
||
Just to clarify the STR here a bit, based on a conversation with Brad:
(In reply to Alin Ilea from comment #0)
- Change the zoom level of viewport to 30%.
This is meant to via done via "Hamburger menu -> Zoom out".
- 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.
Assignee | ||
Comment 14•5 years ago
|
||
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.
Comment 15•5 years ago
|
||
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.)
Assignee | ||
Comment 16•5 years ago
|
||
(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).
Comment 17•5 years ago
|
||
(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.
Assignee | ||
Comment 18•5 years ago
|
||
(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.
Assignee | ||
Comment 19•5 years ago
|
||
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.
Comment 20•5 years ago
|
||
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
)?
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 21•5 years ago
|
||
Depends on D41467
Updated•5 years ago
|
Assignee | ||
Comment 22•5 years ago
|
||
Assignee | ||
Comment 23•5 years ago
|
||
Depends on D44499
Assignee | ||
Comment 24•5 years ago
|
||
Depends on D44501
Assignee | ||
Comment 25•5 years ago
|
||
Depends on D44502
Assignee | ||
Comment 26•5 years ago
|
||
Depends on D44503
Assignee | ||
Comment 27•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 28•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 29•5 years ago
|
||
Assignee | ||
Comment 30•5 years ago
|
||
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.
Comment 31•5 years ago
|
||
(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
orbefore-first-paint
events -
possibly additional times for
DOMMetaAdded
events that occur after the above call, but before the document finishes loading
Assignee | ||
Comment 32•5 years ago
|
||
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.
Assignee | ||
Comment 33•5 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #20)
I notice
SetFullZoom()
sets a flag calledmSuppressResizeReflow
, which causes us to not actually reflow. Perhaps that flag should also suppress theRefreshViewportSize
(or at least theUpdateResolution
)?
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.
Assignee | ||
Comment 34•5 years ago
|
||
Assignee | ||
Comment 35•5 years ago
|
||
Depends on D46669
Assignee | ||
Comment 36•5 years ago
|
||
Depends on D46670
Assignee | ||
Comment 37•5 years ago
|
||
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
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 38•5 years ago
|
||
(In reply to Brad Werth [:bradwerth] from comment #33)
The unwanted calls to
UpdateResolution
come later, throughBrowserChild::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?
Comment 39•5 years ago
|
||
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?
Assignee | ||
Comment 40•5 years ago
|
||
(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.
Assignee | ||
Comment 41•5 years ago
|
||
This ensures that full zoom changes to viewport size won't make
resolution changes.
Depends on D46673
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 42•5 years ago
|
||
(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.
Assignee | ||
Comment 43•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 44•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 45•5 years ago
|
||
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
Assignee | ||
Comment 46•5 years ago
|
||
Depends on D47364
Assignee | ||
Comment 47•5 years ago
|
||
Assignee | ||
Comment 48•5 years ago
|
||
The change to await snapshotWindow is something that should have been
done in Bug 1573254.
Depends on D44504
Assignee | ||
Comment 49•5 years ago
|
||
Assignee | ||
Comment 50•5 years ago
|
||
(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.
Updated•5 years ago
|
Assignee | ||
Comment 51•5 years ago
|
||
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:
nsDocumentViewer::SetFullZoom
sets the app units per dev pixel, which resizes the window.MVM::UpdateResolution
interprets this change and ideally makes no changes to resolution.- The front end rescales the window to match the new zoom level, triggering another resize of the window.
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?
Comment 52•5 years ago
|
||
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.
Assignee | ||
Comment 53•5 years ago
|
||
(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:
- about:config, set the "devtools.responsive.metaViewport.enabled" pref to true.
- Open attachment 9083135 [details].
- Start Responsive Design Mode. Change the width of the RDM pane to 600 pixels wide.
- Turn on Touch Simulation (the hand icon in the center-right of the RDM toolbar). This also turns on meta viewport handling.
- 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.
- 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.
Assignee | ||
Comment 54•5 years ago
|
||
Assignee | ||
Comment 55•5 years ago
|
||
We need to listen to this event to detect when full zoom changes
are complete.
Depends on D48621
Assignee | ||
Comment 56•5 years ago
|
||
Depends on D48622
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 57•5 years ago
|
||
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
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Comment 58•5 years ago
|
||
Comment 59•5 years ago
|
||
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. -
Comment 60•5 years ago
|
||
Comment 61•5 years ago
|
||
Backed out 8 changesets (bug 1561227) for causing browser_viewport_resizing_scrollbar.js to permafail
push that caused the backout:
https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=273017117&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunnable&revision=aa00b1b62ea7c8eae81c69443c9677460a0746e8
backout: https://hg.mozilla.org/integration/autoland/rev/d9f3f9fa527e73b3ed965636f2232627513125dd
Comment 62•5 years ago
|
||
Comment 63•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4c212479e5ca
https://hg.mozilla.org/mozilla-central/rev/0d1ad0f881d1
https://hg.mozilla.org/mozilla-central/rev/0ff7eb5f45fd
https://hg.mozilla.org/mozilla-central/rev/70c9f888057a
https://hg.mozilla.org/mozilla-central/rev/99fce0ce5375
https://hg.mozilla.org/mozilla-central/rev/855980cca497
https://hg.mozilla.org/mozilla-central/rev/068b231e041a
https://hg.mozilla.org/mozilla-central/rev/1ec2654d55f8
Reporter | ||
Comment 64•5 years ago
|
||
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.
Assignee | ||
Comment 65•5 years ago
|
||
(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.
Reporter | ||
Comment 66•5 years ago
|
||
Verified - fixed on latest Nightly 72.0a1 (Build id: 20191028215046), considering the updates from comment 65.
Updated•5 years ago
|
Reporter | ||
Updated•5 years ago
|
Description
•