meta viewport and initial-scale=0 should be clamped to 0.25

NEW
Unassigned

Status

()

Core
Layout
P3
normal
2 years ago
28 days ago

People

(Reporter: karlcow, Unassigned)

Tracking

(Blocks: 2 bugs)

48 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [webcompat])

Attachments

(7 attachments)

(Reporter)

Description

2 years ago
When trying to understand Web Compatibility issues about meta viewport, we seem to not not have the proper algorithm for meta viewport initial-scale value.

<meta name="viewport" content="width=device-width, initial-scale=0">

Currently the draft spec says:
https://drafts.csswg.org/css-device-adapt/#min-scale-max-scale


> The properties are translated into 'zoom', 'min-zoom', 
> and 'max-zoom' respectively with the following translations of values.
> 
>  1. Non-negative number values are translated to <number> values, clamped to the range [0.1, 10]
>    2. Negative number values are dropped
>    3. yes is translated to 1
>    4. device-width and device-height are translated to 10
>    5. no and unknown values are translated to 0.1 
> 
> For a viewport <META> element that translates into an @viewport 
> rule with no max-zoom declaration and a non-auto min-zoom value 
> that is larger than the max-zoom value of the UA stylesheet, 
> the min-zoom declaration value is clamped to the UA stylesheet 
> max-zoom value.

If I interpret this correctly we should be clamping the value to 0.1 aka

<meta name="viewport" content="width=device-width, initial-scale=0.1">

(Maybe I missed it)

See Also: 
https://github.com/whatwg/compat/issues/62
https://webcompat.com/issues/2293
(Reporter)

Updated

2 years ago
Whiteboard: [webcompat]
(Reporter)

Updated

a year ago
Blocks: 1323062
Can we look at Chrome and see what they are doing?
Anything I can do to help move this along?

Do we need to build some testcases for other browsers?
Yeah, first thing that needs to happen is figuring out what other browsers (probably Chrome and Safari are most relevant) are doing here.
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #3)
> Yeah, first thing that needs to happen is figuring out what other browsers
> (probably Chrome and Safari are most relevant) are doing here.

I think Karl has done a lot of cross-browser research here.
Flags: needinfo?(kdubost)
(Reporter)

Comment 5

a year ago
1. Yes, there are clamping values.
2. It's difficult to understand the full code.
I spent last week a bit of time in source code. 
On WebKit if I understand the code right, there are clamping values


Let me add a couple of tests. 
http://www.la-grange.net/2016/12/01/viewport/index.html


Here pages with the following conditions.
1. The biggest element in the page is an element which is 200px wide
2. The width is set at 250
   <meta name="viewport" content="width=250, initial-scale=1.0">
3. we change the initial-scale
  * test 0010 - width=250, initial-scale=1.0
    http://www.la-grange.net/2016/12/01/viewport/viewport-test-0010.html
  * test 0011 - width=250, initial-scale=0.5
    http://www.la-grange.net/2016/12/01/viewport/viewport-test-0011.html
  * test 0012 - width=250, initial-scale=0.1
    http://www.la-grange.net/2016/12/01/viewport/viewport-test-0012.html
  * test 0013 - width=250, initial-scale=0.01
    http://www.la-grange.net/2016/12/01/viewport/viewport-test-0013.html


==============
Results: 
==============
# iOS 10.2.1, Safari 10 (602.1) [iPodTouch]
  window.devicePixelRatio = 2
  window.screen.width = 320px

* test 010 - initial-scale=1.0
  document.documentElement.clientWidth = 320px
* test 011 - initial-scale=0.5
  document.documentElement.clientWidth = 640px  (320/640 = 0.5)
* test 012 - initial-scale=0.1
  document.documentElement.clientWidth = 1280px (320/1280 = 0.25)
* test 013 - initial-scale=0.01
  document.documentElement.clientWidth = 1280px (320/1280 = 0.25)
 
The value is clamped at 0.25

Their test
https://github.com/WebKit/webkit/blob/c595dc9b3993d095e25311b0ec1797bd665447e8/LayoutTests/fast/viewport/viewport-35.html
https://github.com/WebKit/webkit/blob/c595dc9b3993d095e25311b0ec1797bd665447e8/LayoutTests/fast/viewport/viewport-35-expected.txt

And indeed in 
https://github.com/WebKit/webkit/blob/0d326ed82bcfa89da5e4078641049999d4446ac5/Source/WebCore/page/ViewportConfiguration.cpp#L220-L230

ViewportConfiguration::Parameters ViewportConfiguration::webpageParameters()
{
    Parameters parameters;
    parameters.width = 980;
    parameters.widthIsSet = true;
    parameters.allowsUserScaling = true;
    parameters.allowsShrinkToFit = true;
    parameters.minimumScale = 0.25;
    parameters.maximumScale = 5;
    return parameters;
}


to note that they have
ViewportConfiguration::Parameters ViewportConfiguration::textDocumentParameters()
with parameters.minimumScale = 0.25;

BUT (though it doesn't change anything for images inside documents.)
ViewportConfiguration::Parameters ViewportConfiguration::imageDocumentParameters()
with parameters.minimumScale = 0.01;


They also have this but I haven't figured out yet when it was coming into play.
https://github.com/WebKit/webkit/blob/0d326ed82bcfa89da5e4078641049999d4446ac5/Source/WebCore/page/ViewportConfiguration.cpp#L312-L313

void ViewportConfiguration::updateConfiguration()
{
    m_configuration = m_defaultConfiguration;

    const double minimumViewportArgumentsScaleFactor = 0.1;
    const double maximumViewportArgumentsScaleFactor = 10.0;

    bool viewportArgumentsOverridesInitialScale;
    bool viewportArgumentsOverridesWidth;
    bool viewportArgumentsOverridesHeight;
…



In the original code for introducing 0.25, we get the following comment
https://github.com/WebKit/webkit/blame/677689d1ffa397b77634ef32c37574d46961815c/Source/WebCore/page/ViewportConfiguration.cpp#L45-L46


    // Setup a reasonable default configuration to avoid computing infinite scale/sizes.
    // Those are the original iPhone configuration.



# Android 4.4.4 Chrome 56
  window.devicePixelRatio = 3
  window.screen.width = 360px

* test 010 - initial-scale=1.0
  document.documentElement.clientWidth = 360px
* test 011 - initial-scale=0.5
  document.documentElement.clientWidth = 720px  (360/720 = 0.5)
* test 012 - initial-scale=0.1
  document.documentElement.clientWidth = 1440px (360/1440 = 0.25)
* test 013 - initial-scale=0.01
  document.documentElement.clientWidth = 1280px (360/1280 = 0.25)


# Android 4.4.4 Firefox 54
  window.devicePixelRatio = 3
  window.screen.width = 360px

* test 010 - initial-scale=1.0
  document.documentElement.clientWidth = 360px
* test 011 - initial-scale=0.5
  document.documentElement.clientWidth = 720px  (360/720 = 0.5)
* test 012 - initial-scale=0.1
  document.documentElement.clientWidth = 3600px (360/3600 = 0.1)
* test 013 - initial-scale=0.01
  document.documentElement.clientWidth = 250px (360/250 = 1.44)



Let me modify my title for this bug. 
Also under 0.1, it seems Firefox looses control… and ignore initial-scale and comes back to the width specified in the meta viewport. Here 250px. o_O
Flags: needinfo?(kdubost)
Summary: meta viewport and initial-scale=0 should be clamped to 0.1 → meta viewport and initial-scale=0 should be clamped to 0.25
(Reporter)

Comment 6

a year ago
If I'm reading the right thing:
https://dxr.mozilla.org/mozilla-central/rev/e150eaff1f83e4e4a97d1e30c57d233859efe9cb/dom/base/nsViewportInfo.h#15-18

static const mozilla::LayoutDeviceToScreenScale kViewportMinScale(0.1f);
static const mozilla::LayoutDeviceToScreenScale kViewportMaxScale(10.0f);
static const mozilla::CSSIntSize kViewportMinSize(200, 40);
static const mozilla::CSSIntSize kViewportMaxSize(10000, 10000);

Then
https://dxr.mozilla.org/mozilla-central/rev/e150eaff1f83e4e4a97d1e30c57d233859efe9cb/dom/base/nsViewportInfo.h#71-76

     /**
     * Constrain the viewport calculations from the
     * nsIDocument::GetViewportInfo() function in order to always return
     * sane minimum/maximum values.
     */
    void ConstrainViewportValues();

which goes here
https://dxr.mozilla.org/mozilla-central/source/dom/base/nsViewportInfo.cpp#14

I would love to understand what is happening for values under 0.1
dbaron: So it looks like we have our data. 0.25 it is. What do we change here?

I tried changing

static const mozilla::LayoutDeviceToScreenScale kViewportMinScale(0.1f);
to
static const mozilla::LayoutDeviceToScreenScale kViewportMinScale(0.25f);

But that didn't work. Clearly I don't understand this code :)
Flags: needinfo?(dbaron)
(Reporter)

Comment 9

a year ago
This part of the code seems also to play in terms of zoom.
https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#7611
So I don't know much about that code either, so redirecting needinfo to kats, who seems to have at least a bit of the hg blame for nsDocument::GetViewportInfo.
Flags: needinfo?(dbaron) → needinfo?(bugmail)
(In reply to Karl Dubost :karlcow from comment #6)
> which goes here
> https://dxr.mozilla.org/mozilla-central/source/dom/base/nsViewportInfo.cpp#14
> 
> I would love to understand what is happening for values under 0.1

So yes, that's (part of) the relevant code. What happens for values under 0.1 is that we set mDefaultZoom back to 0.1, but we also set mDefaultZoomValid = false. And when we do that, we later use a different algorithm [1] to pick the initial scale.

Reading the spec you quoted in comment 0, it looks like we should be clamping to 0.1 in this case, which could probably be achieved by just not setting mDefaultZoomValid = false in the first place. If that fixes this case but doesn't make anything worse I'd be fine with making that change. Do you have cycles to try it out? If not let me know and I can push a build to tryserver with this change.

[1] http://searchfox.org/mozilla-central/rev/d4adaabd6d2f88be0d0b151e022c06f6554909da/layout/base/MobileViewportManager.cpp#220
Flags: needinfo?(bugmail)
I can try this out locally.
Created attachment 8844948 [details] [diff] [review]
Patch that isn't working

This is the patch I tried, but wsj.com is still showing oversized instead of fitting in the viewport.

Am I missing something?
Attachment #8844948 - Flags: feedback?(bugmail)
Comment on attachment 8844948 [details] [diff] [review]
Patch that isn't working

This patch looks correct as far as I can tell. Can you uncomment the MVM_LOG at the top of layout/base/MobileViewportManager.cpp and load the page with that? The log might shed some more on what's going on.
Attachment #8844948 - Flags: feedback?(bugmail)
Created attachment 8844989 [details]
MVM log

For this log, I loaded wsj.com, cleared the log and then did a reload.

It's definitely showing the mobile version of wsj, just very wide.
Attachment #8844989 - Attachment mime type: text/x-log → text/plain
Wait, why are we looking at wsj.com? That seems to have an initial-scale of 1.0001, at least when I check on desktop with UA spoofing/RDM. I thought http://www.la-grange.net/2016/12/01/viewport/viewport-test-0013.html was the relevant test here, because that one has initial-scale=0.01 (which is less than the allowed minimum).
The original bug that pointed me over here was:

https://bugzilla.mozilla.org/show_bug.cgi?id=1323062#c17

So I thought we were looking at the same issue.

So I guess we have two separate issues?
We have a number of different viewport issues. I don't think the wsj viewport issue is the same as this one. That one (if I'm reading it correctly, we treat 1.0001 very differently from 1.0?) seems pretty weird, and doesn't seem like intended behaviour.
(Reporter)

Comment 19

a year ago
(drifting)
The 1.0001 issue is something different that we need to treat separately. :)
I'm still investigating. From what I have gathered right now, it is due to a code change in iOS between 8 and 9 (an epsilon value for fuzzy comparison, it's all crazy ). And the Web developers wanted to previous behavior, but it creates Web compat issues down the road. 

(back on track)
Here we are talking about the min-value clamping and that any values inferior to the min clamping value should not trigger weird behavior (like we see currently in Firefox)
The spec says 0.1 but probably should say 0.25 as both Chrome and iOS clamps it at 0.25.
I intend to send a comment to the spec developer. 

Thanks kats for catching this.
Thanks mike for working on this patch. 

:)
It looks like la-grange.net went away. Any other site to see this behavior?
Flags: needinfo?(kdubost)
(Reporter)

Comment 21

a year ago
Created attachment 8855127 [details]
viewport-test-0010.html
Flags: needinfo?(kdubost)
(Reporter)

Comment 22

a year ago
Created attachment 8855128 [details]
viewport-test-0011.html
(Reporter)

Comment 23

a year ago
Created attachment 8855129 [details]
viewport-test-0012.html
(Reporter)

Comment 24

a year ago
Created attachment 8855130 [details]
viewport-test-0013.html
(Reporter)

Comment 25

a year ago
Uploaded the test cases here. :)
(YES my server is down for a couple of days. Moving machines.  Sorry about that)
Created attachment 8859367 [details]
Screenshot after patch

So I've finally had the chance to do some testing with these. I do see different results with the patch, but they still don't match chrome.

See the attached.

So there's definitely more going on here...

Updated

11 months ago
Priority: -- → P3
(Reporter)

Updated

28 days ago
Blocks: 1123938
You need to log in before you can comment on or make changes to this bug.