Experimental viewport meta tag support for desktop, Blink-side.

Experimental viewport meta tag support for desktop, Blink-side.

Added flag to allow desktop builds to experimentally turn
on support for the viewport meta tag. The --enable-viewport
flag now turns on only support for @viewport.
--enable-viewport-meta turns on viewport meta and @viewport
support.

On Android, the viewport gets rescaled when the view is
resized, i.e. phone is rotated between landscape and
portrait. On desktop, enabling the viewport causes the page
to zoom out when the window is shrunk, rather than adding
scrollbars. To support viewport on both devices, I've added
a --scale-viewport-on-resize flag that should be set on
Android and not desktop. This prevents the viewport scaling
code in WebViewImpl::resize().

Enabling viewport on desktop means the page scale can
change. This causes a known issue with scrollbars where
mouse events are scaled with the page, making mouse
interaction with scrollbars broken.

This is the Blink side of a 2-side patch.

BUG=232102
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161378

[aelias_OOO_until_Jul13, 2013-10-25 04:10:08]

https://codereview.chromium.org/40423003/diff/1/Source/core/css/html.css
File Source/core/css/html.css (right):

https://codereview.chromium.org/40423003/diff/1/Source/core/css/html.css#newc...
Source/core/css/html.css:1136: width: auto;
If not done here then the 980px value needs to be injected elsewhere or Android
layout of desktop pages will break.  Perhaps rune@ can advise.

https://codereview.chromium.org/40423003/diff/1/Source/web/WebSettingsImpl.h
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/40423003/diff/1/Source/web/WebSettingsImpl.h#...
Source/web/WebSettingsImpl.h:184: virtual void setScaleViewportOnResize(bool);
How about setMainFrameResizesAreOrientationChanges?

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

https://codereview.chromium.org/40423003/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:1696: settings()->viewportEnabled() &&
settings()->scaleViewportOnResize()
You can delete the settings()->viewportEnabled() now that you have a different
setting.

https://codereview.chromium.org/40423003/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:1721: view->layout();
Hmm, this should a no-op after the fixed layout change CL (I thought I mentioned
it should be deleted at the time), please delete this layout call.  Then you can
delete the if statement here and collapse the "!m_fixedLayoutSizeLock" condition
as part of the shouldAnchorAndRescaleViewport boolean setting.

[bokan, 2013-10-25 17:19:37]

https://codereview.chromium.org/40423003/diff/1/Source/core/css/html.css
File Source/core/css/html.css (right):

https://codereview.chromium.org/40423003/diff/1/Source/core/css/html.css#newc...
Source/core/css/html.css:1136: width: auto;
On 2013/10/25 04:10:08, aelias wrote:
> If not done here then the 980px value needs to be injected elsewhere or
Android
> layout of desktop pages will break.  Perhaps rune@ can advise.
I think the idea was that we could have separate stylesheets between Android and
Desktop, though I'm not sure how best to implement that or if now is the time to
do it. What are your thoughts rune@?

https://codereview.chromium.org/40423003/diff/1/Source/web/WebSettingsImpl.h
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/40423003/diff/1/Source/web/WebSettingsImpl.h#...
Source/web/WebSettingsImpl.h:184: virtual void setScaleViewportOnResize(bool);
On 2013/10/25 04:10:08, aelias wrote:
> How about setMainFrameResizesAreOrientationChanges?
SGTM

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

https://codereview.chromium.org/40423003/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:1721: view->layout();
On 2013/10/25 04:10:08, aelias wrote:
> Hmm, this should a no-op after the fixed layout change CL (I thought I
mentioned
> it should be deleted at the time), please delete this layout call.  Then you
can
> delete the if statement here and collapse the "!m_fixedLayoutSizeLock"
condition
> as part of the shouldAnchorAndRescaleViewport boolean setting.
Sorry, you did mention it in that review, but I replied and took the lack or
response as agreement. Why do you think this is a no-op now? I tried removing
this but it breaks numerous unit tests related to minimum scale so it does seem
to be required. None of the changes in the fixed layout CL add a layout above.

[rune, 2013-10-25 20:22:54]

https://codereview.chromium.org/40423003/diff/1/Source/core/css/html.css
File Source/core/css/html.css (right):

https://codereview.chromium.org/40423003/diff/1/Source/core/css/html.css#newc...
Source/core/css/html.css:1136: width: auto;
On 2013/10/25 17:19:38, bokan wrote:
> On 2013/10/25 04:10:08, aelias wrote:
> > If not done here then the 980px value needs to be injected elsewhere or
> Android
> > layout of desktop pages will break.  Perhaps rune@ can advise.
> I think the idea was that we could have separate stylesheets between Android
and
> Desktop, though I'm not sure how best to implement that or if now is the time
to
> do it. What are your thoughts rune@?

You either need a separate stylesheet or a media query to decide which of the
behaviors to have. Separate stylesheets can be done by moving the 980px viewport
rule to a stylesheet that is only applied on Android.

There is an example of Android specific styles in
mediaControlsChromiumAndroid.css. I think that it might be mutually exclusive
with mediaControlsChromium.css and that the selection is done when generating
C++ from CSS at build time. For the @viewport rule, I think I would have put it
in a separate stylesheet that is applied after html.css.

Note that, since 'auto' is the initial value for width, "@viewport { width: auto
}" will not have any effects unless it is used to override another @viewport
rule. So, just removing the rule in the change above will have the same effect.

Regarding a media query to trigger the @viewport rule - If you have a certain
width where you want the Android behavior (min 980px) to trigger if the CSS
pixel device width is less than a certain width, and the "desktop" behavior
(auto) when it's above the same width, you could in theory[1] use:

@media (max-device-width: 1024px) {
  @viewport { min-width: 980px; }
}

In practice this means:

UAs running in an environment where the device-width is > 1024px gets the plain
old desktop behavior where the viewport size follows the window size. UAs having
a device-width < 1024px gets a minimum viewport width of 980px, BUT if the
initial viewport width (window width) is wider than 980px, the actual viewport
width will be the same as the initial viewport width. That is because the
max-width is auto and the actual viewport width will be as close as possible to
the initial viewport width.


There are other alternatives as well. Uglier, but I'm just mentioning:

If @media evaluation works in UA stylesheets, and the device-width will not tell
which behavior to use, introduce an internal media feature:

@media (-internal-something) {
  @viewport { min-width: 980px; }
}

Or use a min-width keyword that depends on a runtime flag:

@viewport { min-width: -internal-min-width; }


For now, I think I would have chosen a separate stylesheet whose application
depends on a runtime flag. At least, I think that having the @viewport rule in
one of the .css files is superior to having it injected in C++ code.

[1] I say "in theory" because I don't think media query evaluation works well in
Blink UA stylesheets. At least, I don't think they will re-evaluate (on
orientation changes for instance). But that needs to be checked further.

[aelias_OOO_until_Jul13, 2013-10-25 23:15:01]

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

https://codereview.chromium.org/40423003/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:1721: view->layout();
OK, that's fine, my fault for not remembering that.

I never really liked that layout call but I still can't come up with a cleaner
alternative whenever I look into it, so OK.

[bokan, 2013-10-28 22:17:51]

https://codereview.chromium.org/40423003/diff/1/Source/core/css/html.css
File Source/core/css/html.css (right):

https://codereview.chromium.org/40423003/diff/1/Source/core/css/html.css#newc...
Source/core/css/html.css:1136: width: auto;
On 2013/10/25 20:22:54, rune - CET wrote:
> On 2013/10/25 17:19:38, bokan wrote:
> > On 2013/10/25 04:10:08, aelias wrote:
> > > If not done here then the 980px value needs to be injected elsewhere or
> > Android
> > > layout of desktop pages will break.  Perhaps rune@ can advise.
> > I think the idea was that we could have separate stylesheets between Android
> and
> > Desktop, though I'm not sure how best to implement that or if now is the
time
> to
> > do it. What are your thoughts rune@?
> 
> You either need a separate stylesheet or a media query to decide which of the
> behaviors to have. Separate stylesheets can be done by moving the 980px
viewport
> rule to a stylesheet that is only applied on Android.
> 
> There is an example of Android specific styles in
> mediaControlsChromiumAndroid.css. I think that it might be mutually exclusive
> with mediaControlsChromium.css and that the selection is done when generating
> C++ from CSS at build time. For the @viewport rule, I think I would have put
it
> in a separate stylesheet that is applied after html.css.
> 
> Note that, since 'auto' is the initial value for width, "@viewport { width:
auto
> }" will not have any effects unless it is used to override another @viewport
> rule. So, just removing the rule in the change above will have the same
effect.
> 
> Regarding a media query to trigger the @viewport rule - If you have a certain
> width where you want the Android behavior (min 980px) to trigger if the CSS
> pixel device width is less than a certain width, and the "desktop" behavior
> (auto) when it's above the same width, you could in theory[1] use:
> 
> @media (max-device-width: 1024px) {
>   @viewport { min-width: 980px; }
> }
> 
> In practice this means:
> 
> UAs running in an environment where the device-width is > 1024px gets the
plain
> old desktop behavior where the viewport size follows the window size. UAs
having
> a device-width < 1024px gets a minimum viewport width of 980px, BUT if the
> initial viewport width (window width) is wider than 980px, the actual viewport
> width will be the same as the initial viewport width. That is because the
> max-width is auto and the actual viewport width will be as close as possible
to
> the initial viewport width.
> 
> 
> There are other alternatives as well. Uglier, but I'm just mentioning:
> 
> If @media evaluation works in UA stylesheets, and the device-width will not
tell
> which behavior to use, introduce an internal media feature:
> 
> @media (-internal-something) {
>   @viewport { min-width: 980px; }
> }
> 
> Or use a min-width keyword that depends on a runtime flag:
> 
> @viewport { min-width: -internal-min-width; }
> 
> 
> For now, I think I would have chosen a separate stylesheet whose application
> depends on a runtime flag. At least, I think that having the @viewport rule in
> one of the .css files is superior to having it injected in C++ code.
> 
> [1] I say "in theory" because I don't think media query evaluation works well
in
> Blink UA stylesheets. At least, I don't think they will re-evaluate (on
> orientation changes for instance). But that needs to be checked further.

Ok, I added a viewportAndroid.css. I've tried a desktop and an Android build and
it looks like it works.

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

https://codereview.chromium.org/40423003/diff/1/Source/web/WebViewImpl.cpp#ne...
Source/web/WebViewImpl.cpp:1696: settings()->viewportEnabled() &&
settings()->scaleViewportOnResize()
On 2013/10/25 04:10:08, aelias wrote:
> You can delete the settings()->viewportEnabled() now that you have a different
> setting.

Done.

[bokan, 2013-10-28 22:20:25]

Rune, Kenneth mentioned that @viewport is only enabled with
--enable-experimental-web-platform-features. Is this for author stylesheets
only? It looks like the @viewport rule from the UA sheet is applied without it,
I'm assuming UA sheets aren't guarded by that flag?

[rune, 2013-10-28 22:28:25]

On 2013/10/28 22:20:25, bokan wrote:
> Rune, Kenneth mentioned that @viewport is only enabled with
> --enable-experimental-web-platform-features. Is this for author stylesheets
> only? It looks like the @viewport rule from the UA sheet is applied without
it,
> I'm assuming UA sheets aren't guarded by that flag?

Yes, @viewport works from UA sheets regardless of the runtime flag setting.

[bokan, 2013-10-31 15:38:07]

PTAL at this and the associated Chromium CL

[aelias_OOO_until_Jul13, 2013-10-31 21:06:58]

https://codereview.chromium.org/40423003/diff/130001/Source/core/core_derived...
File Source/core/core_derived_sources.gyp (right):

https://codereview.chromium.org/40423003/diff/130001/Source/core/core_derived...
Source/core/core_derived_sources.gyp:439: 'css/viewportAndroid.css',
I don't see what code makes sure this CSS file is loaded on Android?  It doesn't
look like adding it here should be sufficient unless there is some string
matching going on?

https://codereview.chromium.org/40423003/diff/130001/Source/web/WebSettingsIm...
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/40423003/diff/130001/Source/web/WebSettingsIm...
Source/web/WebSettingsImpl.h:184: virtual void
setMainFrameResizesAreOrientationChanges(bool);
nit: alphabetical order

https://codereview.chromium.org/40423003/diff/130001/Source/web/tests/data/fi...
File Source/web/tests/data/fixed_layout.html (right):

https://codereview.chromium.org/40423003/diff/130001/Source/web/tests/data/fi...
Source/web/tests/data/fixed_layout.html:3: /* Since tests are built with desktop
viewport settings set the viewport
Hmm, this doesn't feel like the right way to test this.  Is there a way we could
get the relevant unit tests to use the new Android UA stylesheet instead?

https://codereview.chromium.org/40423003/diff/130001/public/web/WebSettings.h
File public/web/WebSettings.h (right):

https://codereview.chromium.org/40423003/diff/130001/public/web/WebSettings.h...
public/web/WebSettings.h:196: virtual void
setMainFrameResizesAreOrientationChanges(bool) = 0;
nit: place it in alphabetical order

[bokan, 2013-10-31 21:18:13]

https://codereview.chromium.org/40423003/diff/130001/Source/core/core_derived...
File Source/core/core_derived_sources.gyp (right):

https://codereview.chromium.org/40423003/diff/130001/Source/core/core_derived...
Source/core/core_derived_sources.gyp:439: 'css/viewportAndroid.css',
On 2013/10/31 21:06:59, aelias wrote:
> I don't see what code makes sure this CSS file is loaded on Android?  It
doesn't
> look like adding it here should be sufficient unless there is some string
> matching going on?
I believe the GYP rules automatically apply files that end in
Android/Win/Linux/etc if the build is for that platform and exclude others.

https://codereview.chromium.org/40423003/diff/130001/Source/web/tests/data/fi...
File Source/web/tests/data/fixed_layout.html (right):

https://codereview.chromium.org/40423003/diff/130001/Source/web/tests/data/fi...
Source/web/tests/data/fixed_layout.html:3: /* Since tests are built with desktop
viewport settings set the viewport
On 2013/10/31 21:06:59, aelias wrote:
> Hmm, this doesn't feel like the right way to test this.  Is there a way we
could
> get the relevant unit tests to use the new Android UA stylesheet instead?
Hmm...I agree this feels wrong. The test this belongs to (WebFrameTest) has a
mix of Android and Desktop tests though. Also, the UA stylesheet is included in
the build so it will apply to all unit tests. It should be possible to replace
the stylesheet in code, but we'd have to split out the Android specific tests
into their own unit test for that to work cleanly (something I'm already planing
on doing)

[aelias_OOO_until_Jul13, 2013-10-31 22:11:51]

OK, splitting off those tests in a separate patch sounds fine.  lgtm.

Adding darin@ for public/web/ review.

[darin (slow to review), 2013-10-31 22:26:20]

LGTM

[bokan, 2013-10-31 23:19:50]

I tied the values of ViewportMetaEnabled and
MainFrameResizesAreOriangationChanges to ViewportEnabled so that this CL doesn't
break Android while the Chromium side is unlanded (and thus no flags to set
those).

https://codereview.chromium.org/40423003/diff/130001/Source/web/WebSettingsIm...
File Source/web/WebSettingsImpl.h (right):

https://codereview.chromium.org/40423003/diff/130001/Source/web/WebSettingsIm...
Source/web/WebSettingsImpl.h:184: virtual void
setMainFrameResizesAreOrientationChanges(bool);
On 2013/10/31 21:06:59, aelias wrote:
> nit: alphabetical order

Done.

https://codereview.chromium.org/40423003/diff/130001/public/web/WebSettings.h
File public/web/WebSettings.h (right):

https://codereview.chromium.org/40423003/diff/130001/public/web/WebSettings.h...
public/web/WebSettings.h:196: virtual void
setMainFrameResizesAreOrientationChanges(bool) = 0;
On 2013/10/31 21:06:59, aelias wrote:
> nit: place it in alphabetical order

Done.

[commit-bot: I haz the power, 2013-10-31 23:25:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/40423003/280001

[commit-bot: I haz the power, 2013-11-01 01:55:40]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[bokan, 2013-11-01 08:01:18]

On 2013/11/01 01:55:40, I haz the power (commit-bot) wrote:
> Retried try job too often on win_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

I was wrong when I thought adding viewportAndroid.css to the gyp would be
enough. I added new files ViewportStyle* and added them in core.gyp and
core.gypi. I'm not sure I've put these in the right place and maybe it's
overkill for viewport? PTAL

I also disabled a test in the css3/device-adapt layout tests that was checking
the UA stylesheet applies a 980 viewport. Now that desktop is non-980 I'm not
sure what to do about this long term.

Rune, please take a look in CSSDefaultStyleSheets.cpp. I added the @viewport
rule after the application of html.css, it seems like it works when I tried it
out but I'm no expert on how CSS rules are applied so I may not be looking at
the right pages. Is there a page you know of I can double check on that would
test the UA viewport?

[kenneth.r.christiansen, 2013-11-01 11:54:59]

> I also disabled a test in the css3/device-adapt layout tests that was checking
> the UA stylesheet applies a 980 viewport. Now that desktop is non-980 I'm not
> sure what to do about this long term.

We can let the tests add a custom user sheet. There is already a test doing
that.

[kenneth.r.christiansen, 2013-11-01 11:55:15]

https://codereview.chromium.org/40423003/diff/530001/Source/core/page/Setting...
File Source/core/page/Settings.cpp (right):

https://codereview.chromium.org/40423003/diff/530001/Source/core/page/Setting...
Source/core/page/Settings.cpp:407: // FIXME: Remove once Chromium-side lands
Mini nit. Comments usually ends with punctuation mark

[rune, 2013-11-02 13:48:49]

On 2013/11/01 08:01:18, bokan wrote:
> On 2013/11/01 01:55:40, I haz the power (commit-bot) wrote:
> > Retried try job too often on win_blink_rel for step(s) webkit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
> 
> I was wrong when I thought adding viewportAndroid.css to the gyp would be
> enough. I added new files ViewportStyle* and added them in core.gyp and
> core.gypi. I'm not sure I've put these in the right place and maybe it's
> overkill for viewport? PTAL

I am not an expert here, but I would perhaps generate the
viewportUserAgentStyleSheet from different .css files instead of using that C++
class.

> I also disabled a test in the css3/device-adapt layout tests that was checking
> the UA stylesheet applies a 980 viewport. Now that desktop is non-980 I'm not
> sure what to do about this long term.
> 
> Rune, please take a look in CSSDefaultStyleSheets.cpp. I added the @viewport
> rule after the application of html.css, it seems like it works when I tried it
> out but I'm no expert on how CSS rules are applied so I may not be looking at
> the right pages. Is there a page you know of I can double check on that would
> test the UA viewport?

UA viewport should trigger 980px layout based on the UA style on pages without
meta viewport or @viewport.

To trigger the simple default stylesheet:

data:text/html,<!DOCTYPE html><body
onload="alert(document.documentElement.offsetWidth);">

To trigger the full default stylesheet:

data:text/html,<!DOCTYPE html><body
onload="alert(document.documentElement.offsetWidth);"><button></button>

[rune, 2013-11-02 13:48:59]

https://codereview.chromium.org/40423003/diff/530001/Source/core/css/CSSDefau...
File Source/core/css/CSSDefaultStyleSheets.cpp (right):

https://codereview.chromium.org/40423003/diff/530001/Source/core/css/CSSDefau...
Source/core/css/CSSDefaultStyleSheets.cpp:123:
defaultStyle->addRulesFromSheet(parseUASheet(ViewportStyle::viewportStyleSheet()),
screenEval());
Since you removed the @viewport from the simple default stylesheet above, you
need to do this in loadSimpleDefaultStyle too.

[bokan, 2013-11-04 21:25:04]

On 2013/11/02 13:48:49, rune - CET wrote:
> I am not an expert here, but I would perhaps generate the
> viewportUserAgentStyleSheet from different .css files instead of using that
C++
> class.
What do you mean? I have a .css file for the @viewport rule but I couldn't find
a way to only include it in the Android build other than adding a C++ class with
implementation dependent on the build. As far as I can tell the other UA sheets
also do this and it didn't seem appropriate to add this into any of them.
> 
> To trigger the simple default stylesheet:
> 
> data:text/html,<!DOCTYPE html><body
> onload="alert(document.documentElement.offsetWidth);">
> 
Thanks, I've now confirmed these work.

[bokan, 2013-11-04 21:30:58]

On 2013/11/01 11:54:59, kenneth.r.christiansen wrote:
> > I also disabled a test in the css3/device-adapt layout tests that was
checking
> > the UA stylesheet applies a 980 viewport. Now that desktop is non-980 I'm
not
> > sure what to do about this long term.
> 
> We can let the tests add a custom user sheet. There is already a test doing
> that.

Ah! Got it, thanks. I manually injected the @viewport {min-width: 980px} into
that test for now. With the differing default behavior on the two platforms I
think the future solution will be to have both platforms have its own set of
unit tests where appropriate.

[bokan, 2013-11-04 21:35:34]

https://codereview.chromium.org/40423003/diff/530001/Source/core/css/CSSDefau...
File Source/core/css/CSSDefaultStyleSheets.cpp (right):

https://codereview.chromium.org/40423003/diff/530001/Source/core/css/CSSDefau...
Source/core/css/CSSDefaultStyleSheets.cpp:123:
defaultStyle->addRulesFromSheet(parseUASheet(ViewportStyle::viewportStyleSheet()),
screenEval());
On 2013/11/02 13:48:59, rune - CET wrote:
> Since you removed the @viewport from the simple default stylesheet above, you
> need to do this in loadSimpleDefaultStyle too.

Done.

https://codereview.chromium.org/40423003/diff/530001/Source/core/page/Setting...
File Source/core/page/Settings.cpp (right):

https://codereview.chromium.org/40423003/diff/530001/Source/core/page/Setting...
Source/core/page/Settings.cpp:407: // FIXME: Remove once Chromium-side lands
On 2013/11/01 11:55:16, kenneth.r.christiansen wrote:
> Mini nit. Comments usually ends with punctuation mark

Done on all FIXMEs

[aelias_OOO_until_Jul13, 2013-11-05 00:10:51]

lgtm

[kenneth.r.christiansen, 2013-11-05 14:34:23]

lgtm

[commit-bot: I haz the power, 2013-11-05 19:39:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bokan@chromium.org/40423003/810001

[commit-bot: I haz the power, 2013-11-05 21:21:23]

Change committed as 161378