[CSS Grid Layout] Avoid using StyleAdjuster to resolve 'auto' values.

[CSS Grid Layout] Avoid using StyleAdjuster to resolve 'auto' values.

CSS Box Alignment specification defines a particular way of resolving
'auto' values for the alignment properties, resulting in different
values depending on the kind of the styled element. Because of this, we
were resolving such values during the style cascade resolution.

This approach implies that we need to Reattach the style whenever a
change is made in the value of these properties, in order to perform
again the style cascade resolution.

The Reattach causes many issues so we need to explore a different approach
to perform the 'auto' values resolution. This patch delegates this logic
to the appropriated LayoutObjects during the layout, where these values
are actually used.

The cons of this approach is that we need to duplicated part of this code
to implement the computed style logic for these properties, which I think
is something we can assume.

BUG=249451, 376823

[jfernandez, 2015-05-13 22:42:56]

jfernandez@igalia.com changed reviewers:
+ dsinclair@chromium.org, esprehn@chromium.org, jchaffraix@chromium.org

[jfernandez, 2015-05-14 14:20:45]

The patch for this issue changes considerably the repaint area caused by
alignment style changes. There are many new repaint tests added in issue
1130843004, which are now   modified by this patch.

[jfernandez, 2015-05-15 11:07:05]

Patch Set 6 provides a small optimization, since calling to defaultJustifyItems
causes a performance regression in the grid related tests. The reason is that
positionForColumn is called for every grid item when it's being positioned and
having to check out for the grid container's containing block looking for values
using the "legacy" keyword adds an unnecessary overhead. 

However, we need to take that into account, hence I propose to do it in the
StyleAdjuster logic (only for this specific case) and whenever alignment style
changes (see changes at LayoutBox::styleDidChange).

It's worth mentioning that this change implies to modify the LayoutObject's
computed style instance (it's usually persistent) by calling to
mutableStyleRef(), which I'm not sure is correct/usual.

[rune, 2015-05-15 14:53:15]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2015-05-15 14:53:16]

Drive-by-comments:

I haven't looked at the code in detail yet, but not doing the adjustments on
ComputedStyle means that the values returned from getComputedStyle() changes.
The values on ComputedStyle should follow what the relevant spec says about what
the computed style should be in order to get the correct values out of the
CSSOM.

If the existing re-attach is too brutal, it should be possible to return a more
correct change from the diff-method that triggers the correct layout instead.

[esprehn, 2015-05-15 15:06:49]

What are the problems with reattach? Going through a reattach should be largely
transparent except for plugins. It's too much work for this certainly, but it
shouldn't be causing issues.

Also I feel like we're playing whack-a-mole with this feature. We keep moving it
from StyleAdjuster -> layout -> back and around.

Something doesn't seem right with either the spec or our implementation that we
keep having to move this around. Can you make a design doc to explain what
you're doing here? I'm concerned we don't have a plan.

[jfernandez, 2015-05-15 16:00:19]

On 2015/05/15 14:53:16, rune wrote:
> Drive-by-comments:
> 
> I haven't looked at the code in detail yet, but not doing the adjustments on
> ComputedStyle means that the values returned from getComputedStyle() changes.
> The values on ComputedStyle should follow what the relevant spec says about
what
> the computed style should be in order to get the correct values out of the
> CSSOM.
> 

The 'auto' values are resolved by the computed style logic by itself; it's kind
of code
duplication but, somehow, unavoidable. The StyleAdjustment logic was the simpler
and more natural way of resolving the 'auto' values when they are initially
load. There
are certain cases, like style changes via JS which makes the StyleAdjustment
logic not
enough and hence, they must be resolved at computed style or even during layout.


> If the existing re-attach is too brutal, it should be possible to return a
more
> correct change from the diff-method that triggers the correct layout instead.

I've been told by @jchaffraix that it may cause issues, with plugins for
instance. It was
the easier way to deal with style changes on properties that impact the value of
children's
properties (eg, align-items, justify-items). 

Assuming that we will have to resolve 'auto' values for both, computed style and
layout,
I thought that I could try to change the current approach, also thinking on
additional
improvements, as I'll explain bellow.

[jfernandez, 2015-05-15 16:10:59]

On 2015/05/15 15:06:49, esprehn wrote:
> What are the problems with reattach? Going through a reattach should be
largely
> transparent except for plugins. It's too much work for this certainly, but it
> shouldn't be causing issues.

Well, plugins was indeed a problem @jchaffraix mentioned some weeks ago. The
issue appeared when I was implementing some repaint tests for the alignment
properties. It was @jchaffraix who commented that the repaint tests caused too
many invalidations. As I could verify later, most of them are caused by the
Reattach
and multiple layouts performed, perhaps unnecessarily, as a consequence of
changes
on this properties.   

> 
> Also I feel like we're playing whack-a-mole with this feature. We keep moving
it
> from StyleAdjuster -> layout -> back and around.

Well, personally I think the approach of using the StyleAdjuster for resolving
the
'auto' values is the way to go. Perhaps is worth mentioning that the same
approach
was rejected in WebKit because it impacted performance due the copy of data when
resolving the 'auto' values (default/initial value) on elements even not using
the
alignment at all. 

There are other cases, like LayoutFullScreen, LayoutButton, which derive from
LayoutFlexibleBox and expect to have the 'stretch' value as default even that
they are not using the display 'flex', which is what the StyleAdjuster uses for
determining the type of element. This makes unavoidable of resolving the values
anyway at layout phase, as I commented before. 

So this change, and some other ones I'm working on, are just a way of doing
some improvements in terms of performance (in some cases), style and paint
invalidations and avoid unnecessary layouts. 

It might be the case that this is too much work for the benefits I could
eventually
get, but I wanted to give it a try. I'd appreciate any feedback or suggestions
on
this regard.

> 
> Something doesn't seem right with either the spec or our implementation that
we
> keep having to move this around. Can you make a design doc to explain what
> you're doing here? I'm concerned we don't have a plan.

[cbiesinger, 2015-05-18 23:06:20]

My opinion is that putting the code in StyleAdjuster is essentially duplicating
a lot of code in computed style and in flexbox/grid, and that duplication is a
bad thing. Furthermore, each adjustment in StyleAdjuster makes the code harder
to understand, because now you magically changed what's in ComputedStyle without
any stylesheet anywhere saying so -- e.g. it does not show up in inspector's CSS
view.

So, I'm in favor of this change, though I haven't read the patch in detail. (I
also haven't followed the history of why this code keeps moving around)

[jfernandez, 2015-05-19 08:35:36]

On 2015/05/18 23:06:20, cbiesinger wrote:
> My opinion is that putting the code in StyleAdjuster is essentially
duplicating
> a lot of code in computed style and in flexbox/grid, and that duplication is a
> bad thing. Furthermore, each adjustment in StyleAdjuster makes the code harder
> to understand, because now you magically changed what's in ComputedStyle
without
> any stylesheet anywhere saying so -- e.g. it does not show up in inspector's
CSS
> view.

I'm afraid that code duplication is something hard to avoid for this issue.
Precisely, 
the StyleAdjuster logic was an intent to avoid it, resolving all the 'auto'
values during
the style cascade resolution. Unfortunately, several layout objects reuse the
flexbox
implementation without using the proper display value; style changes made by JS
or
even during layout don't trigger the StyleAdjuster logic. But we need the 'auto'
values
to be resolved somehow, so it seems the only way to do it for all the cases is
duplicating logic in ComputedStyleValueMapping and on each LayoutObject using
the alignment properties (via ComputedStyle). 

So the point is whether we should keep or not the StyleAdjuster logic assuming
that
we will have to resolve it again, for some cases. It's true that it covers style
loading
but I wonder how impacts performance. Precisely, as I commented before,
performance
was the reason why it was discarded in WebKit. I was at the last WK hackfest
reviewing
this issue with Hyatt and he was the one suggesting the idea of using layout for
resolving the auto values, assuming the code duplication for getting computed
values.

> 
> So, I'm in favor of this change, though I haven't read the patch in detail. (I
> also haven't followed the history of why this code keeps moving around)

The issue that motivated this patch, and others I'm still working on, is
improving
performance, reducing layouts and style invalidations, optimizing the repaint
tests results minimizing the invalidated areas. The StyleAdjuster logic required
to perform a Reattach whenever some of the alignment properties changed. This
approach, besides causing problems with Plugins, implies a full layout and paint
invalidation of a many boxes which haven't changed. 

I'm not sure whether, still, the performance gain will be enough, it will for
the specific
case of the alignment properties change but I'm not sure if it's a use case we
want to optimize. I'd like to get feedback as well on the repaint tests issues,
how
important is for us to minimize the repainting area when alignment changes. It'd
be
awesome to  gather ideas of better ways of invalidating children styles when the
Default Alignment properties (align-items and justify-items) change.

[dsinclair, 2015-08-05 18:10:38]

dsinclair@chromium.org changed reviewers:
- dsinclair@chromium.org

[jfernandez, 2016-08-23 08:11:20]

We don't need to land this patch because we have decided to implement a
different approach.