Initial containing block for print not affected by page zoom.

Initial containing block for print not affected by page zoom.

The page zoom factor is not applied to the ICB for printing, yet the
ICB basis for media queries and viewport units were. Use 1 as a page
zoom factor in viewportSizeForViewportUnits and use that method to get
the size for the ICB for evaluating media queries.

BUG=699014, 699910
Review-Url: https://codereview.chromium.org/2738173002
Cr-Commit-Position: refs/heads/master@{#456040}
Committed: https://chromium.googlesource.com/chromium/src/+/de209f3db7bbfda754be262cef35b67340c37774

[rune, 2017-03-09 08:40:44]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 08:41:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[rune, 2017-03-09 10:37:45]

rune@opera.com changed reviewers:
+ bokan@chromium.org

[rune, 2017-03-09 10:37:46]

ptal

[commit-bot: I haz the power, 2017-03-09 10:51:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 10:51:27]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-03-09 16:29:56]

https://codereview.chromium.org/2738173002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/MediaValues.cpp (right):

https://codereview.chromium.org/2738173002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/MediaValues.cpp:40: return
frame->view()->viewportSizeForViewportUnits().height();
I think this is slightly wrong. The reason we have a specific viewportSize for
ViewportUnits is because 100vh is actually taller than the ICB by the height of
the URL bar and I think Media Queries should really report the ICB. I would
either add a zoomedLayoutSize method on FrameView or keep the zoom logic in
here.

[bokan, 2017-03-09 16:45:02]

On 2017/03/09 16:29:56, bokan wrote:
>
https://codereview.chromium.org/2738173002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/MediaValues.cpp (right):
> 
>
https://codereview.chromium.org/2738173002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/MediaValues.cpp:40: return
> frame->view()->viewportSizeForViewportUnits().height();
> I think this is slightly wrong. The reason we have a specific viewportSize for
> ViewportUnits is because 100vh is actually taller than the ICB by the height
of 
> the URL bar and I think Media Queries should really report the ICB. I would
> either add a zoomedLayoutSize method on FrameView or keep the zoom logic in
> here.

It just occurred to me that this is pretty subtle and not documented. Since
you're already here, could you please add a comment to
viewportSizeForViewportUnits (on FrameView and LayoutView) about the difference
between this and ICB height?

[rune, 2017-03-09 17:47:36]

https://codereview.chromium.org/2738173002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/MediaValues.cpp (right):

https://codereview.chromium.org/2738173002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/MediaValues.cpp:40: return
frame->view()->viewportSizeForViewportUnits().height();
On 2017/03/09 16:29:56, bokan wrote:
> I think this is slightly wrong. The reason we have a specific viewportSize for
> ViewportUnits is because 100vh is actually taller than the ICB by the height
of
> the URL bar and I think Media Queries should really report the ICB. I would
> either add a zoomedLayoutSize method on FrameView or keep the zoom logic in
> here.

Why is there a difference between ICB and viewport units? The documentation says
it's in order to match Safari. Is that the only reason? 100% height relative to
the ICB is different from 100vh in Chromium, which is strange:

http://jsbin.com/bepejugowu/1/edit?html,css,js,output

Based on that jsbin snippet vh is relative to the whole screen while the ICB is
relative to the available space without the URL-bar. I noticed that layout ng is
actually using viewportSizeForViewportUnits for its constraint space. That would
be different from the current code afaict?

[bokan, 2017-03-09 18:21:25]

On 2017/03/09 17:47:36, rune wrote:
>
https://codereview.chromium.org/2738173002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/MediaValues.cpp (right):
> 
>
https://codereview.chromium.org/2738173002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/MediaValues.cpp:40: return
> frame->view()->viewportSizeForViewportUnits().height();
> On 2017/03/09 16:29:56, bokan wrote:
> > I think this is slightly wrong. The reason we have a specific viewportSize
for
> > ViewportUnits is because 100vh is actually taller than the ICB by the height
> of
> > the URL bar and I think Media Queries should really report the ICB. I would
> > either add a zoomedLayoutSize method on FrameView or keep the zoom logic in
> > here.
> 
> Why is there a difference between ICB and viewport units? The documentation
says
> it's in order to match Safari. Is that the only reason? 100% height relative
to
> the ICB is different from 100vh in Chromium, which is strange:
> 
> http://jsbin.com/bepejugowu/1/edit?html,css,js,output
> 
> Based on that jsbin snippet vh is relative to the whole screen while the ICB
is
> relative to the available space without the URL-bar.

Yes, its not satisfying but we decided that interop trumps rationality in terms
of developer pain. Unfortunately, I couldn't get Safari engineers didn't respond
to this in hopes that they'd agree and change to make 100vh == 100%, so we ended
up just matching their behavior. One possible way out of this is to add a CSS
property (similar to box-sizing) to specify whether 100vh means "largest
possible viewport", "smallest possible", or "dynamically resize". There's cases
for each so this would be helpful and would also help explain some of the
irrationality (position: fixed makes vh and % resize in response to the URL
bar). I haven't yet had time to think about this more and propose it though.

> I noticed that layout ng is
> actually using viewportSizeForViewportUnits for its constraint space. That
would
> be different from the current code afaict?

Yes, in that case I believe that's a mistake.

[rune, 2017-03-09 22:04:46]

On 2017/03/09 16:45:02, bokan wrote:
> On 2017/03/09 16:29:56, bokan wrote:

> It just occurred to me that this is pretty subtle and not documented. Since
> you're already here, could you please add a comment to
> viewportSizeForViewportUnits (on FrameView and LayoutView) about the
difference
> between this and ICB height?

There is
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra...
already. Did you mean adding documentation to the header files?

[rune, 2017-03-09 22:04:55]

https://codereview.chromium.org/2738173002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/MediaValues.cpp (right):

https://codereview.chromium.org/2738173002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/MediaValues.cpp:40: return
frame->view()->viewportSizeForViewportUnits().height();
On 2017/03/09 16:29:56, bokan wrote:
> I think this is slightly wrong. The reason we have a specific viewportSize for
> ViewportUnits is because 100vh is actually taller than the ICB by the height
of
> the URL bar and I think Media Queries should really report the ICB. I would
> either add a zoomedLayoutSize method on FrameView or keep the zoom logic in
> here.

I added a new viewportSizeForMediaQueries() instead.

https://codereview.chromium.org/2738173002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2738173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1410: zoom =
frame().pageZoomFactor();
I have kept this change since this is what fixes issue 699910.

[rune, 2017-03-09 22:20:29]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 22:20:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2017-03-09 22:56:22]

On 2017/03/09 22:04:46, rune wrote:
> On 2017/03/09 16:45:02, bokan wrote:
> > On 2017/03/09 16:29:56, bokan wrote:
> 
> > It just occurred to me that this is pretty subtle and not documented. Since
> > you're already here, could you please add a comment to
> > viewportSizeForViewportUnits (on FrameView and LayoutView) about the
> difference
> > between this and ICB height?
> 
> There is
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra...
> already. Did you mean adding documentation to the header files?

Yes, the current comment is clearly not very visible.

[bokan, 2017-03-09 22:58:06]

Thanks, lgtm

https://codereview.chromium.org/2738173002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2738173002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:1410: zoom =
frame().pageZoomFactor();
On 2017/03/09 22:04:55, rune wrote:
> I have kept this change since this is what fixes issue 699910.

Acknowledged.

[commit-bot: I haz the power, 2017-03-10 00:08:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 00:08:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[rune, 2017-03-10 00:09:37]

The CQ bit was checked by rune@opera.com

[rune, 2017-03-10 00:09:37]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2738173002/#ps40001
(title: "Added documentation.")

[commit-bot: I haz the power, 2017-03-10 00:10:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 01:27:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 01:27:36]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[rune, 2017-03-10 09:53:30]

The CQ bit was checked by rune@opera.com

[rune, 2017-03-10 09:53:30]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2738173002/#ps60001
(title: "Missing IncludeScrollbars.")

[commit-bot: I haz the power, 2017-03-10 09:53:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 11:12:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489139610491560,
"parent_rev": "f4366520e5828318c92113012e02f1027779a792", "commit_rev":
"de209f3db7bbfda754be262cef35b67340c37774"}

[commit-bot: I haz the power, 2017-03-10 11:13:03]

Description was changed from

==========
Initial containing block for print not affected by page zoom.

The page zoom factor is not applied to the ICB for printing, yet the
ICB basis for media queries and viewport units were. Use 1 as a page
zoom factor in viewportSizeForViewportUnits and use that method to get
the size for the ICB for evaluating media queries.

BUG=699014,699910
==========

to

==========
Initial containing block for print not affected by page zoom.

The page zoom factor is not applied to the ICB for printing, yet the
ICB basis for media queries and viewport units were. Use 1 as a page
zoom factor in viewportSizeForViewportUnits and use that method to get
the size for the ICB for evaluating media queries.

BUG=699014,699910

Review-Url: https://codereview.chromium.org/2738173002
Cr-Commit-Position: refs/heads/master@{#456040}
Committed:
https://chromium.googlesource.com/chromium/src/+/de209f3db7bbfda754be262cef35...
==========

[commit-bot: I haz the power, 2017-03-10 11:13:04]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/de209f3db7bbfda754be262cef35...