Translate viewport related meta tags into @viewport descriptors as suggested by the CSS Device Adap…

Translate viewport related meta tags into @viewport descriptors as suggested by the CSS Device Adaptation spec.

This futhermore extends it to the XHTML Mobile Profile, and the handheldfriendly and mobileoptimized meta tags.

[abarth-chromium, 2013-07-17 18:15:54]

Should I be worried that LayoutTests/fast/viewport/viewport-*-expected.txt are
changing?

[abarth-chromium, 2013-07-17 18:17:17]

https://codereview.chromium.org/19555002/diff/4001/Source/core/testing/Intern...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/19555002/diff/4001/Source/core/testing/Intern...
Source/core/testing/Internals.cpp:938: + " and userScalable " +
(arguments.userZoom ? "true" : "false");
Is there a way to make this change in smaller steps so that it's clearer that
our behavior isn't changing?  Currently you're changing a lot of code and
changing the results for a large number of tests.  That makes it difficult to
determine whether your code change is causing regressions.

[kenneth.r.christiansen, 2013-07-17 19:49:55]

Hi there,

It is pretty hard, but Mikhail is helping me do that.
https://codereview.chromium.org/19632002/ is one example.

I have tried to be very careful with the tests, and many actually got fixed.

The height change is a conscious change with is due to some spec changes that
seems to have consensus; the spec got rid of the device-size and are using
initial-viewport-size.

It shouldn't matter at all if Android changes (?) to set the
initial-viewport-size equal to the whole available screen area. It might even be
doing that because of the fullscreen browsing, but that needs verification.

For width it is already equal (due to no borders on Android) so that should not
be a problem. For height it might, but height= is very seldom used and we
actually fail all heights tests currently.

I will see if I can look at this together with John Mellor and we can probably
do that change in a separate patch.

[kenneth.r.christiansen, 2013-07-17 19:50:28]

> The height change is a conscious change with is due to some spec changes that
> seems to have consensus; the spec got rid of the device-size and are using
> initial-viewport-size.

And is using 100% of the initial viewport size.

[Mikhail, 2013-07-19 12:56:20]

https://codereview.chromium.org/19555002/diff/4001/Source/core/css/resolver/S...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/19555002/diff/4001/Source/core/css/resolver/S...
Source/core/css/resolver/StyleResolver.cpp:1496: return
m_viewportStyleResolver->affectedByViewportChange();
This is causing dead loop if having '%' values of viewport width/height
descriptors: m_viewportStyleResolver->affectedByViewportChange() always returns
true in this case -> StyleResolver::affectedByViewportChange() returns true
also.

So have the following loop:
FrameView::performPostLayoutTasks() -> Document::styleResolverChanged() ->
view()->scheduleRelayout() -> FrameView::performPostLayoutTasks().

Document::styleResolverChanged() is invoked in
FrameView::performPostLayoutTasks() on following condition:

if (m_frame->document()->styleResolver()->affectedByViewportChange())

So, one solution could be changing it to:

if (resized && m_frame->document()->styleResolver()->affectedByViewportChange())


but first we need to make sure that it would not bring regressions to orienation
media queries.

[kenneth.r.christiansen, 2013-07-19 16:31:41]

Given your description is sounds as this problem exists already with viewport
dependent media queries. Isn't that tested? or why is it not a problem for
those?

[apavlov, 2013-07-30 12:06:44]

https://codereview.chromium.org/19555002/diff/4001/Source/core/html/HTMLMetaE...
File Source/core/html/HTMLMetaElement-in.cpp (right):

https://codereview.chromium.org/19555002/diff/4001/Source/core/html/HTMLMetaE...
Source/core/html/HTMLMetaElement-in.cpp:334: // Numbers in the range <-1, 1>,
and unknown values (represented as 0), are mapped to "fixed".
You should use (-1, 1) to denote the open interval (since 1 and -1 are mapped to
"zoom")

[rune, 2013-08-05 20:16:09]

On 2013/07/19 16:31:41, kenneth.r.christiansen wrote:
> Given your description is sounds as this problem exists already with viewport
> dependent media queries. Isn't that tested? or why is it not a problem for
> those?

Because media queries make affectedByViewportChange only when the result of eval
changes. The eval change will only happen for the first call after a resize, not
the following calls. ViewportStyleResolver::affectedByViewportChange always
return true for percentages regardless.

[rune, 2013-08-05 20:47:03]

On 2013/08/05 20:16:09, rune wrote:
> On 2013/07/19 16:31:41, kenneth.r.christiansen wrote:
> > Given your description is sounds as this problem exists already with
viewport
> > dependent media queries. Isn't that tested? or why is it not a problem for
> > those?
> 
> Because media queries make affectedByViewportChange only when the result of
eval
> changes. The eval change will only happen for the first call after a resize,
not
> the following calls. ViewportStyleResolver::affectedByViewportChange always
> return true for percentages regardless.

If I read the code correctly, it currently recalculates all style on resize when
you have percentage sizes for @viewport. If so, that's way too expensive. You
currently need that because we store pixel lengths calculated from percentages
in ViewportArguments. Instead, percentages could be kept as a result of
cascading @viewport and resolve the percentage values as part of
ViewportArguments::resolve which I believe is called on resize. Then we
shouldn't need the affectedByViewportChange method in ViewportStyleResolver at
all.

[rune, 2013-08-07 07:33:16]

On 2013/08/05 20:47:03, rune wrote:

> If I read the code correctly, it currently recalculates all style on resize
when
> you have percentage sizes for @viewport. If so, that's way too expensive. You
> currently need that because we store pixel lengths calculated from percentages
> in ViewportArguments. Instead, percentages could be kept as a result of
> cascading @viewport and resolve the percentage values as part of
> ViewportArguments::resolve which I believe is called on resize. Then we
> shouldn't need the affectedByViewportChange method in ViewportStyleResolver at
> all.

I've implemented that here:

https://codereview.chromium.org/22549002/

[rune, 2013-08-15 13:28:52]

I've split done removal of deviceSize for configurationForViewport in:

https://codereview.chromium.org/23101004

Which should cause less LayoutTest changes in here.

[rune, 2013-08-21 09:18:21]

https://codereview.chromium.org/19555002/diff/30001/Source/core/css/resolver/...
File Source/core/css/resolver/ViewportStyleResolver.cpp (left):

https://codereview.chromium.org/19555002/diff/30001/Source/core/css/resolver/...
Source/core/css/resolver/ViewportStyleResolver.cpp:96: m_propertySet = 0;
I can't see where the property set is reset between cascades with this removal.

[rune, 2013-08-22 15:43:38]

https://codereview.chromium.org/19555002/diff/20001/Source/core/page/PageScal...
File Source/core/page/PageScaleConstraintsSet.cpp (right):

https://codereview.chromium.org/19555002/diff/20001/Source/core/page/PageScal...
Source/core/page/PageScaleConstraintsSet.cpp:143: // FIXME: Why this particular
Android behavior?
I've tried to take your CL and build it here locally. I tried to find tests for
this functionality, and did not find any in LayoutTests, but I did in
webkit_unit_tests. Have you tried to build and run those?

I got two fails running those with my local version of this CL, but I might have
messed up the patch application.

I think there are some tests for this in the android WebView tests, too.

I can try and analyze this behavior further.

[kenneth.r.christiansen, 2013-08-22 16:13:54]

> I got two fails running those with my local version of this CL, but I might
have
> messed up the patch application.
> 
> I think there are some tests for this in the android WebView tests, too.
> 
> I can try and analyze this behavior further.

Yes I ran exactly those and they (at least used to) pass, thought I didn't check
that with the latest rebase.

[rune, 2013-08-23 07:43:38]

On 2013/08/22 16:13:54, kenneth.r.christiansen wrote:
> > I got two fails running those with my local version of this CL, but I might
> have
> > messed up the patch application.
> > 
> > I think there are some tests for this in the android WebView tests, too.
> > 
> > I can try and analyze this behavior further.
> 
> Yes I ran exactly those and they (at least used to) pass, thought I didn't
check
> that with the latest rebase.

There's an issue with width=device-width -> 100% in the relevant test-cases. If
I switch to 640 in the .html files, the tests pass. Will investigate further ...

[rune, 2013-08-23 09:45:15]

On 2013/08/23 07:43:38, rune wrote:
> On 2013/08/22 16:13:54, kenneth.r.christiansen wrote:
> > > I got two fails running those with my local version of this CL, but I
might
> > have
> > > messed up the patch application.
> > > 
> > > I think there are some tests for this in the android WebView tests, too.
> > > 
> > > I can try and analyze this behavior further.
> > 
> > Yes I ran exactly those and they (at least used to) pass, thought I didn't
> check
> > that with the latest rebase.
> 
> There's an issue with width=device-width -> 100% in the relevant test-cases.
If
> I switch to 640 in the .html files, the tests pass. Will investigate further
...

Works if I re-introduce the ViewportResolver::affectedByViewport change. So, I
guess applying https://codereview.chromium.org/22549002/ should work too.

[rune, 2013-08-26 21:32:23]

https://codereview.chromium.org/19555002/diff/20001/Source/core/page/PageScal...
File Source/core/page/PageScaleConstraintsSet.cpp (right):

https://codereview.chromium.org/19555002/diff/20001/Source/core/page/PageScal...
Source/core/page/PageScaleConstraintsSet.cpp:143: // FIXME: Why this particular
Android behavior?
On 2013/08/22 15:43:38, rune wrote:
> I can try and analyze this behavior further.

After reading [1] and [2] it starts to be clearer what this code is supposed to
do, but it looks kind of complicated because the code tries to undo parts of
what has been done in ViewportArguments::resolve. I think that it would be
clearer if the targetDensityDpi was taken into account in
ViewportArguments::resolve(), and useWideViewport was taken into account when
meta viewport was translated into @viewport descriptors.

A question that needs to be answered about @viewport here? Should @viewport
rules be affected by the Android WebView settings the same way as meta viewport
is?

Also, I think there are some code in there that is really not intentional. For
instance, if useWideViewPort is false, ViewportResolver::resolve will resolve
width and height values as usual, but the width is adjusted here. Then, the
adjusted height will be set relative to the aspect ratio of the resolved
width/height that didn't take useWideViewport=false into account. That sounds
really strange since useWideViewport=false means you should basically use the
initial viewport size scaled with the initial-scale value resolved from the meta
viewport and override the resolved size completely. Making the aspect ratio of
the overridden width/height have an effect on height does not make sense.

[1]
http://developer.android.com/reference/android/webkit/WebSettings.html#setUse...
[2]
http://developer.android.com/reference/android/webkit/WebSettings.html#setLoa...

[johnme, 2013-08-27 12:38:59]

+mnaganov and aelias to comment on the Android WebView quirks

[rune, 2013-08-27 12:46:45]

On 2013/08/27 12:38:59, johnme wrote:
> +mnaganov and aelias to comment on the Android WebView quirks

Would be nice to know what is the exact expected behavior for various
combinations of:

- useWideViewPort true/false
- loadWithOverviewMode true/false
- meta viewport tags in the loaded document

To know which peculiarities could be seen as bugs and fixed, as which behaviors
need to be kept.

[mnaganov (inactive), 2013-08-27 14:28:19]

On 2013/08/27 12:46:45, rune wrote:
> On 2013/08/27 12:38:59, johnme wrote:
> > +mnaganov and aelias to comment on the Android WebView quirks
> 
> Would be nice to know what is the exact expected behavior for various
> combinations of:
> 
> - useWideViewPort true/false
> - loadWithOverviewMode true/false
> - meta viewport tags in the loaded document
> 
> To know which peculiarities could be seen as bugs and fixed, as which
behaviors
> need to be kept.

We have WebFrameTest tests for important tests
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...),
to run them, use the 'webkit_unit_tests' target in Chromium.

Also we have tests in android_webview/ in Chromium, but you will need to run
them on a device or on an emulator.

[aelias_OOO_until_Jul13, 2013-08-27 16:58:08]

Unfortunately, the only specification for the meaning of useWideViewport and
loadWithOverviewMode are the old WebView source code.  Roughly speaking,
useWideViewport = true means that layoutFallbackWidth of 980 should be used,
useWideViewport = false means that device-width should be used for layout width.
 loadWithOverviewMode roughly means that initial-scale should default to
minimum-scale (if true), or 1 (if false).  The complexity arises when a viewport
tag is also specified: then in some cases the viewport tag "wins" and in some
cases the boolean flag "wins", it's arbitrary, and wrote tests for the behaviors
that we observed the old implementation had.

https://codereview.chromium.org/19555002/diff/30001/Source/core/page/PageScal...
File Source/core/page/PageScaleConstraintsSet.cpp (right):

https://codereview.chromium.org/19555002/diff/30001/Source/core/page/PageScal...
Source/core/page/PageScaleConstraintsSet.cpp:146: // @viewport { zoom: 2.0;
width: extend-to-zoom; } according to the spec.
We've observed that classic Android WebView does not behave this way;
initial-scale didn't affect the layout width in that implementation (and some
apps have come to depend on this).  mnaganov@ recently prepared a patch to apply
this quirk more generally behind a setting:
https://codereview.chromium.org/23038013/ .

[rune, 2013-08-28 23:30:13]

https://codereview.chromium.org/19555002/diff/30001/Source/core/css/resolver/...
File Source/core/css/resolver/ViewportStyleResolver.cpp (right):

https://codereview.chromium.org/19555002/diff/30001/Source/core/css/resolver/...
Source/core/css/resolver/ViewportStyleResolver.cpp:90: bool
ViewportStyleResolver::shouldDisableDesktopWorkarounds() const
I have tried to split this out as a separate change where I work on the
constrained values in WebViewImpl instead:

https://codereview.chromium.org/23592015/

https://codereview.chromium.org/19555002/diff/30001/Source/core/css/resolver/...
Source/core/css/resolver/ViewportStyleResolver.cpp:103: if (minZoom == maxZoom
&& minZoom == ViewportArguments::ValueAuto)
Should have been "minZoom != ViewportArguments::ValueAuto".

[rune, 2013-08-30 22:49:02]

https://chromiumcodereview.appspot.com/19555002/diff/30001/LayoutTests/fast/v...
File LayoutTests/fast/viewport/viewport-131-expected.txt (right):

https://chromiumcodereview.appspot.com/19555002/diff/30001/LayoutTests/fast/v...
LayoutTests/fast/viewport/viewport-131-expected.txt:1: ALERT: viewport size
320x352 scale 1 with limits [1, 5] and userScalable false
This is not correct as the output from this test are the scale factor limits
after userScalable=false has been translated into min and max scale factors
being equal.

[apavlov, 2013-11-25 09:13:16]

Is this patch still of interest?

[kenneth.r.christiansen, 2013-11-25 09:19:55]

On 2013/11/25 09:13:16, apavlov wrote:
> Is this patch still of interest?

No, closed

[johnme, 2013-11-25 11:43:08]

On 2013/11/25 09:19:55, kenneth.r.christiansen wrote:
> On 2013/11/25 09:13:16, apavlov wrote:
> > Is this patch still of interest?
> 
> No, closed

Is there a new plan for how to handle this, or did you just run out of time?

[kenneth.r.christiansen, 2013-11-25 12:00:38]

On 2013/11/25 11:43:08, johnme wrote:
> On 2013/11/25 09:19:55, kenneth.r.christiansen wrote:
> > On 2013/11/25 09:13:16, apavlov wrote:
> > > Is this patch still of interest?
> > 
> > No, closed
> 
> Is there a new plan for how to handle this, or did you just run out of time?

Rune thought it was not worth it, so we just keep the two code paths.