Relayout RenderObjects with viewport-percentage CSS properties when viewport size changes

Relayout RenderObjects with viewport-percentage CSS properties when viewport size changes

Previously if a RenderObject with viewport-percentage CSS properties
is under another RenderObject which doesn't need to relayout, when the
viewport size changes, the former doesn't relayout.

Relayout of viewport-percentage style objects can't depend on the
top-down hierachy but must be triggered by the RenderView when viewport
size changes. Let RenderView manage a list of objects that may have
viewport-percentage style.

This will also simplify the top-down relayout process that is needed for
bug 310875.

BUG=310874
TEST=fast/css/viewport-percentage-relayout.html

[leviw_travelin_and_unemployed, 2013-10-25 20:30:35]

This looks okay to me, but I'm adding a couple other folks to take a look.

https://codereview.chromium.org/43873004/diff/1/Source/core/rendering/RenderV...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/43873004/diff/1/Source/core/rendering/RenderV...
Source/core/rendering/RenderView.cpp:1247:
m_viewportPercentageStyleObjects->add(object);
Can you add asserts that we aren't double-adding or double-removing objects?

https://codereview.chromium.org/43873004/diff/1/Source/core/rendering/style/R...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/43873004/diff/1/Source/core/rendering/style/R...
Source/core/rendering/style/RenderStyle.h:83:
updateMayHaveViewportPercentage(value); \
I'm a little worried that this could be expensive, but I don't see a better way
to do it.

[esprehn, 2013-10-25 22:25:51]

Why not handle this inside style resolve? We need to re-eval media queries when
the viewport size changes anyway, I feel like we should be tracking this at the
DOM node level and doing setNeedsStyleRecalc up there.

Triggering a layout doesn't handle font-size properly.

<div style="font-size: 2vh;">
  <div style="position: absolute">Text</div>
</div>

I don't think this patch is correct.

https://codereview.chromium.org/43873004/diff/1/LayoutTests/fast/css/resource...
File
LayoutTests/fast/css/resources/viewport-percentage-relayout-frame-expected.html
(right):

https://codereview.chromium.org/43873004/diff/1/LayoutTests/fast/css/resource...
LayoutTests/fast/css/resources/viewport-percentage-relayout-frame-expected.html:3:
<div id="viewport-percentage" style="width: 10vw; height: 10vh;
background-color: blue">
You need tests for inherited values like font-size: 1vh to make sure it gets
inherited down properly

https://codereview.chromium.org/43873004/diff/1/Source/core/rendering/RenderO...
File Source/core/rendering/RenderObject.cpp (right):

https://codereview.chromium.org/43873004/diff/1/Source/core/rendering/RenderO...
Source/core/rendering/RenderObject.cpp:2063: if (RenderView* view =
this->view()) {
You don't need to null check this, going through styleDidChange when there's no
view should not be possible

https://codereview.chromium.org/43873004/diff/1/Source/core/rendering/style/R...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/43873004/diff/1/Source/core/rendering/style/R...
Source/core/rendering/style/RenderStyle.h:1084: setClipLeft(left);
Can you put these at the bottom of the file (and the ones above too) since
they're not just one line. We don't usually put longer ones inside the class
def.

[Xianzhu, 2013-10-25 22:36:07]

On 2013/10/25 22:25:51, esprehn wrote:
> Why not handle this inside style resolve? We need to re-eval media queries
when
> the viewport size changes anyway, I feel like we should be tracking this at
the
> DOM node level and doing setNeedsStyleRecalc up there.
> 

Thanks for the info. Will investigate this way.

> Triggering a layout doesn't handle font-size properly.
> 
> <div style="font-size: 2vh;">
>   <div style="position: absolute">Text</div>
> </div>
> 
> I don't think this patch is correct.

This is a pre-existing issue and I planned to file a separate bug about it.

[esprehn, 2013-10-26 00:04:01]

On 2013/10/25 22:36:07, Xianzhu wrote:
> On 2013/10/25 22:25:51, esprehn wrote:
> > Why not handle this inside style resolve? We need to re-eval media queries
> when
> > the viewport size changes anyway, I feel like we should be tracking this at
> the
> > DOM node level and doing setNeedsStyleRecalc up there.
> > 
> 
> Thanks for the info. Will investigate this way.
> 
> > Triggering a layout doesn't handle font-size properly.
> > 
> > <div style="font-size: 2vh;">
> >   <div style="position: absolute">Text</div>
> > </div>
> > 
> > I don't think this patch is correct.
> 
> This is a pre-existing issue and I planned to file a separate bug about it.

These should be solved as the same issue. We should be resolving viewport units
at style recalc time so that RenderStyle (and rendering in general) has no
concept of them. When the viewport changes sizes we should walk the set of all
elements that are impacted by viewport units and do
setNeedsStyleRecalc(LocalStyleChange). That will cause an update and the layout
will be updated as well if the units resolve to a different length.

[Timothy Loh, 2013-10-26 00:08:56]

While this approach is an incremental improvement to our viewport unit support,
I'm not sure it really helps us get closer to having viewport units working
completely. My understanding of this is that there are a few reasons why our
viewport unit support isn't great

- We require a RenderView to resolve these in Length objects, which we can't do
in all places (e.g. transforms) because it would be a layering violation.
- Various properties which take a CSS <length> get resolved straight into
numeric types (e.g. shadows, border widths, font size) instead of being stored
as a Length (which is more useful when <percent> is allowed). These either don't
support viewport units, or otherwise don't work on resizes.
- Viewport units don't work with calc, although I suspect we could wire this up
and have them work with calc anywhere they currently work.
- We don't correctly decide when to relayout in the presence of viewport units.

As Elliot mentioned, I think we'll want to resolve viewport units at style
recalc time (I was going to work on this some time in the next couple of weeks
but you can take it up if you're interested). We'd mark the RenderStyles which
use viewport units, and  on resize we'd clear marked RenderStyles from the
MatchedPropertiesCache and ask for style recalcs for appropriate nodes. Given
that we'll probably need to do this for the second point, and it'll also resolve
the other points, I'm not sure if it's worthwhile to improve the current system
where we resolve at layout time, instead of fixing everything at once. WDYT?

[esprehn, 2013-10-26 00:12:04]

On 2013/10/26 00:08:56, Timothy Loh wrote:
> ...I'm not sure if it's worthwhile to improve the current system
> where we resolve at layout time, instead of fixing everything at once. WDYT?

I agree. Lets not do this bandaid, I'd rather we just worked to getting viewport
units working properly.

not lgtm on this patch.