Fix percent height calc for svg docs.

Fix percent height calc for svg docs.

Previously, percent-height svg elements in a pure-svg doc would get
their size based on the LayoutView's logical height from the previous
layout pass.  In practice, this didn't show up much because FrameView
always does at least one extra initial layout pass to add and then
subtract scrollbars.  The extra layout pass would cause the percent
height descendants to get the right size.

Setting overlay scrollbars on the FrameView eliminates the extra
layout passes and allows the bug to manifest.

Review-Url: https://codereview.chromium.org/2835153002
Cr-Commit-Position: refs/heads/master@{#469327}
Committed: https://chromium.googlesource.com/chromium/src/+/cad9446c3ae02d9d97d4e0223afa6002eeb2e48a

[szager1, 2017-04-25 17:23:52]

Description was changed from

==========
Fix percent height calc for svg docs.

BUG=
==========

to

==========
Fix percent height calc for svg docs.
==========

[szager1, 2017-04-25 17:24:09]

szager@chromium.org changed reviewers:
+ pdr@chromium.org

[szager1, 2017-04-25 17:26:53]

Description was changed from

==========
Fix percent height calc for svg docs.
==========

to

==========
Fix percent height calc for svg docs.

Previously, percent-height svg elements in a pure-svg doc would get their size
based on the LayoutView's logical height from the previous layout pass.  In
practice, this didn't show up much because FrameView always does at least one
extra initial layout pass to add and then subtract scrollbars.  The extra layout
pass would cause the percent height descendants to get the right size.

Setting overlay scrollbars on the FrameView eliminates the extra layout passes
and allows the bug to manifest.
==========

[pdr., 2017-04-26 03:05:04]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 03:05:37]

Dry run: CQ is trying da patch.

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

[pdr., 2017-04-26 04:56:47]

https://codereview.chromium.org/2835153002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/custom/masking-clipping-hidpi.svg
(right):

https://codereview.chromium.org/2835153002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/custom/masking-clipping-hidpi.svg:3: if
(self.internals)
This test doesn't fail for me without the patch (OSX). Does it fail for you? I
tried with other flags such as --root-layer-scrolls but still couldn't get it to
fail.

I found a few tests that did run the code above (e.g., set relayout_children =
true;) such as svg/custom/svg-image-initial-size.html but these seem to pass
without the change. Why do these tests hit the new code but aren't affected by
it?

The testrunner has some special svg sizing code. Is it possible to reproduce
this outside the testrunner?

Can you add a simpler test? I had trouble digging much deeper in this review
without being able to repro.

https://codereview.chromium.org/2835153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2835153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:284: // For a pure SVG doc,
the svg root will have percentage height, which will be
Is this the tree we're considering?
frameview -> layoutview -> svgroot_with_percentage_height ->
svg_child_with_relative_lengths

If so, is this the chain of events?
1) begin layout, layoutview has 0,0 height
2) with existing code, relayout_children is false
3) layout the svg root
4) update the layout view size
5) update the frameview size
6) finish layout

https://codereview.chromium.org/2835153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:292: if (!relayout_children
&& GetDocument().IsSVGDocument()) {
Can you use GetDocument().AccessSVGExtensions().rootElement() to simplify this?

Something like:
if (!relayout_children && GetDocument().IsSVGDocument()) {
  if (auto* svg_root = GetDocument().AccessSVGExtensions().rootElement()) {
    const Length& svg_root_height = svg_root->Style()->LogicalHeight();
    if (svg_root_height.IsPercentOrCalc()) {
      if (svg_root->LogicalHeight() !=
          ValueForLength(svg_root_height,
                        AvailableLogicalHeightUsing(
                            svg_root_height, kExcludeMarginBorderPadding))) {
        relayout_children = true;
        layout_scope.SetChildNeedsLayout(child);
      }
    }
  }
}

[commit-bot: I haz the power, 2017-04-26 05:23:48]

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

[commit-bot: I haz the power, 2017-04-26 05:23:48]

Dry run: This issue passed the CQ dry run.

[szager1, 2017-04-26 10:40:24]

Description was changed from

==========
Fix percent height calc for svg docs.

Previously, percent-height svg elements in a pure-svg doc would get their size
based on the LayoutView's logical height from the previous layout pass.  In
practice, this didn't show up much because FrameView always does at least one
extra initial layout pass to add and then subtract scrollbars.  The extra layout
pass would cause the percent height descendants to get the right size.

Setting overlay scrollbars on the FrameView eliminates the extra layout passes
and allows the bug to manifest.
==========

to

==========
Fix percent height calc for svg docs.

Previously, percent-height svg elements in a pure-svg doc would get
their size based on the LayoutView's logical height from the previous
layout pass.  In practice, this didn't show up much because FrameView
always does at least one extra initial layout pass to add and then
subtract scrollbars.  The extra layout pass would cause the percent
height descendants to get the right size.

Setting overlay scrollbars on the FrameView eliminates the extra
layout passes and allows the bug to manifest.
==========

[szager1, 2017-04-26 12:11:30]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[szager1, 2017-04-26 12:11:36]

https://codereview.chromium.org/2835153002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/svg/custom/masking-clipping-hidpi.svg
(right):

https://codereview.chromium.org/2835153002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/svg/custom/masking-clipping-hidpi.svg:3: if
(self.internals)
On 2017/04/26 04:56:47, pdr. wrote:
> This test doesn't fail for me without the patch (OSX). Does it fail for you? I
> tried with other flags such as --root-layer-scrolls but still couldn't get it
to
> fail.

It fails for me on linux.
 
> I found a few tests that did run the code above (e.g., set relayout_children =
> true;) such as svg/custom/svg-image-initial-size.html but these seem to pass
> without the change. Why do these tests hit the new code but aren't affected by
> it?

As I mentioned in another comment, this doesn't typically manifest on first
layout, because the frame view size is stable by the time first layout runs. 
It's the call to setBackingScaleFactor after first layout that causes the
problem.


> The testrunner has some special svg sizing code. Is it possible to reproduce
> this outside the testrunner?

I couldn't figure out how to change the zoom factor from a unit test. 
ChromeClient doesn't offer anything useful.

> Can you add a simpler test? I had trouble digging much deeper in this review
> without being able to repro.

I can add a simpler test, but if this one doesn't reproduce for you, then most
likely a simplified test won't reproduce either.

https://codereview.chromium.org/2835153002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/2835153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:284: // For a pure SVG doc,
the svg root will have percentage height, which will be
On 2017/04/26 04:56:47, pdr. wrote:
> Is this the tree we're considering?
> frameview -> layoutview -> svgroot_with_percentage_height ->
> svg_child_with_relative_lengths
> 
> If so, is this the chain of events?
> 1) begin layout, layoutview has 0,0 height
> 2) with existing code, relayout_children is false
> 3) layout the svg root
> 4) update the layout view size
> 5) update the frameview size
> 6) finish layout

Not exactly.  First layout is typically not a problem, because it winds up doing
additional layouts with the frame size stable.

The reason it shows up in the masking-clipping-hidpi.svg test is because the
FrameView size changes (due to the zoom level change) *after* the first layout. 
It goes like this:

WebViewImpl::ResizeViewWhileAnchored
  - FrameView::layout_size_ gets updated
  ...
  FrameView::UpdateLayout
    LayoutView::UpdateLayout
      - On this pass, relayout_children is true
    Document::LayoutUpdated()
      ChromeClientImpl::LayoutUpdated()
        WebViewImpl::LayoutUpdated()
          - FrameView::size_ gets updated
          FrameView::UpdateLayout
            LayoutView::UpdateLayout
              - On this pass, relayout_children is false

On the first layout pass, relayout_children is true because the
FrameView::LayoutSize() changed.  During this pass, the svg root resolves its
size against LayoutView::LogicalHeight() *before* the LayoutView calls
UpdateLogicalHeight().  So, it's based on the LayoutView's LogicalHeight from
the *previous* layout pass (i.e., the unzoomed height).  LayoutView runs
UpdateLogicalHeight *after* laying out its children; at that point, the
LayoutView's logical height is set based on (the new) FrameView::LayoutSize().

On the second layout pass, FrameView::Size() has changed, but relayout_children
is false because the FrameView::LayoutSize() did not change since the first
layout pass.  So, the svg root does not run UpdateLayout, and the new
LayoutView::LogicalHeight() from the first layout pass is never used to resolve
the svg root's height.

https://codereview.chromium.org/2835153002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutView.cpp:292: if (!relayout_children
&& GetDocument().IsSVGDocument()) {
On 2017/04/26 04:56:47, pdr. wrote:
> Can you use GetDocument().AccessSVGExtensions().rootElement() to simplify
this?
> 
> Something like:
> if (!relayout_children && GetDocument().IsSVGDocument()) {
>   if (auto* svg_root = GetDocument().AccessSVGExtensions().rootElement()) {
>     const Length& svg_root_height = svg_root->Style()->LogicalHeight();
>     if (svg_root_height.IsPercentOrCalc()) {
>       if (svg_root->LogicalHeight() !=
>           ValueForLength(svg_root_height,
>                         AvailableLogicalHeightUsing(
>                             svg_root_height, kExcludeMarginBorderPadding))) {
>         relayout_children = true;
>         layout_scope.SetChildNeedsLayout(child);
>       }
>     }
>   }
> }

Yep, this works; I updated the patch.

[commit-bot: I haz the power, 2017-04-26 12:11:39]

Dry run: 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-04-26 13:59:39]

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

[commit-bot: I haz the power, 2017-04-26 13:59:40]

Dry run: This issue passed the CQ dry run.

[pdr., 2017-04-27 05:25:01]

On 2017/04/26 at 12:11:36, szager wrote:
> As I mentioned in another comment, this doesn't typically manifest on first
layout, because the frame view size is stable by the time first layout runs. 
It's the call to setBackingScaleFactor after first layout that causes the
problem.

setBackingScaleFactor is only used for tests. Are we just missing a force layout
in that function?

I think we should fix this another way unless we can hit this with the real
browser.

[szager1, 2017-04-27 09:36:21]

On 2017/04/27 05:25:01, pdr. wrote:
> On 2017/04/26 at 12:11:36, szager wrote:
> > As I mentioned in another comment, this doesn't typically manifest on first
> layout, because the frame view size is stable by the time first layout runs. 
> It's the call to setBackingScaleFactor after first layout that causes the
> problem.
> 
> setBackingScaleFactor is only used for tests. Are we just missing a force
layout
> in that function?

The key factor is resizing the window.

> I think we should fix this another way unless we can hit this with the real
> browser.

I can reproduce it in stable chrome on linux with the overlay-scrollbars flag
set to enabled, by resizing the browser window.

As I said before, the problem tends to get masked currently because we do extra
layout passes to add and subtract FrameView scrollbars.  My other patch
refactors a bunch of that and eliminates some of the extra layout, and that
causes the problem to manifest even without enabling overlay scrollbars.  That
patch is critical for root layer scrolling.

[pdr., 2017-04-28 00:57:19]

Can you describe how to reproduce this in a browser?

I think this may not reproduce on OSX because overlay scrollbar support is
different there (e.g., using --enable-overlay-scrollbar won't work). I tried
this on Windows where the flag does work and loaded masking-clipping-hidpi.svg
and tried resizing without issue. I tried adding percentage heights as well and
everything seems to work.

[szager1, 2017-04-28 10:42:29]

On 2017/04/28 00:57:19, pdr (OOO until May 3rd) wrote:
> Can you describe how to reproduce this in a browser?
> 
> I think this may not reproduce on OSX because overlay scrollbar support is
> different there (e.g., using --enable-overlay-scrollbar won't work). I tried
> this on Windows where the flag does work and loaded masking-clipping-hidpi.svg
> and tried resizing without issue. I tried adding percentage heights as well
and
> everything seems to work.

Linux repro:

1. Resize browser window to 800x600
2. Open masking-clipping-hidpi.svg
3. Open inspector
4. Select the height=100% rect in the inspector.
4. $0->getBoundingClientRect().height should be 600px.
5. Press Alt-F8 to begin window resize.
6. Press up arrow once (this should shorten the window by 10px).
7. $0->getBoundingClientRect() will still be 600px; it should be 590px.

[szager1, 2017-04-28 10:43:06]

On 2017/04/28 10:42:29, szager1 wrote:
> On 2017/04/28 00:57:19, pdr (OOO until May 3rd) wrote:
> > Can you describe how to reproduce this in a browser?
> > 
> > I think this may not reproduce on OSX because overlay scrollbar support is
> > different there (e.g., using --enable-overlay-scrollbar won't work). I tried
> > this on Windows where the flag does work and loaded
masking-clipping-hidpi.svg
> > and tried resizing without issue. I tried adding percentage heights as well
> and
> > everything seems to work.
> 
> Linux repro:
> 
> 1. Resize browser window to 800x600
> 2. Open masking-clipping-hidpi.svg
> 3. Open inspector
> 4. Select the height=100% rect in the inspector.
> 4. $0->getBoundingClientRect().height should be 600px.
> 5. Press Alt-F8 to begin window resize.
> 6. Press up arrow once (this should shorten the window by 10px).
> 7. $0->getBoundingClientRect() will still be 600px; it should be 590px.

I forgot:

6.5 Press Enter to finish window resize

[pdr., 2017-04-28 20:16:49]

On 2017/04/28 at 10:43:06, szager wrote:
> On 2017/04/28 10:42:29, szager1 wrote:
> > On 2017/04/28 00:57:19, pdr (OOO until May 3rd) wrote:
> > > Can you describe how to reproduce this in a browser?
> > > 
> > > I think this may not reproduce on OSX because overlay scrollbar support is
> > > different there (e.g., using --enable-overlay-scrollbar won't work). I
tried
> > > this on Windows where the flag does work and loaded
masking-clipping-hidpi.svg
> > > and tried resizing without issue. I tried adding percentage heights as
well
> > and
> > > everything seems to work.
> > 
> > Linux repro:
> > 
> > 1. Resize browser window to 800x600
> > 2. Open masking-clipping-hidpi.svg
> > 3. Open inspector
> > 4. Select the height=100% rect in the inspector.
> > 4. $0->getBoundingClientRect().height should be 600px.
> > 5. Press Alt-F8 to begin window resize.
> > 6. Press up arrow once (this should shorten the window by 10px).
> > 7. $0->getBoundingClientRect() will still be 600px; it should be 590px.
> 
> I forgot:
> 
> 6.5 Press Enter to finish window resize

I can reproduce this but only on linux and not when drag-resizing the window.
Are we just missing some invalidation codepath in the alt+f8 resize code?

[szager1, 2017-04-28 20:19:25]

On 2017/04/28 20:16:49, pdr (OOO until May 3rd) wrote:
> On 2017/04/28 at 10:43:06, szager wrote:
> > On 2017/04/28 10:42:29, szager1 wrote:
> > > On 2017/04/28 00:57:19, pdr (OOO until May 3rd) wrote:
> > > > Can you describe how to reproduce this in a browser?
> > > > 
> > > > I think this may not reproduce on OSX because overlay scrollbar support
is
> > > > different there (e.g., using --enable-overlay-scrollbar won't work). I
> tried
> > > > this on Windows where the flag does work and loaded
> masking-clipping-hidpi.svg
> > > > and tried resizing without issue. I tried adding percentage heights as
> well
> > > and
> > > > everything seems to work.
> > > 
> > > Linux repro:
> > > 
> > > 1. Resize browser window to 800x600
> > > 2. Open masking-clipping-hidpi.svg
> > > 3. Open inspector
> > > 4. Select the height=100% rect in the inspector.
> > > 4. $0->getBoundingClientRect().height should be 600px.
> > > 5. Press Alt-F8 to begin window resize.
> > > 6. Press up arrow once (this should shorten the window by 10px).
> > > 7. $0->getBoundingClientRect() will still be 600px; it should be 590px.
> > 
> > I forgot:
> > 
> > 6.5 Press Enter to finish window resize
> 
> I can reproduce this but only on linux and not when drag-resizing the window.
> Are we just missing some invalidation codepath in the alt+f8 resize code?

No, we're not.  Try this:

- Resize window to 590px
- $0->getBoundingClientRect().height will be 600px
- Resize window to 580px
- $0->getBoundingClientRect().height will be 590px
...

It's always exactly one layout behind.

[pdr., 2017-04-28 20:22:29]

On 2017/04/28 at 20:19:25, szager wrote:
> It's always exactly one layout behind.

Oh I see, you are correct.

[pdr., 2017-04-29 03:38:38]

Looked into this a little more and I agree with your analysis.

I'm not a big fan of the approach in this patch because it adds an extra layout
that just works around the bug, and this new extra layout hits on pages that
don't reproduce the bug.

I think we might be able to fix the root issue here. WDYT of adding the
following at the bottom of LayoutBox::ComputeLogicalHeight?:
... } else if (IsSVG() && IsDocumentElement() &&
HeightForDocumentElement(GetDocument()).IsPercentOrCalc()) {
  // SVG documents with percentage heights can immediately grab the view's
  // layout height instead of relying on a second layout.
  computed_values.extent_ = View()->ViewLogicalHeightForPercentages();
}

[szager1, 2017-04-30 12:20:59]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-30 12:22:30]

Dry run: CQ is trying da patch.

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

[szager1, 2017-04-30 12:24:14]

On 2017/04/29 03:38:38, pdr (OOO until May 3rd) wrote:
> Looked into this a little more and I agree with your analysis.
> 
> I'm not a big fan of the approach in this patch because it adds an extra
layout
> that just works around the bug, and this new extra layout hits on pages that
> don't reproduce the bug.
> 
> I think we might be able to fix the root issue here. WDYT of adding the
> following at the bottom of LayoutBox::ComputeLogicalHeight?:
> ... } else if (IsSVG() && IsDocumentElement() &&
> HeightForDocumentElement(GetDocument()).IsPercentOrCalc()) {
>   // SVG documents with percentage heights can immediately grab the view's
>   // layout height instead of relying on a second layout.
>   computed_values.extent_ = View()->ViewLogicalHeightForPercentages();
> }

I did something along those lines, see how you like it.

[commit-bot: I haz the power, 2017-04-30 14:18:04]

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

[commit-bot: I haz the power, 2017-04-30 14:18:05]

Dry run: 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_...)

[szager1, 2017-04-30 18:50:57]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-30 18:51:10]

Dry run: CQ is trying da patch.

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

[szager1, 2017-04-30 18:51:27]

Switched to ViewLogicalHeightForPercentages; PTAL

[szager1, 2017-04-30 18:54:31]

The CQ bit was checked by szager@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-30 18:54:37]

Dry run: 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-04-30 20:54:45]

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

[commit-bot: I haz the power, 2017-04-30 20:54:46]

Dry run: This issue passed the CQ dry run.

[pdr., 2017-05-03 15:27:23]

Lgtm with a minimal test

https://codereview.chromium.org/2835153002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/svg/custom/masking-clipping-hidpi.svg
(right):

https://codereview.chromium.org/2835153002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/svg/custom/masking-clipping-hidpi.svg:3: if
(self.internals)
Can you add a minimal test that just covers this bug? I could see someone
rebaselining this without realizing the implications.

[szager1, 2017-05-04 12:32:58]

The CQ bit was checked by szager@chromium.org

[szager1, 2017-05-04 12:32:58]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2835153002/#ps120001
(title: "Split percent height test into percent-height.svg")

[commit-bot: I haz the power, 2017-05-04 12:33:12]

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-05-04 14:28:29]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493901178349990,
"parent_rev": "7289d3fc394829aefc58cf79bc4d1d5b51810b50", "commit_rev":
"cad9446c3ae02d9d97d4e0223afa6002eeb2e48a"}

[commit-bot: I haz the power, 2017-05-04 14:28:40]

Description was changed from

==========
Fix percent height calc for svg docs.

Previously, percent-height svg elements in a pure-svg doc would get
their size based on the LayoutView's logical height from the previous
layout pass.  In practice, this didn't show up much because FrameView
always does at least one extra initial layout pass to add and then
subtract scrollbars.  The extra layout pass would cause the percent
height descendants to get the right size.

Setting overlay scrollbars on the FrameView eliminates the extra
layout passes and allows the bug to manifest.
==========

to

==========
Fix percent height calc for svg docs.

Previously, percent-height svg elements in a pure-svg doc would get
their size based on the LayoutView's logical height from the previous
layout pass.  In practice, this didn't show up much because FrameView
always does at least one extra initial layout pass to add and then
subtract scrollbars.  The extra layout pass would cause the percent
height descendants to get the right size.

Setting overlay scrollbars on the FrameView eliminates the extra
layout passes and allows the bug to manifest.

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

[commit-bot: I haz the power, 2017-05-04 14:28:41]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/cad9446c3ae02d9d97d4e0223afa...