[CSS Grid Layout] Don't resolve align-self and justify-self properties

[CSS Grid Layout] Don't resolve align-self and justify-self properties
during css cascade, but during layout.

There are several reasons why we can't resolve the align-self and
justify-self; anonymous renders and wrappers are not part of the css
cascade, for instance.

There are quite many renders that are implemented using RenderFlexibleBox
but not using the proper display, so they might not want the "stretch"
default behaviour.

Changes in the align-items or justify-items during the layout would
need a style recalculation, which is not desirable. This makes unavoidable
to resolve alignment "auto" values during the layout.

BUG=249451, 376823

[jfernandez, 2014-08-02 00:25:19]

This patch is a complement of issue 363133003. I've thought it was more elegant,
since it resolves completely the align-items and justify-items during the
cascade; now it sets "start" for elements different than flex and grid, as the
specs clearly states.

It also secures that anonymous renders an wrappers properly resolve the
alignment during the layout, which is actually when the alignment happens  an
the property value is needed.

On the other side, it makes the renders, implemented as RenderFlexibleBox but
not setting the proper display value to be "treated" as regular blocks, so
"auto" will be resolved to "start" instead. This might lead on a different
rendering behavior in certain cases, but tests will tell.

[Julien - ping for review, 2014-08-04 16:33:04]

On 2014/08/02 at 00:25:19, jfernandez wrote:
> This patch is a complement of issue 363133003. I've thought it was more
elegant, since it resolves completely the align-items and justify-items during
the cascade; now it sets "start" for elements different than flex and grid, as
the specs clearly states.

Can we test that? (the test change is for flexbox AFAICT)

> It also secures that anonymous renders an wrappers properly resolve the
alignment during the layout, which is actually when the alignment happens  an
the property value is needed.

Same question.

[jfernandez, 2014-08-18 22:05:08]

On 2014/08/04 16:33:04, Julien Chaffraix - PST wrote:
> On 2014/08/02 at 00:25:19, jfernandez wrote:
> > This patch is a complement of issue 363133003. I've thought it was more
> elegant, since it resolves completely the align-items and justify-items during
> the cascade; now it sets "start" for elements different than flex and grid, as
> the specs clearly states.
> 
> Can we test that? (the test change is for flexbox AFAICT)
> 

Well, actually the test plays with the 'display' property so it verifies the
behavior
of the alignment properties for non-flex elements as well. I'm not sure whether
you
are suggesting to add an specific tests for that; my impression is that the test
is
generic enough, but I agree that the folder where it's located is confusing.

> > It also secures that anonymous renders an wrappers properly resolve the
> alignment during the layout, which is actually when the alignment happens  an
> the property value is needed.
> 
> Same question.

There are several scenarios where this is happening; some examples are the
RenderFullScreen,
for which there are already various unit tests, the RenderButton and some
others. It's hard to
actually tests this within the current codebase, since "start" is not
implemented by the
RenderFlexibleBox class; actually, there is an ASSERT guarding this, so it
crashes in debug 
builds. I'm working on a patch for avoiding the assertion, so it would defaults
to "stretch" for
any undefined value.

[esprehn, 2014-08-19 00:56:33]

https://codereview.chromium.org/433153003/diff/1/Source/core/css/CSSComputedS...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/433153003/diff/1/Source/core/css/CSSComputedS...
Source/core/css/CSSComputedStyleDeclaration.cpp:1830: overflow =
parent->computedStyle()->alignItemsOverflowAlignment();
This seems like code duplication, also same issues as below.

https://codereview.chromium.org/433153003/diff/1/Source/core/css/CSSComputedS...
Source/core/css/CSSComputedStyleDeclaration.cpp:2008: if (parent &&
parent->computedStyle()) {
You need to use NodeRenderingTraversal::parent(), parentNode() is not where your
parent style comes from (or where you're going to be inserted into) with Shadow
DOM or pseudo elements. You should also not call computedStyle() repeatedly like
this either.

What are you trying to do here?

https://codereview.chromium.org/433153003/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/433153003/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleAdjuster.cpp:283: if (absolutePositioned) {
You should combine these if statements now.

https://codereview.chromium.org/433153003/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleAdjuster.cpp:295:
style.setAlignItems(ItemPositionStart);
Doesn't this go against what your patch says it does?

https://codereview.chromium.org/433153003/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleAdjuster.cpp:295:
style.setAlignItems(ItemPositionStart);
Doesn't this go against what your patch says it does?

https://codereview.chromium.org/433153003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/433153003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderFlexibleBox.cpp:199: align =
parentStyle->alignItems();
What if your parent is also auto? This doesn't seem like it can work.

[jfernandez, 2014-08-19 21:00:12]

Thanks for the review; replied with comments and some questions.

https://codereview.chromium.org/433153003/diff/1/Source/core/css/CSSComputedS...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/433153003/diff/1/Source/core/css/CSSComputedS...
Source/core/css/CSSComputedStyleDeclaration.cpp:1830: overflow =
parent->computedStyle()->alignItemsOverflowAlignment();
On 2014/08/19 00:56:33, esprehn wrote:
> This seems like code duplication, also same issues as below.

Do you mean code duplication with the logic managing the justify-self property ?

https://codereview.chromium.org/433153003/diff/1/Source/core/css/CSSComputedS...
Source/core/css/CSSComputedStyleDeclaration.cpp:2008: if (parent &&
parent->computedStyle()) {
On 2014/08/19 00:56:33, esprehn wrote:
> You need to use NodeRenderingTraversal::parent(), parentNode() is not where
your
> parent style comes from (or where you're going to be inserted into) with
Shadow
> DOM or pseudo elements. You should also not call computedStyle() repeatedly
like
> this either.
> 

I used parentNode() because it was what it was used in the original code, so
that where the confusion comes from. Also, I had the idea that this
computedStyle thing was a DOM matter, independently of how the render tree is
finally built. 

Anyway, I think I've probably misunderstood the concept, so I'll apply your
suggestions. Of course, I totally agree with avoiding repeatedly calling to
computedStyle. 

> What are you trying to do here?

The idea is that 'auto' values on aling-self are resolved to the aling-items's
computed value on its parent element, as the Box Alignment Specification
describes.

https://codereview.chromium.org/433153003/diff/1/Source/core/css/resolver/Sty...
File Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/433153003/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleAdjuster.cpp:283: if (absolutePositioned) {
On 2014/08/19 00:56:33, esprehn wrote:
> You should combine these if statements now.

Acknowledged.

https://codereview.chromium.org/433153003/diff/1/Source/core/css/resolver/Sty...
Source/core/css/resolver/StyleAdjuster.cpp:295:
style.setAlignItems(ItemPositionStart);
On 2014/08/19 00:56:33, esprehn wrote:
> Doesn't this go against what your patch says it does?

Actually not. The patch precisely tries to resolve align-items and justify-items
during the cascade, leaving the align-self and justify-self for the rendering
phase. The reason is that many render objects change the value of the xxx-items
property to force a re-layout and assign new values to the xxx-self properties
(when defined as 'auto').

[jfernandez, 2014-08-19 21:39:18]

Forgot to reply to one comment.

https://codereview.chromium.org/433153003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/433153003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderFlexibleBox.cpp:199: align =
parentStyle->alignItems();
On 2014/08/19 00:56:33, esprehn wrote:
> What if your parent is also auto? This doesn't seem like it can work.

It shouldn't be; the 'align-items' property value is resolved during the
cascade. It was my previous patch which forced stretch for the cases where
parent was 'auto', but I've decided to go back to the original code to detect
incorrect cases. Actually. there is an ASSERTION to prevent 'auto' values during
the layout.

[jfernandez, 2014-08-19 21:39:21]

Forgot to reply to one comment.

[jfernandez, 2014-09-03 20:53:02]

esprehn, julien could you give me some feeadback about my comments ? Thanks.

[esprehn, 2014-09-03 21:45:28]

https://codereview.chromium.org/433153003/diff/1/Source/core/css/CSSComputedS...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/433153003/diff/1/Source/core/css/CSSComputedS...
Source/core/css/CSSComputedStyleDeclaration.cpp:1830: overflow =
parent->computedStyle()->alignItemsOverflowAlignment();
On 2014/08/19 at 21:00:12, jfernandez wrote:
> On 2014/08/19 00:56:33, esprehn wrote:
> > This seems like code duplication, also same issues as below.
> 
> Do you mean code duplication with the logic managing the justify-self property
?

Yes.

https://codereview.chromium.org/433153003/diff/1/Source/core/css/CSSComputedS...
Source/core/css/CSSComputedStyleDeclaration.cpp:2008: if (parent &&
parent->computedStyle()) {
On 2014/08/19 at 21:00:12, jfernandez wrote:
> On 2014/08/19 00:56:33, esprehn wrote:
> > You need to use NodeRenderingTraversal::parent(), parentNode() is not where
your
> > parent style comes from (or where you're going to be inserted into) with
Shadow
> > DOM or pseudo elements. You should also not call computedStyle() repeatedly
like
> > this either.
> > 
> 
> I used parentNode() because it was what it was used in the original code, so
that where the confusion comes from. Also, I had the idea that this
computedStyle thing was a DOM matter, independently of how the render tree is
finally built. 

I don't understand what you mean, style is resolved by using the distributed
parent per how Shadow DOM works. You can't use parentNode() to check your parent
style.

> 
> Anyway, I think I've probably misunderstood the concept, so I'll apply your
suggestions. Of course, I totally agree with avoiding repeatedly calling to
computedStyle. 
> 
> > What are you trying to do here?
> 
> The idea is that 'auto' values on aling-self are resolved to the aling-items's
computed value on its parent element, as the Box Alignment Specification
describes.

NodeRenderingTraversal::parent() is where your parent style comes from, not from
parentNode(). The traversal version is what deals with shadow dom distributions.

https://codereview.chromium.org/433153003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/433153003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderFlexibleBox.cpp:199: align =
parentStyle->alignItems();
On 2014/08/19 at 21:39:18, jfernandez wrote:
> On 2014/08/19 00:56:33, esprehn wrote:
> > What if your parent is also auto? This doesn't seem like it can work.
> 
> It shouldn't be; the 'align-items' property value is resolved during the
cascade. It was my previous patch which forced stretch for the cases where
parent was 'auto', but I've decided to go back to the original code to detect
incorrect cases. Actually. there is an ASSERTION to prevent 'auto' values during
the layout.

That doesn't make sense, if childStyle->alignSelf() can be Auto here, why can't
parentStyle() alignItems() be Auto?

[jfernandez, 2014-09-03 22:27:49]

Replied.

https://codereview.chromium.org/433153003/diff/1/Source/core/css/CSSComputedS...
File Source/core/css/CSSComputedStyleDeclaration.cpp (right):

https://codereview.chromium.org/433153003/diff/1/Source/core/css/CSSComputedS...
Source/core/css/CSSComputedStyleDeclaration.cpp:1830: overflow =
parent->computedStyle()->alignItemsOverflowAlignment();
On 2014/09/03 21:45:28, esprehn wrote:
> On 2014/08/19 at 21:00:12, jfernandez wrote:
> > On 2014/08/19 00:56:33, esprehn wrote:
> > > This seems like code duplication, also same issues as below.
> > 
> > Do you mean code duplication with the logic managing the justify-self
property
> ?
> 
> Yes.

Done.

https://codereview.chromium.org/433153003/diff/1/Source/core/css/CSSComputedS...
Source/core/css/CSSComputedStyleDeclaration.cpp:2008: if (parent &&
parent->computedStyle()) {
On 2014/09/03 21:45:27, esprehn wrote:
> On 2014/08/19 at 21:00:12, jfernandez wrote:
> > On 2014/08/19 00:56:33, esprehn wrote:
> > > You need to use NodeRenderingTraversal::parent(), parentNode() is not
where
> your
> > > parent style comes from (or where you're going to be inserted into) with
> Shadow
> > > DOM or pseudo elements. You should also not call computedStyle()
repeatedly
> like
> > > this either.
> > > 
> > 
> > I used parentNode() because it was what it was used in the original code, so
> that where the confusion comes from. Also, I had the idea that this
> computedStyle thing was a DOM matter, independently of how the render tree is
> finally built. 
> 
> I don't understand what you mean, style is resolved by using the distributed
> parent per how Shadow DOM works. You can't use parentNode() to check your
parent
> style.
> 
> > 
> > Anyway, I think I've probably misunderstood the concept, so I'll apply your
> suggestions. Of course, I totally agree with avoiding repeatedly calling to
> computedStyle. 
> > 
> > > What are you trying to do here?
> > 
> > The idea is that 'auto' values on aling-self are resolved to the
aling-items's
> computed value on its parent element, as the Box Alignment Specification
> describes.
> 
> NodeRenderingTraversal::parent() is where your parent style comes from, not
from
> parentNode(). The traversal version is what deals with shadow dom
distributions.

Acknowledged.

https://codereview.chromium.org/433153003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderFlexibleBox.cpp (right):

https://codereview.chromium.org/433153003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderFlexibleBox.cpp:199: align =
parentStyle->alignItems();
On 2014/09/03 21:45:28, esprehn wrote:
> On 2014/08/19 at 21:39:18, jfernandez wrote:
> > On 2014/08/19 00:56:33, esprehn wrote:
> > > What if your parent is also auto? This doesn't seem like it can work.
> > 
> > It shouldn't be; the 'align-items' property value is resolved during the
> cascade. It was my previous patch which forced stretch for the cases where
> parent was 'auto', but I've decided to go back to the original code to detect
> incorrect cases. Actually. there is an ASSERTION to prevent 'auto' values
during
> the layout.
> 
> That doesn't make sense, if childStyle->alignSelf() can be Auto here, why
can't
> parentStyle() alignItems() be Auto?

Well, it's not that it can't, but it shouldn't. Current spec states that "auto"
value must be resolved (for all the alignment related properties) to either
'start' or 'stretch' depending on different aspects. The purpose of align-items
and justify-items is to determine the default/auto value for align-self and
justify-self respectively on the children.  Any 'auto' value on the xxx-items
properties will be resolved during the cascade. This patch precisely avoids
doing the same with the xxx-self  properties because many Renders alter the
xxx-items value during the layout phase, assuming that the corresponding
xxx-self properties will be update accordingly. 

It's possible, though, that some Render objects set "auto" to the xxx-items
property during the layout, or at any other stage after the cascade. This should
not be allowed since "auto" is not a valid value for any of those properties. We
could keep current behavior and default to 'stretch' (for flex and grid) if
'auto' is set on the xxx-items property. I thought that using the ASSERT we
could detect some incorrect logic out there.

[jfernandez, 2014-09-04 17:00:22]

Patch rebased and applied suggested changes.

Still, some of the open issues of the previous patch still apply, so please
review it carefully.

[jfernandez, 2014-09-23 13:50:06]

Gentle reminder, thanks.