Closed Bug 1423709 Opened 2 years ago Closed 11 months ago

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

Categories

(Core :: Layout, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox59 --- affected
firefox65 --- fixed

People

(Reporter: botond, Assigned: hiro)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [webcompat:p1][layout:p2])

Attachments

(7 files, 1 obsolete file)

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.
Priority: -- → P1
Could you provide some examples of such cases?
Flags: needinfo?(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.
Priority: P1 → P3
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."
> 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.
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.
Duplicate of this bug: 1336332
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
Summary: Make the choice of initial scale consistent with other browsers → Make the choice of initial / minimum scale same with Chrome?
FWIW, this is a minimal testcase based on the code in comment 3.
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
(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>.
> Firefox doesn't allow zooming in further.

I mean... zooming out.
Priority: P3 → P2
Whiteboard: [webcompat:p1] → [webcompat:p1][layout:p2]
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.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
https://webcompat.com/issues/9868

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


no meta viewport. Can scroll.
Firefox: zoomed in
Chrome: zoomed out
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 ゴシック", "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)
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.
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
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
https://webcompat.com/issues/7442

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

body {
	width: 380px;
	min-width: 380px;
	margin: 0 auto;
}
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);
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'.
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?
Duplicate of this bug: 1336292
(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?
Flags: needinfo?(bokan)
Attached patch Adapt initial-scale patch (obsolete) — Splinter Review
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.
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.
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.
Blocks: 1407582
Depends on: 1501106
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
Attachment #9018121 - Attachment is obsolete: true
Attachment #9020719 - Flags: feedback?(botond)
Attachment #9020719 - Attachment is patch: true
Attachment #9020719 - Attachment mime type: application/octet-stream → text/plain
(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 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.
Attachment #9020719 - Flags: feedback?(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.
Flags: needinfo?(bokan) → needinfo?(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.
Flags: needinfo?(mstange)
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?
Flags: needinfo?(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?
Flags: needinfo?(botond)
(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?
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
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
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
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
(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).
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:kats@mozilla.com) 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.
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.
(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.
Attachment #9021101 - Attachment description: Bug 1423709 - Add mAutoScale in nsViewportInfo. r=botond → Bug 1423709 - Set mDefaultZoomValid is false in the case of auto-scale. r=botond
Depends on: 1504360
Depends on: 1504362
Duplicate of this bug: 1476225
Depends on: 1508177
No longer depends on: 1504360
Pushed by hikezoe@mozilla.com:
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
Blocks: 1508177
No longer depends on: 1508177
Depends on: 1516322
Depends on: 1515113
Duplicate of this bug: 1178110
Depends on: 1552713
You need to log in before you can comment on or make changes to this bug.