1496609 - Add tests of getViewportInfo with initial-scale smaller than min-scale

Status: RESOLVED FIXED

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

Description

[Hiroyuki Ikezoe (:hiro)]

STR

1. Open attachment 8855130 [details] in RDM
2. Enable touch simulation

Comment 1

[Martin Balfanz [:mbalfanz]]

Happens in Nightly only, not Stable or Dev Edition.

Comment 2

[Martin Balfanz [:mbalfanz]]

Attachment: RDM-with-touch-simulation

Comment 3

[Hiroyuki Ikezoe (:hiro)]

I accidentally noticed how to solve this issue.
In MobileViewportManager::RefreshViewportSize we should also check nsLayoutUtils::ShouldHandleMetaViewport() along with gfxPrefs::APZAllowZooming() check [1].  And probably we also do the same thing in MobileViewportManager::RefreshVisualViewportSize().

[1] https://hg.mozilla.org/mozilla-central/file/e4220fa7a191/layout/base/MobileViewportManager.cpp#l413

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Gah, simply adding ShouldHandleMetaViewport check breaks a bunch of test cases which enable dom.meta-viewport.enabled.

Botond, is it an intentional behavior that MobileViewportManager is generated when the pref is true?  (I thought ShouldHandleMetaViewport just simply returns true when the touch simulator is enabled on RDM).  If so, we will figure out another way to solve this bug.

Comment 5

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> Gah, simply adding ShouldHandleMetaViewport check breaks a bunch of test
> cases which enable dom.meta-viewport.enabled.
> 
> Botond, is it an intentional behavior that MobileViewportManager is
> generated when the pref is true?  (I thought ShouldHandleMetaViewport just
> simply returns true when the touch simulator is enabled on RDM).  If so, we
> will figure out another way to solve this bug.

We need a MobileViewportManager for correct handling of a meta viewport tag, so we create one when touch simulation is enabled in RDM (when we want to start respecting the page's meta viewport tag).

Let me know if that answers your question. I don't understand the breakage you mention, if you go into more detail I might be able to suggest something.

Comment 6

[Brad Werth [:bradwerth]]

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

Comment 7

[Brad Werth [:bradwerth]]

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

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.

Comment 8

[Marco Mucci [:MarcoM]]

Hello Hiroyuki. Want to confirm if you are still working on this bug or if it can be reassigned to a member of the RDM Team? Thanks.

Comment 9

[Brad Werth [:bradwerth]]

I attempted to reproduce this bug and noticed a more severe problem that's appeared since the bug was filed. After returning from RDM, the page content is cropped to a much smaller area. According to mozregression, this new unwanted behavior appeared with the landing of Bug 1501665. That isn't terribly surprising since that bug changed so much.

Not changing the bug title yet because these results are probably related to the same cause. In other words, the scrollbars being cropped to a too-small rectangle (the original bug) is now joined by the viewport being cropped to a too-small rectangle.

Comment 10

[Brad Werth [:bradwerth]]

Attachment 8855130 [details] has meta viewport tag:

<meta name="viewport" content="width=250, initial-scale=0.01">

initial-scale is the likely issue for this problem. I don't think we have good test coverage of RDM with initial-scale.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

No, I am not actively working on this any more. Thanks!

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

Attachment 8855130 [details] has meta viewport tag:

<meta name="viewport" content="width=250, initial-scale=0.01">

initial-scale is the likely issue for this problem. I don't think we have good test coverage of RDM with initial-scale.

FWIW, the behavior for cases where initial-scale < 0.25 might be changed by bug 1550105, since the spec says non negative scale value is clamped to the range [0.1, 10], and we changed the value of the minimum boundary to 0.25 in bug 1510214.

Comment 12

[Brad Werth [:bradwerth]]

Still thinking this through. The call to nsPresContext::SetVisibleArea is getting correct values, but the visible area is getting scaled by the very high resolution value that was used by the RDM mode with meta viewport on. You can see the content cropping bug just by toggling touch simulation button (which also controls meta viewport).

Comment 13

[Brad Werth [:bradwerth]]

Attachment: Bug 1496609 Part 1: Prevent MobileViewportManager zoom scaling when display size hasn't changed.

Comment 14

[Brad Werth [:bradwerth]]

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

Comment 15

[Brad Werth [:bradwerth]]

This is another one that's difficult to test in an automated fashion. I'll request review of the patch as-is with an explanation of the difficulties.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

I've been wondering we can simulate RDM in our reftest harness.

Comment 17

[Brad Werth [:bradwerth]]

Hiro, would you please test that the attached patch solves the issue you found with the scrollbars? I was never really able to replicate that issue but instead found this screen cropping issue instead. I hope that the patch will resolve the issue you found, as well.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

It's been already fixed by you! Bug 1521814 fixed the issue I mentioned in comment 0. Thank you!

Comment 19

[Brad Werth [:bradwerth]]

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

It's been already fixed by you! Bug 1521814 fixed the issue I mentioned in comment 0. Thank you!

In that case, would you please retest with the pref devtools.responsive.metaViewport.enabled set? That's all that Bug 1521814 affected.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

With the pref set (true) the scrollbars in the example in question are not incorrectly rendered but initial-scale=0.1 seems not to be applied. But with the binary in the try in comment 14 the example looks fine to me.

Comment 21

[Brad Werth [:bradwerth]]

Attachment: Bug 1496609 Part 1: Make GetViewportInfo properly report viewport sizes when initial-scale is less than min-scale.

Comment 22

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=738e73098325d165bc93e0beae8719f558545cb1

Comment 23

[Brad Werth [:bradwerth]]

Attachment: Bug 1496609 Part 1: Add a test of getViewportInfo clamping initial-scale to min-scale.

Depends on D32321

Comment 24

[Brad Werth [:bradwerth]]

It appears this has been fixed by Bug 1550105.

Comment 25

[Botond Ballo [:botond]]

We might as well still land the test.

Comment 26

[Brad Werth [:bradwerth]]

I just came to the same conclusion. The tests for Bug 1550105 don't go through getViewportInfo, and that seems useful to test as well.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b337e189320
Part 1: Add a test of getViewportInfo clamping initial-scale to min-scale. r=botond

Comment 28

[Andrei Ciure[:aciure]]

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