Zoom instead of autosizing for desktop sites.

Replace text autosizing with page zoom in some cases.

This applies the device scale adjustment as a reduction in layout width, so that text autosizing is not needed.

It mainly affects desktop sites on landscape tablets, where the layout width can be reduced but remain >= 980px.

BUG=135869

[skobes, 2013-11-07 01:16:24]

This is the second half of proposal #4 in http://goo.gl/CcNYcf.

[aelias_OOO_until_Jul13, 2013-11-07 02:05:25]

https://codereview.chromium.org/61593006/diff/1/Source/web/PageScaleConstrain...
File Source/web/PageScaleConstraintsSet.cpp (right):

https://codereview.chromium.org/61593006/diff/1/Source/web/PageScaleConstrain...
Source/web/PageScaleConstraintsSet.cpp:190: if (width > minimumWidth &&
!viewportDescription.isSpecifiedByAuthor() && deviceScaleAdjustment > 1.0f)
Can we delete the condition "deviceScaleAdjustment > 1.0f"?  If <1 values are
bad then we should avoid producing them in GetDeviceScaleAdjustment() on the
Chromium side in the first place.

https://codereview.chromium.org/61593006/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/61593006/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:2999: ViewportDescription adjustedDescription =
description;
Could you mutate this value instead of changing the PageScaleConstraintsSet
post-facto?

https://codereview.chromium.org/61593006/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:3017:
m_pageScaleConstraintsSet.applyDeviceScaleAdjustment(adjustedDescription,
page()->settings().deviceScaleAdjustment(),
page()->settings().layoutFallbackWidth());
layoutFallbackWidth setting is due for removal soon, we shouldn't add another
dependency on it.  The value 980 is now specified by
"@viewport{min-width:980px}" in UA CSS files.  It doesn't look like you need it
either, since you can expect m_pageDefinedConstraints.layoutSize.width() to
already be 980 or higher if !viewportDescription.isSpecifiedByAuthor().

[aelias_OOO_until_Jul13, 2013-11-07 02:07:50]

Aside from the details above, I have the higher-level concern that I'm not sure
this will really do anything much of the time.  A lot of desktop sites already
have elements forcing the page size to be 980 or higher so it will just end up
being a no-op.  Maybe the real way to fix this is to introduce full-page zoom
using the same codepaths used when you press Ctrl-+ on desktop Chrome?  That
more forcibly lowers the layout width.

[skobes, 2013-11-07 17:55:26]

You're correct that this will have no effect in most cases (only large tablets
in landscape mode).

I was assuming we don't want layout width to drop below 980px, and we don't want
to zoom closer than the layout width (or else we will get horizontal
scrollbars), so we are pretty constrained in what can be done here.

If widths below 980px are acceptable we can tweak the constant and possibly get
more benefit from this.  But I don't know how many sites would look broken at
narrower widths... do you know if we have any data on this?

https://codereview.chromium.org/61593006/diff/1/Source/web/PageScaleConstrain...
File Source/web/PageScaleConstraintsSet.cpp (right):

https://codereview.chromium.org/61593006/diff/1/Source/web/PageScaleConstrain...
Source/web/PageScaleConstraintsSet.cpp:190: if (width > minimumWidth &&
!viewportDescription.isSpecifiedByAuthor() && deviceScaleAdjustment > 1.0f)
On 2013/11/07 02:05:25, aelias wrote:
> Can we delete the condition "deviceScaleAdjustment > 1.0f"?  If <1 values are
> bad then we should avoid producing them in GetDeviceScaleAdjustment() on the
> Chromium side in the first place.

Done.

https://codereview.chromium.org/61593006/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/61593006/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:3017:
m_pageScaleConstraintsSet.applyDeviceScaleAdjustment(adjustedDescription,
page()->settings().deviceScaleAdjustment(),
page()->settings().layoutFallbackWidth());
On 2013/11/07 02:05:25, aelias wrote:
> layoutFallbackWidth setting is due for removal soon, we shouldn't add another
> dependency on it.  The value 980 is now specified by
> "@viewport{min-width:980px}" in UA CSS files.  It doesn't look like you need
it
> either, since you can expect m_pageDefinedConstraints.layoutSize.width() to
> already be 980 or higher if !viewportDescription.isSpecifiedByAuthor().

It's needed to avoid reducing the width below 980px.  Changed to a constant.

[pdr., 2013-11-07 22:00:26]

LGTM but please wait for an LGTM from aelias as well.

[aelias_OOO_until_Jul13, 2013-11-07 22:02:50]

> If widths below 980px are acceptable we can tweak the constant and possibly
get
more benefit from this.  But I don't know how many sites would look broken at
narrower widths... do you know if we have any data on this?

We don't have any data.  My intuition is that 99% of sites would be fine but
there may be a 1% of important sites that may have some issues.  So I'm inclined
to agree, let's keep the minimum for now and possibly revisit in a later patch. 
Just have some minor implementation comments then.

https://codereview.chromium.org/61593006/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/61593006/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:2999: ViewportDescription adjustedDescription =
description;
On 2013/11/07 02:05:25, aelias wrote:
> Could you mutate this value instead of changing the PageScaleConstraintsSet
> post-facto?

Looks like you missed this comment, please follow up.

https://codereview.chromium.org/61593006/diff/70001/Source/web/PageScaleConst...
File Source/web/PageScaleConstraintsSet.cpp (right):

https://codereview.chromium.org/61593006/diff/70001/Source/web/PageScaleConst...
Source/web/PageScaleConstraintsSet.cpp:42: static const float minimumLayoutWidth
= 980.0f;
ViewportDescription.minWidth.value() should always be 980 when on Android and
!isSpecifiedByAuthor().  Could you use that instead to avoid duplicating the
magic number?

https://codereview.chromium.org/61593006/diff/70001/Source/web/PageScaleConst...
Source/web/PageScaleConstraintsSet.cpp:191: if (width > minimumLayoutWidth &&
!viewportDescription.isSpecifiedByAuthor())
Please delete the condition "width > minimumLayoutWidth" as it does not change
any behavior (redundant with std::max()).

[skobes, 2013-11-08 03:26:46]

https://codereview.chromium.org/61593006/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/61593006/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:2999: ViewportDescription adjustedDescription =
description;
On 2013/11/07 22:02:50, aelias wrote:
> On 2013/11/07 02:05:25, aelias wrote:
> > Could you mutate this value instead of changing the PageScaleConstraintsSet
> > post-facto?
> 
> Looks like you missed this comment, please follow up.

I did miss this, sorry.

Unfortunately I don't think I can do it that way... the value we need to adjust
is the layoutSize inside the PageScaleConstraints object, which is computed by
ViewportDescription::resolve but not actually stored in the ViewportDescription.

https://codereview.chromium.org/61593006/diff/70001/Source/web/PageScaleConst...
File Source/web/PageScaleConstraintsSet.cpp (right):

https://codereview.chromium.org/61593006/diff/70001/Source/web/PageScaleConst...
Source/web/PageScaleConstraintsSet.cpp:42: static const float minimumLayoutWidth
= 980.0f;
On 2013/11/07 22:02:50, aelias wrote:
> ViewportDescription.minWidth.value() should always be 980 when on Android and
> !isSpecifiedByAuthor().  Could you use that instead to avoid duplicating the
> magic number?

Done.

https://codereview.chromium.org/61593006/diff/70001/Source/web/PageScaleConst...
Source/web/PageScaleConstraintsSet.cpp:191: if (width > minimumLayoutWidth &&
!viewportDescription.isSpecifiedByAuthor())
On 2013/11/07 22:02:50, aelias wrote:
> Please delete the condition "width > minimumLayoutWidth" as it does not change
> any behavior (redundant with std::max()).

Done.

[aelias_OOO_until_Jul13, 2013-11-08 04:06:16]

OK, looks good.  One more thing, could you write a test in WebFrameTest.cpp
checking the width is reduced as expected and that 980px is the minimum?  There
are several similar tests in that file you can base it on.  (Sorry for
forgetting to request this in the last review.)

https://codereview.chromium.org/61593006/diff/1/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/61593006/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:2999: ViewportDescription adjustedDescription =
description;
On 2013/11/08 03:26:47, skobes wrote:
> Unfortunately I don't think I can do it that way... the value we need to
adjust
> is the layoutSize inside the PageScaleConstraints object, which is computed by
> ViewportDescription::resolve but not actually stored in the
ViewportDescription.

Right, never mind, sorry.

[skobes, 2013-11-09 00:44:26]

On 2013/11/08 04:06:16, aelias wrote:
> OK, looks good.  One more thing, could you write a test in WebFrameTest.cpp
> checking the width is reduced as expected and that 980px is the minimum? 
There
> are several similar tests in that file you can base it on.

Added a test, but I can't figure out how to test the 980px minimum.  It comes
from Source/core/css/viewportAndroid.css but the test doesn't seem to use that. 
I see Source/web/tests/data/fixed_layout.html specifies an @viewport to mimic
Android, but in my case an explicit @viewport disables the behavior I'm testing.

[aelias_OOO_until_Jul13, 2013-11-09 01:46:56]

Try using Source/web/tests/data/no_viewport_tag.html .  It has an explicit
min-width to address the same problem in other tests.  bokan@ is working on
making that unnecessary.

lgtm otherwise.

[skobes, 2013-11-09 03:36:03]

On 2013/11/09 01:46:56, aelias wrote:
> Try using Source/web/tests/data/no_viewport_tag.html .  It has an explicit
> min-width to address the same problem in other tests.  bokan@ is working on
> making that unnecessary.

no_viewport_tag.html looks similar to fixed_layout.html, they both have an
explicit @viewport.

[johnme, 2013-11-11 15:49:37]

+Kenneth and Rune (our external viewport experts).

(see http://crbug.com/135869, http://crbug.com/229151 and
http://crbug.com/252828 for context)

On 2013/11/07 17:55:26, skobes wrote:
> If widths below 980px are acceptable we can tweak the constant and possibly
get
> more benefit from this.  But I don't know how many sites would look broken at
> narrower widths... do you know if we have any data on this?

Sadly I don't know of any data on this. It's definitely worth investigating
further, as the smaller we can drive this value the less zooming & autosizing
the user gets subjected to.

For example desktop wikipedia has a flexible layout, and looks much better when
you give it a fallback width of e.g. 640 than it does at 980 (but if you let the
fallback width go down to device-width, the layout breaks and starts overlapping
itself, because they forgot to put a min-width in).

The legacy Android Browser used to use a fallback width of 800 (and I'm not
aware of any major problems from that), but Chrome ended up switching to 980 to
match Safari. 980 has always seemed a strange value to use though; it's clearly
driven by the old dominance of 1024x768 among smaller desktop screen resolutions
(and hence some fraction of web developers may have tolerated their sites
breaking at narrower widths than 1024). But if that's the reason, why not just
use 1024 itself, as Internet Explorer does?

https://codereview.chromium.org/61593006/diff/330001/Source/web/PageScaleCons...
File Source/web/PageScaleConstraintsSet.cpp (right):

https://codereview.chromium.org/61593006/diff/330001/Source/web/PageScaleCons...
Source/web/PageScaleConstraintsSet.cpp:207: if
(!viewportDescription.isSpecifiedByAuthor()) {
Actually, I think we should also reduce the viewport width if the page author
specifies width=device-width (meaning their site renders well at any reasonable
width).

For example, consider a partially sighted user who sets fontScaleFactor to 200%.
When they browse the web on an 8" tablet, they should get the phone UI of every
site, scaled up by 200%.

Such a UI would render perfectly, with no layout issues (unlike any other
possible way of applying a 200% zoom without adding horizontal scrolling - and
of course horizontal scrolling isn't a suitable solution, since with wide
paragraphs it often means you have to scroll back and forth for every line of
text).

As for whether the fudge factor gets applied this way as well, I'm less certain,
as I buy your arguments about it being strange that device-width behaves
differently in Chrome and Chrome-powered WebView for example. Though applying
different amounts of Text Autosizing to the two is probably no better...

https://codereview.chromium.org/61593006/diff/330001/Source/web/PageScaleCons...
Source/web/PageScaleConstraintsSet.cpp:209:
layoutSize.setWidth(std::max(viewportDescription.minWidth.value(),
layoutSize.width() / deviceScaleAdjustment));
I'm a bit concerned that this patch is going to have strange side-effects.

On desktop we use browser zoom (full page zoom), and on mobile we use page scale
(pinch zoom). This patch essentially allows us to use a little bit of browser
zoom in addition to page scale (which is great!).

But it's doing it in quite an indirect manner, which means other parts of Blink
won't know the page is zoomed. For example http://crrev.com/23000005 and
http://crrev.com/23192002 made window.devicePixelRatio and the corresponding
@media queries take into account the amount of browser zoom.

You want the same thing to happen here, for example if you have a
background-image:-webkit-image-set("xs.jpg" 1x, "s.jpg" 1.5x, "m.jpg" 2x,
"l.jpg" 2.5x, "xl.jpg" 3x) on a page with no meta viewport on a landscape Nexus
10 (2560x1600 @ 2x with a fontScaleFactor of 1.3), we'll make the page max(980,
2560 / (2*1.3)) = 984.6px wide. That means, the devicePixelRatio (the ratio
between unzoomed CSS pixels and physical screen pixels) will be 2560/984.6 =
2.6x; hence the browser (assuming no bandwidth constraints) should download
either l.jpg or xl.jpg as those are the closest to 2.6x, and hence will give the
crispest image. Ditto with anything the page does themselves based on
window.devicePixelRatio or the MQs.

It still seems to me that the cleanest way of doing this patch would be to
modify the deviceScaleFactor used throughout the browser. I believe everything
would "just work" then.

Alternatively if it's possible to more explicitly tie this patch into the
browser zoom machinery (such that Frame::pageZoomFactor would be 1.3 in the
example above, and hence Frame::devicePixelRatio would be 2.6 in the example
above), that might make sense.

However I recognise that there are additional constraints here, such as the
(sensible) desire for this zoom not to reduce page width below 980px on desktop
sites, so it does make sense to tie it into viewports (though modifying
deviceScaleFactor should have the same effect). Either way, please make sure
that this zoom is taken into account in Frame::devicePixelRatio, to match our
behavior on desktop.

[johnme, 2013-11-11 15:50:11]

Actually +Kenneth & Rune this time.

[bokan, 2013-11-11 20:51:48]

On 2013/11/09 03:36:03, skobes wrote:
> On 2013/11/09 01:46:56, aelias wrote:
> > Try using Source/web/tests/data/no_viewport_tag.html .  It has an explicit
> > min-width to address the same problem in other tests.  bokan@ is working on
> > making that unnecessary.
> 
> no_viewport_tag.html looks similar to fixed_layout.html, they both have an
> explicit @viewport.

I'm currently working on a CL that will inject the appropriate UA stylesheet for
the Android based tests (and general cleanup in WebFrameTest). If you want to
land this, just leave it with the explicit @viewport tag and I'll fix it when
I'm landing my changes. (Maybe leave a FIXME as a reminder).

Thanks,
David

[skobes, 2013-11-11 21:43:06]

John, thanks for the thorough response!

On 2013/11/11 15:49:37, johnme wrote:
> [Website tolerance of layout width < 980px is] definitely worth investigating
> further, as the smaller we can drive this value the less zooming & autosizing
> the user gets subjected to.

Agreed.

> Actually, I think we should also reduce the viewport width if the page author
> specifies width=device-width (meaning their site renders well at any
reasonable
> width).
> 
> For example, consider a partially sighted user who sets fontScaleFactor to
200%.

Note that this patch only adjusts layout width based on the fudge factor, not
the actual slider value.  Doing it for the slider value as well is an
interesting idea but if we go down that road we should change the UI label to
something other than "Text scaling" since it would scale everything instead of
just the text.  Do we have any idea how many users actually move the slider? 
I'm not sure how much effort we want to put into optimizing for non-default
slider values.

> On desktop we use browser zoom (full page zoom), and on mobile we use page
scale
> (pinch zoom). This patch essentially allows us to use a little bit of browser
> zoom in addition to page scale (which is great!).
> 
> But it's doing it in quite an indirect manner, which means other parts of
Blink
> won't know the page is zoomed. For example http://crrev.com/23000005 and
> http://crrev.com/23192002 made window.devicePixelRatio and the corresponding
> @media queries take into account the amount of browser zoom.

Interesting... doesn't the problem you're describing already exist today for
sites that are not width=device-width?  If the layout width in CSS px is not
equal to the device width in DIPs, we scale the page to fit the viewport on the
initial render (I don't know the right term for this type of "zoom"), but we
don't alter devicePixelRatio based on that scaling.  So any logic based on
device pixel ratio (@media, -webkit-image-set, etc.) is already doing the wrong
thing with respect to crispness on these sites.  Maybe that should be fixed?

If there is no separate plan to make devicePixelRatio account for the initial
page scale, AND we want to alter the layout width on pages that have
width=device-width, then I agree that to preserve crispness that alteration
would need to be done on the device scale factor instead of the way it is done
in this patch.

[skobes, 2013-11-11 22:37:33]

On 2013/11/11 20:51:48, bokan wrote:
> I'm currently working on a CL that will inject the appropriate UA stylesheet
for
> the Android based tests (and general cleanup in WebFrameTest).

I think this will be moot since John persuaded me in
https://codereview.chromium.org/49913005/ that an explicit meta viewport with
width > device width should be treated as a desktop site.  As a nice side effect
of that, it becomes easier to test.

[skobes, 2013-11-12 19:48:32]

On 2013/11/11 22:37:33, skobes wrote:
> On 2013/11/11 20:51:48, bokan wrote:
> > I'm currently working on a CL that will inject the appropriate UA stylesheet
> for
> > the Android based tests (and general cleanup in WebFrameTest).
> 
> I think this will be moot since John persuaded me in
> https://codereview.chromium.org/49913005/ that an explicit meta viewport with
> width > device width should be treated as a desktop site.  As a nice side
effect
> of that, it becomes easier to test.

Actually disregard what I said above, we do still need to distinguish explicit
vs. implicit viewport, because it seems wrong to override an explicit numeric
width even if it meets the threshold for being a "desktop site".  If the page
author sets width=1280 and we change it to 984 that is clearly a bug.

[kenneth.r.christiansen, 2013-11-19 16:32:52]

https://codereview.chromium.org/61593006/diff/330001/Source/web/PageScaleCons...
File Source/web/PageScaleConstraintsSet.cpp (right):

https://codereview.chromium.org/61593006/diff/330001/Source/web/PageScaleCons...
Source/web/PageScaleConstraintsSet.cpp:207: if
(!viewportDescription.isSpecifiedByAuthor()) {
I am not sure that is a good idea.

People who add width=device-width do so because they want one CSS pixel to be
equal to one UX pixel, ie they know that they toolbars will have the same size
of native toolbars etc and this would break that promise.

On the other hand if authors add width=device-width and their text is too small
to be readable, well they it is their fault as they did it on purpose.

[kenneth.r.christiansen, 2014-01-27 17:22:28]

Did this get abandoned?

[skobes, 2014-01-27 17:30:54]

On 2014/01/27 17:22:28, kenneth.r.christiansen wrote:
> Did this get abandoned?

Yes, given the limited scope (10" tablets in landscape) and concerns raised
about crispness and distinguishing mobile vs. desktop sites I decided not to
pursue this and focus my time on the single-pass autosizer.  Feel free to pick
it up if you like.