1423709 - Make the choice of initial / minimum scale same with Chrome?

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P2)

Description

[Botond Ballo [:botond]]

Firefox will sometimes choose the initial scale (zoom level) for displaying a web page on a mobile device differently than Chrome or Safari.

As a result, pages can e.g. load zoomed in in Firefox when they load zoomed out in Chrome.

We should try to make the behaviour consistent here.

Note that this is much less severe than bug 1423013, where a page loads zoomed in *and* you can't scroll to see the rest of the page; that's a different issue.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Could you provide some examples of such cases?

Comment 2

[Botond Ballo [:botond]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #1)
> Could you provide some examples of such cases?

Yes, sorry, please see the linked webcompat issues. I meant to move them over from bug 1123938 but forgot.

It's possible that some of the other webcompat issues currently linked to bug 1123938 are this issue as well, I just haven't had a chance to go through all of them yet.

Comment 3

[Karl Dubost💡 :karlcow]

Xidorn, for example see https://webcompat.com/issues/8581#issuecomment-319244877

where the meta viewport is 

<meta name="viewport" content="width=device-width">

then html, body at 100%. This is good.

#contentcontainer, #leftcontainer, #totalcontainer, body, html {
	height: 100%;
	-webkit-text-size-adjust: none;
	font-size: 100%;
	min-height: 673px;
	width: 100%;
}


Then 

one element is set to

#contentcontainer, #leftcontainer, #totalcontainer {
	max-width: 675px !important;
	min-width: 675px;
}


Note that since the site has been changed. This is part of my Rules of Life in Webcompat

> "Wait long enough, every bug disappears."

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

> Note that since the site has been changed. This is part of my Rules of Life in Webcompat

Hmmm... It would always be helpful to have a minimal reproducible testcase I guess. That would help analyzing over time even if the original source has changed. We may need such testcase for adding test for changes eventually.

Comment 5

[Karl Dubost💡 :karlcow]

Xidorn, Definitely. 

I did this in 
http://la-grange.net/2016/12/01/viewport/viewport-test-0003.html

And this makes Bug 1336332 a duplicate of this one.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

So, the issue here is actually more about the minimum scale than the initial scale. Firefox doesn't allow zooming in further.

In addition, there are no "other browsers" at all. Chrome is the only browser which behaves differently than us on this specific issue. Safari also has an initial scale (and minimum scale) based on size of ICB rather than widest location.

Given the viewport explainer from Chromium team, I suppose they are looking at changing their behavior to align with Safari and us[1], because this behavior makes more sense, and they have other trouble with their current behavior.

I guess that probably means we probably should do nothing here, and maybe close it at some point.


[1] https://github.com/bokand/bokand.github.io/blob/master/web_viewports_explainer.md#minimum-scale

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase based on comment 3

FWIW, this is a minimal testcase based on the code in comment 3.

Comment 9

[Karl Dubost💡 :karlcow]

Xidorn, what do you think about the range of scales. 
http://la-grange.net/2016/12/01/viewport/index.html

Here too they vary in between browsers. The clamping values are not the same.
see Bug 1313264

Also Bug 1431601

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Karl Dubost :karlcow from comment #9)
> Xidorn, what do you think about the range of scales. 
> http://la-grange.net/2016/12/01/viewport/index.html
> 
> Here too they vary in between browsers. The clamping values are not the same.
> see Bug 1313264
> 
> Also Bug 1431601

That sounds like different issues than this one? I don't have much idea about scale specified in <meta>.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

> Firefox doesn't allow zooming in further.

I mean... zooming out.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Assigning to myself since I noticed that the test case in comment 8 is a bit different from the case what Chrome tries to fix and I will figure out what we can do for this bug and other webcompat issues in viewport handling.

Comment 13

[Karl Dubost💡 :karlcow]

https://webcompat.com/issues/9868

#main_container, #footer {
	width: 1050px;
	margin: 0 auto;
}


no meta viewport. Can scroll.
Firefox: zoomed in
Chrome: zoomed out

Comment 14

[Karl Dubost💡 :karlcow]

https://webcompat.com/issues/1677

<meta name="viewport" content="width=450">

```css
body {
	-webkit-text-size-adjust: 100%;
	margin: 0px;
	margin-left: auto;
	margin-right: auto;
	padding: 0px;
	spacing: 0px;
	width: 640px;
	font-family: "ヒラギノ角ゴ Pro W3", "Hiragino Kaku Gothic Pro", "メイリオ", Meiryo, Osaka, "ＭＳ ゴシック", "MS Gothic", sans-serif !important;
}
```

for a 360px viewport
Firefox: zoomed in (showing only 360px of the 450px )
Chrome : content zoomed out. html set to 450px, body is set to 640px width and margin-right of -190px (as shown in the devtools layout widget of the inspector)

Comment 15

[Karl Dubost💡 :karlcow]

https://webcompat.com/issues/13445
Tablet issue. 

```css
body {
	min-width: 1012px;
	word-wrap: break-word;
}
```
<meta name="viewport" content="width=device-width">


Assuming a nexus 7 size (600x960px)
Firefox: Zoomed in. html (showing 600 px on 1012px)
Chrome: Zoomed out. html = 600px body 1012px Adjusting the zoom on the body size.

Comment 16

[Karl Dubost💡 :karlcow]

https://webcompat.com/issues/14852

no meta viewport.

```
<div id="wrapper" style="width:1200px;">
```

Assuming 360px wide device
Firefox: zoomed in. showing 360px on the 1200px. html size is 980px, wrapper is 1200px
Chrome: zoomed out. showing 1200px. html = 980 px, wrapper is 1200px

Comment 17

[Karl Dubost💡 :karlcow]

Probably a difficult dependency in 
https://bugzilla.mozilla.org/show_bug.cgi?id=1469747#c1
for https://webcompat.com/issues/15180

Comment 18

[Karl Dubost💡 :karlcow]

https://webcompat.com/issues/16097

```js
 if(screen.width <= 480) { var mvp = document.getElementById('myViewport'); mvp.setAttribute('content','width=480');} 
```

and once it has been set 
<meta id="myViewport" name="viewport" content="width=480">

For a 360px device.
screen.width will be different in Firefox and Chrome (if I didn't mess up.)
Firefox: 480
Chrome: 360

Comment 19

[Karl Dubost💡 :karlcow]

https://webcompat.com/issues/7442

<meta name="viewport" content="width=device-width">

body {
	width: 380px;
	min-width: 380px;
	margin: 0 auto;
}

Comment 20

[Karl Dubost💡 :karlcow]

Another issue where the initial reported value of the browser will change the behavior of the layout
https://webcompat.com/issues/18292

<meta name="viewport" id="meta_viewport" content="device-width, initial-scale=1.29">

This relies on

		(function($) {
			var width = null;
			var scale = 0.85;
			var lock_zooming = false;

			var vp = $('#meta_viewport');

			var portrait_width, landscape_width;
			if (screen.width > screen.height) {
				portrait_width = screen.height;
				landscape_width = screen.width;
			} else {
				portrait_width = screen.width;
				landscape_width = screen.height;
			}

			function setViewport() {

				width = 760;


				var sw;

				if (typeof(window.orientation) == 'undefined') {
					sw = (screen.height > screen.width) ? portrait_width : landscape_width;
				} else {
					sw = Math.abs(window.orientation) == 90 ? landscape_width : portrait_width;
				}

				if (sw < 489) {
					width = 489;
				} else if (sw > 760) {
					width = 760;
				}

				var old_scale = scale;
				scale = Math.ceil((sw / width) * 100)/100;

				// sets the initial zooming and locks it so the browser can't auto-zoom on the user
				vp.attr('content', "device-width, initial-scale="+scale+", maximum-scale="+scale+", minimum-scale="+scale);

				if (old_scale != scale) {
					$.post(
						"/setvisitorcookie",
						{
							namespace: "mobile_viewport",
							name: "scale",
							value: scale,
							expires: "+1 year"
						},
						function(result) {
							if (!result.success) {
								alert(result.msg);
							}
						},
						"json"
					 );
				}

				if (lock_zooming) return;
				setTimeout(function() {
					// releases the zoom locking so users can pinch-zoom again
					vp.attr('content', "device-width, initial-scale="+scale);
				}, 600); // accounts for rotation animations on certain phones
			}

			window.addEventListener("orientationchange", function() {
				setViewport();
			}, false);

			$(function() {
				setViewport();
			});
		})(jQuery);

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Great work, Karl!

At least https://webcompat.com/issues/18292 is an issue related to 'extend-to-zoom' (bug 1494422), the site works fine with my current WIP patch for the 'extend-to-zoom'.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

I think now I understand what Chrome does.  What Chrome does is;

1) Resolve values in meta viewport tag, e.g. width, initial-scale, etc.
2) Do a layout process with the resolved values (I guess, at this moment, initial-scale=1 is used in the case where initial-scale is not specified)
3) Compute minimum-scale with layouted content width and the screen size [1]
4) Use the minimum-scale for initial-scale if no initial-scale is specified

And I guess Safari seemed to do a similar thing on iOS9 but even if initial-scale is specified [2], but soon after that, they did restrict the behavior only in split-screen mode [3].

[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/frame/page_scale_constraints.cc?l=84&rcl=eb25f32f656f7c3c4e58b0b384647428cb13e919
[2] https://developer.apple.com/library/archive/releasenotes/General/WhatsNewInSafari/Articles/Safari_9_0.html#//apple_ref/doc/uid/TP40014305-CH9-SW36
[3] https://developer.apple.com/library/archive/releasenotes/General/WhatsNewInSafari/Articles/Safari_9_1.html#//apple_ref/doc/uid/TP40014305-CH10-SW6

Anyway, the Chrome behavior sounds reasonable, I am going to try to implement it.  What I am now concerned is that this need to trigger another reflow, but it's acceptable?

Comment 24

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #22)
> Anyway, the Chrome behavior sounds reasonable, I am going to try to
> implement it.  What I am now concerned is that this need to trigger another
> reflow, but it's acceptable?

Let's double check with David Bokan from the Chrome team, to see if we are understanding correctly: is Chrome doing two reflows here?

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Attachment: Adapt initial-scale patch

Thank you, Botond, but I might misunderstand the reflow thing.  Actually I did write a very ugly hacky way to re-compute initial-scale based on the content width (though it currently includes overflow:hidden regions because I haven't found a handy way to get the width)

Anyway, I'd really appreciate any feedback from David.
Thanks!

FWIW, here is the patch.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Oops, I missed an important sentence.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #25)
> Created attachment 9018121 [details] [diff] [review]
> Adapt initial-scale patch
> 
> Thank you, Botond, but I might misunderstand the reflow thing.  Actually I
> did write a very ugly hacky way to re-compute initial-scale based on the
> content width (though it currently includes overflow:hidden regions because
> I haven't found a handy way to get the width)

Actually it seems the way works fine without the additional reflow at least on Responsive Design Mode.

Comment 27

[David Bokan]

The description in #22 is accurate w.r.t. Chrome. We don't need to do a second full reflow, just a special case just for position: fixed elements attached to the viewport, if I recall correctly.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

Attachment: Adapt initial zoom value to fit the wider contents to screen device width

Botond, I'd like to make sure whether I am on the right track or not.

Here is the main part of the patch for this bug.  What the patch does is that just scaling down the wider document as if the document was pinched out to fit the contents to device screen width.

What I am not convinced yet is the way to get the content width. Currently I am calling nsIFrame::GetVisualOverflowRect() for nsCanvasFrame, but it seems there is a case it doesn't work.  Actually a marionette test,
testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py TestVisibility.testShouldClickOnELementPartiallyOffRight, fails with this patch 
[1].  The test case contains a position: absolute element whose width is 100px and right is -50px, but the whole element seems to be painted there.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e8e2792cadedfca4b72d116fd0e9325fa6c9b5d&selectedJob=208316019

Comment 29

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> What I am not convinced yet is the way to get the content width. Currently I
> am calling nsIFrame::GetVisualOverflowRect() for nsCanvasFrame, but it seems
> there is a case it doesn't work.  Actually a marionette test,
> testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py
> TestVisibility.testShouldClickOnELementPartiallyOffRight, fails with this
> patch 
> [1].  The test case contains a position: absolute element whose width is
> 100px and right is -50px, but the whole element seems to be painted there.

I just realized that Chrome also paints the whole element.  I did use https://hiikezoe.github.io/test-viewport-3.html .

Comment 30

[Botond Ballo [:botond]]

Comment on attachment 9020719 [details] [diff] [review]
Adapt initial zoom value to fit the wider contents to screen device width

Review of attachment 9020719 [details] [diff] [review]:
-----------------------------------------------------------------

My understanding is that Chrome chooses the initial scale based on the "minimum scale width", that is, the width of the visual viewport when we're zoomed out as far as you can.

I think that's equivalent to using the content width, but clamping the resulting scale to the min-scale, which I don't see us doing in this patch.

Comment 31

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #28)
> What I am not convinced yet is the way to get the content width. Currently I
> am calling nsIFrame::GetVisualOverflowRect() for nsCanvasFrame, 

I think you may want to use GetScrollableOverflowRect(), so we don't include areas clipped away by overflow:hidden?

Once upon a time, an earlier version of the code that today lives in MobileViewportManager used Element::ScrollWidth() / Element::ScrollHeight() on the <html> or <body> element [1] to compute the content size.

Anyways, best ask Markus about this.

[1] https://hg.mozilla.org/mozilla-central/rev/b501e96201ef#l1.104

> but it seems
> there is a case it doesn't work.  Actually a marionette test,
> testing/marionette/harness/marionette_harness/tests/unit/test_visibility.py
> TestVisibility.testShouldClickOnELementPartiallyOffRight, fails with this
> patch 

I haven't looked at the test case, but since we're making a behaviour change, it's possible that some test expectations on Android will need to be updated.

Comment 32

[Markus Stange [:mstange]]

I suggest using 

nsIScrollableFrame* rootScrollFrame = mPresShell->GetRootScrollFrameAsScrollable();
if (rootScrollableFrame) {
  nsRect scrollableRect = rootScrollFrame->GetScrolledRect();
  // ...
}

Botond is right, the visual overflow rect contains additional areas that do not contribute to the scrolled rect. For example, on
data:text/html,<div style="width:500px;box-shadow:3000px 0 cyan">Hello
the visual overflow rect includes the box-shadow, but the scrollable overflow rect does not.
ScrollFrameHelper::GetScrolledRect() uses the scrolled frame's scrollable overflow rect as the basis for computing the scrolled rect.

Comment 33

[Hiroyuki Ikezoe (:hiro)]

Thank you, Botond and Markus for the feedback.  I did change locally the code to use GetRootScrollFrameAsScrollable and GetScrolledRect.

(In reply to Botond Ballo [:botond] from comment #30)
> My understanding is that Chrome chooses the initial scale based on the
> "minimum scale width", that is, the width of the visual viewport when we're
> zoomed out as far as you can.

Ah I see.  Now I am pretty sure that the failure test case should have 'initial-scale=1' to avoid shrinking the contents.

> I think that's equivalent to using the content width, but clamping the
> resulting scale to the min-scale, which I don't see us doing in this patch.

Yes, in my understanding, it's the final part we have to do here in this bug.  To make the clamp work, is it a reasonable way to let MobileViewportManager have ZoomConstraintsClient instead of nsPresShell having ZoomConstraintsClient?  That's because with the new setup we can't tell the minimum-scale value until reflows finish.  I mean, what I have in my mind is that we call ZoomConstraintsClient::RefreshZoomConstraints at the end of MobileViewportManager::RefreshViewportSize. Botond, any insights on this way?

Comment 34

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #33)
> That's because with the new setup we can't
> tell the minimum-scale value until reflows finish.

Why is that - are we changing how nsViewportInfo::mMinScale is computed?

Comment 35

[Hiroyuki Ikezoe (:hiro)]

(In reply to Botond Ballo [:botond] from comment #34)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #33)
> > That's because with the new setup we can't
> > tell the minimum-scale value until reflows finish.
> 
> Why is that - are we changing how nsViewportInfo::mMinScale is computed?

Yes, part of.  I mean, I am not going to change the value returned by nsIDocument::GetViewportInfo(), the value is a result before reflow happens.  What I am going to do is to give a modified viewport info to ZoomConstraintsClient after reflow finished.  Otherwise, I think, we end up doing at least a reflow in nsIDocument::GetViewportInfo(), no?

Comment 36

[Botond Ballo [:botond]]

I don't think the value of ZoomConstraintsClient::mMinScale needs to depend on the outcome of reflow, either. APZ knows to additionally clamp the zoom level based on the content size [1].

[1] https://searchfox.org/mozilla-central/rev/8848b9741fc4ee4e9bc3ae83ea0fc048da39979f/gfx/layers/apz/src/AsyncPanZoomController.cpp#1576

Comment 37

[Hiroyuki Ikezoe (:hiro)]

I've added some reftests including Xidorn's test case in comment 8, Markus' test case in 32 and a test case that Botond told me on IRC (clamping the initial scale by minimum-scale).  They work fine locally on Linux64 but some of them fail on WebRender for some reasons. Initially I though they fail on Android, but all of them work fine on Android.  Here is the link to the failure log.
https://treeherder.mozilla.org/logviewer.html#?job_id=208557389&repo=try&lineNumber=37985
Anyway, I am going to mark fails-if for WebRender and take a look into it in a follow-up bug.

Here is another try disabling them on WebRender.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d83096b6d23eb8ca9a36674329e405c165b9945f

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1423709 - Change boolean arguments for nsViewportInfo to enum classes. r=botond

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1423709 - Set mDefaultZoomValid is false in the case of auto-scale. r=botond

Depends on D10194

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1423709 - Add initial-scale=1 for tests supposing the page is rendered 1:1 scale. r=botond

Depends on D10195

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1423709 - Adapt initial zoom value to fit the wider contents to screen device width. r=botond

What we need to do here is to scale down the document as if the document was pinched in
to fit the contents to screen width when all the following conditions are met.

 - it's the first paint for the document
 - no initial-scale is specified in meta viewport tags
 - no mRestoreResolution hasn't set
 - the content width is wider than the initial viewport width

Depends on D10196

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1423709 - Reftests for auto initial zoom value calculation. r=botond

There are 5 test cases in this commit.

 - no-viewport.html
   A test case that wider contents should be scaled down even if no viewport
   meta tag exists.

 - viewport-width.html
   A test case that wider contents should be scaled down (i.e. fit to the
   device screen).

 - minimum-scale.html
   A test case that the initial zoom value is clamped by minimum-scale.

 - initial-scale-1.html
   A test case that the auto initial zoom calculation doesn't apply to the
   documents specifying initial-scale.

 - box-shadow.html
   A test case that box-shadow-ed area isn't incorporated into the initial zoom
   value.

Depends on D10197

Comment 43

[Kartikaya Gupta (email:[e-mail])]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #37)
> Anyway, I am going to mark fails-if for WebRender and take a look into it in
> a follow-up bug.

Does it fail on WebRender just on Linux? Or Windows? If it fails on Windows I would be very strongly against marking it fails-if without first doing a preliminary investigation to understand why it happens and whether it will affect any user-visible scenarios (responsive design mode in devtools counts as a user-visible scenario).

Comment 44

[Hiroyuki Ikezoe (:hiro)]

They failed on Linux at least, I haven't run them on Windows yet.
Let's see the results on other platforms.

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #43)
> whether it will
> affect any user-visible scenarios (responsive design mode in devtools counts
> as a user-visible scenario).

I am not worried about it too much since the change here doesn't affect on normal RDM, it requires setting apz.allow_zooming true.  And it seems to me that RDM has already been broken with apz.allow_zooming.

Comment 45

[Hiroyuki Ikezoe (:hiro)]

The failure on WebRender is caused by a pre-existing bug, I filed bug 1503428 for that (it seems either dom.meta-viewport.enabled or apz.allow_zooming doesn't work well on WebRender).

That's said, some reftests still fail on non WebRender on Windows (https://treeherder.mozilla.org/logviewer.html#?job_id=208628178&repo=try&lineNumber=86407), I am going to investigate the failure reason.

Comment 46

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #45)
> That's said, some reftests still fail on non WebRender on Windows
> (https://treeherder.mozilla.org/logviewer.
> html#?job_id=208628178&repo=try&lineNumber=86407), I am going to investigate
> the failure reason.

This also seems to be another pre-existing bug, filed bug 1503514.

Comment 48

[Hiroyuki Ikezoe (:hiro)]

A final try;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bd78ea91e44a01a0219821b8b109ab4ad748b7e

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e00d7edbb963
Change boolean arguments for nsViewportInfo to enum classes. r=botond
https://hg.mozilla.org/integration/autoland/rev/e5485cbe65cd
Set mDefaultZoomValid is false in the case of auto-scale. r=botond
https://hg.mozilla.org/integration/autoland/rev/e41abff7fff9
Add initial-scale=1 for tests supposing the page is rendered 1:1 scale. r=botond
https://hg.mozilla.org/integration/autoland/rev/9f9d8ca6b972
Adapt initial zoom value to fit the wider contents to screen device width. r=botond
https://hg.mozilla.org/integration/autoland/rev/efb925fd7a3c
Reftests for auto initial zoom value calculation. r=botond

Comment 50

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/e00d7edbb963
https://hg.mozilla.org/mozilla-central/rev/e5485cbe65cd
https://hg.mozilla.org/mozilla-central/rev/e41abff7fff9
https://hg.mozilla.org/mozilla-central/rev/9f9d8ca6b972
https://hg.mozilla.org/mozilla-central/rev/efb925fd7a3c