[css-align] New CSS Value 'normal' for Self Alignment

[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be
used as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in
the parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. However, it's
still valid for justify-items and both of Self Alignment
properties. We will use the StyleAdjuster to resolve these 'auto'
values.

In order to deal with repaint and relayout whenever some alignment
property changes and resolve again the 'auto' values, current
implementation relies on triggering a Reattach whenever any of these
properties changes. This is a source of many problems for animations,
plugins, ... (See bug 580070 for details). The correct way of
implementing this is using "Inherit" instead so we can run the
StyleAdjuster again only for the nodes affected by the change.

This approach reduces considerably the number of style invalidations
and let us unskip the align-items repaint and relayout related
tests. These, as well as the justify-items related ones, had to be
rebaselined due the different invalidation areas generated.

Because of resolving everything in the StyleAdjuster, we don't need to
duplicate the resolution logic for generating the computed style. We
only have to deal with the 'auto' flag duality, which is not both
'auto' and 'normal' in any post-adjustement logic. This also gives us
the opportunity to get rid of the ShadowDOM and slotted elements
issues, since we don't have to deal with parent's style.

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.

BUG=565883, 474798, 448371, 582230
Committed: https://crrev.com/fe59cb90e7b08f1faa146088ddbd7589d87eebb5
Cr-Commit-Position: refs/heads/master@{#412728}

[jfernandez, 2016-02-18 11:33:40]

jfernandez@igalia.com changed reviewers:
+ cbiesinger@chromium.org, svillar@igalia.com, timloh@chromium.org

[cbiesinger, 2016-02-18 20:13:17]

lgtm for flexbox/grid

https://codereview.chromium.org/1709963002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1709963002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/TestExpectations:1042: crbug.com/587641
fast/repaint/justify-items-change.html [ Failure ]
Why are they failing and what's the plan to fix it?

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

https://codereview.chromium.org/1709963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:396: if
(parent)
I'd prefer if this function were consistent with resolveJustifySelfAuto, in that
this if checks if (!parent) instead of the other way around.

[jfernandez, 2016-02-18 21:28:58]

https://codereview.chromium.org/1709963002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/1709963002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/TestExpectations:1042: crbug.com/587641
fast/repaint/justify-items-change.html [ Failure ]
On 2016/02/18 20:13:17, cbiesinger wrote:
> Why are they failing and what's the plan to fix it?

Well, we are removing the style Reattch in this patch, as we did for align-items
in a previous patch to fix a regression in animations. I commented it in the CL
description. Because of that, we don't detect properly that we changed from/to
stretch so we would have to relayout grid items. 

I'll fix that in issue  1712613002 which is also waiting for review.

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

https://codereview.chromium.org/1709963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:396: if
(parent)
On 2016/02/18 20:13:17, cbiesinger wrote:
> I'd prefer if this function were consistent with resolveJustifySelfAuto, in
that
> this if checks if (!parent) instead of the other way around.

Acknowledged.

[Timothy Loh, 2016-02-25 00:23:17]

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

https://codereview.chromium.org/1709963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:396: if
(parent)
On 2016/02/18 21:28:57, jfernandez wrote:
> On 2016/02/18 20:13:17, cbiesinger wrote:
> > I'd prefer if this function were consistent with resolveJustifySelfAuto, in
> that
> > this if checks if (!parent) instead of the other way around.
> 
> Acknowledged.

err.. were you going to do this?

https://codereview.chromium.org/1709963002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1709963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.h:995: const ComputedStyle*
virtualEnsureComputedStyle(PseudoId pseudoElementSpecifier = NOPSEUDO) override;
Why was this change needed?

[jfernandez, 2016-02-25 01:52:37]

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

https://codereview.chromium.org/1709963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:396: if
(parent)
On 2016/02/25 00:23:17, Timothy Loh wrote:
> On 2016/02/18 21:28:57, jfernandez wrote:
> > On 2016/02/18 20:13:17, cbiesinger wrote:
> > > I'd prefer if this function were consistent with resolveJustifySelfAuto,
in
> > that
> > > this if checks if (!parent) instead of the other way around.
> > 
> > Acknowledged.
> 
> err.. were you going to do this?

I wanted to get your feedback before providing a new patch, but I'll
definitively follow christian's suggestion.

https://codereview.chromium.org/1709963002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1709963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.h:995: const ComputedStyle*
virtualEnsureComputedStyle(PseudoId pseudoElementSpecifier = NOPSEUDO) override;
On 2016/02/25 00:23:17, Timothy Loh wrote:
> Why was this change needed?

To ensure every Node accessible from ComputeStyleValueMapping has a valid
ComputedStyle pointer. Since for some properties, like 'justify-items',
'justify-self' and 'align-self', the auto value must be resolved with the
parent's computed value, we have to assume the root node has the RenderView's
ComputedStyle instance.

[jfernandez, 2016-03-01 20:36:38]

Patch Set 4 introduces some important changes in the approach. As @timloh
commented, we may not need the implementation of
Document::virtualEnsureComputedStyle if we apply a different approach for the
'legacy' keyword's behavior. 

The spec states that if the inherited value contains the 'legacy' keyword, any
'auto' justify-items values will use such inherited value. This would require to
traverse the style tree looking for such 'legacy' keyword, until we reach the
root node. We can just check for that root node and resolve as 'normal' (initial
value) as the CSS and Cascading spec states for this case, so no need to use the
RenderView' s ComputedStyle instance for that case.

Additionally, I have lots of doubts about the 'legacy' keyword itself, some of
them already expressed in the www-style mailing list. I managed to resolve the
computed value issue, but for layout is not that easy. 

The StyleAdjuster logic partially solved both computed style and layout issues,
but in order to work properly we would need a full style reattach whenever any
justify-items property changes, since we have to reapply the whole style cascade
and 'auto' value resolution. 

We removed the Reattach approach in a previous patch (see issue # 1625993004 for
details) as it was proven an incorrect solution and root cause of many and
severe bugs. 

So, I've decided to leave unimplemented the layout side of the 'legacy'
keyword's behavior, so I can clarify with the Alignment spec editors if it's
something we really want and explain the complexity of its implementation. 

I'll check out what other web engines do, just in case we can either follow a
similar approach or let them contribute to the discussion in the www-style
mailing list.

[jfernandez, 2016-03-04 22:42:50]

Description was changed from

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be used
as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in the
parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. Hence, I removed
the StyleResolver logic because is not required now; the specific
behavior of the 'normal' value will be resolved at layout time.

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.

Finally, this patch takes the opportunity to avoid the Reattach whenever
justify-items changes, since it's the root cause of many issues, like
animations, plugins, ... (See bug 580070 for details).

BUG=565883
==========

to

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be used
as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in the
parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. Hence, I removed
the StyleResolver logic because is not required now; the specific
behavior of the 'normal' value will be resolved at layout time.

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.

Finally, this patch takes the opportunity to avoid the Reattach whenever
justify-items changes, since it's the root cause of many issues, like
animations, plugins, ... (See bug 580070 for details).

BUG=565883
==========

[jfernandez, 2016-03-04 22:42:50]

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

[Timothy Loh, 2016-03-07 05:58:32]

Can we split this up into a change for adding normal/removing auto and a change
for avoiding the reattach? Aside from that, I think the changes are fine.

https://codereview.chromium.org/1709963002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/alignment/parse-align-items.html
(right):

https://codereview.chromium.org/1709963002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/alignment/parse-align-items.html:203: <!--
The 'auto' value is no valid for the align-items property. -->
err this is javascript, comments are // :-)

typo here too, no -> not

https://codereview.chromium.org/1709963002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/alignment/parse-align-items.html:204:
element.style.alignItems = "end";
better to set to "" which clears it IMO

[jfernandez, 2016-03-14 09:29:29]

https://codereview.chromium.org/1709963002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/alignment/parse-align-items.html
(right):

https://codereview.chromium.org/1709963002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/alignment/parse-align-items.html:203: <!--
The 'auto' value is no valid for the align-items property. -->
On 2016/03/07 05:58:31, Timothy Loh wrote:
> err this is javascript, comments are // :-)
> 
> typo here too, no -> not

Done.

https://codereview.chromium.org/1709963002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/alignment/parse-align-items.html:204:
element.style.alignItems = "end";
On 2016/03/07 05:58:31, Timothy Loh wrote:
> better to set to "" which clears it IMO

Done.

[jfernandez, 2016-03-14 22:41:38]

Patchset #8 (id:140001) has been deleted

[Timothy Loh, 2016-03-15 00:30:28]

Patch description needs to be updated.

Is this fixing our behaviour in shadow dom? If so, the patch description should
mention it and we should make sure there are tests (e.g. using auto on document
root / shadow roots / slotted elements / elements inside <slot>)

https://codereview.chromium.org/1709963002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/1709963002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/parser/CSSPropertyParser.cpp:3642: if
(identMatches<CSSValueAuto>(m_range.peek().id())) {
Can we just make a separate function for align-items? We're trying to avoid
having logic inside this switch statement.

[jfernandez, 2016-03-16 22:25:41]

jfernandez@igalia.com changed reviewers:
+ hayato@chromium.org

[jfernandez, 2016-03-16 22:25:41]

Adding @hayato to the loop since I'm not totally sure my patch addresses all the
ShadowDOM cases. 

In the patch for issue #1256873002 I was using ComposedTreeTraversal::parent to
get the parent's style in the DOM to 'resolve auto' values in any of its
children. This should work for any case, including the ones commented by @timloh
in his last review. 

It seems  ComposedTreeTraversal::parent does not exists now, but I'm not sure
whether FlatTreeTraversal::parent is the one I m looking for.

[kochi, 2016-03-18 05:19:02]

kochi@chromium.org changed reviewers:
+ kochi@chromium.org

[kochi, 2016-03-18 05:19:03]

https://codereview.chromium.org/1709963002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/1709963002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:382: if
(isTreeScopeRoot(parent))
As FlatTreeTraversal::parent() returns shadow host instead of shadow root
for an immediate children of shadow root.

Thus this only becomes when parent is a Document, right?

(I'm just making sure if you intend to reset this style at shadow root or not)

[hayato, 2016-03-18 07:14:07]

On 2016/03/16 at 22:25:41, jfernandez wrote:
> Adding @hayato to the loop since I'm not totally sure my patch addresses all
the ShadowDOM cases. 
> 
> In the patch for issue #1256873002 I was using ComposedTreeTraversal::parent
to get the parent's style in the DOM to 'resolve auto' values in any of its
children. This should work for any case, including the ones commented by @timloh
in his last review. 
> 

> It seems  ComposedTreeTraversal::parent does not exists now, but I'm not sure
whether FlatTreeTraversal::parent is the one I m looking for.

Yeah, FlatTreeTraversal is just the new name of ComposedTreeTraversal. You can
use FlatTreeTraveral::parent() to know the parent from where CSS inherits.

[jfernandez, 2016-03-18 13:54:41]

On 2016/03/18 07:14:07, hayato wrote:
> On 2016/03/16 at 22:25:41, jfernandez wrote:
> > Adding @hayato to the loop since I'm not totally sure my patch addresses all
> the ShadowDOM cases. 
> > 
> > In the patch for issue #1256873002 I was using ComposedTreeTraversal::parent
> to get the parent's style in the DOM to 'resolve auto' values in any of its
> children. This should work for any case, including the ones commented by
@timloh
> in his last review. 
> > 
> 
> > It seems  ComposedTreeTraversal::parent does not exists now, but I'm not
sure
> whether FlatTreeTraversal::parent is the one I m looking for.
> 
> Yeah, FlatTreeTraversal is just the new name of ComposedTreeTraversal. You can
> use FlatTreeTraveral::parent() to know the parent from where CSS inherits.

Thin I think the implementation is, at least, safe and it will resolve 'auto'
values in
a comprehensive and sensible way. I still have some doubts about its correctness
regarding the Box Alignment and ShadowDOM specs, though.

I'm preparing a new layout tests, as @timloh suggested, using 'auto' values in 
the different root elements.

[jfernandez, 2016-03-18 13:54:54]

https://codereview.chromium.org/1709963002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
(right):

https://codereview.chromium.org/1709963002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:382: if
(isTreeScopeRoot(parent))
On 2016/03/18 05:19:03, kochi wrote:
> As FlatTreeTraversal::parent() returns shadow host instead of shadow root
> for an immediate children of shadow root.
> 
> Thus this only becomes when parent is a Document, right?
> 
> (I'm just making sure if you intend to reset this style at shadow root or not)

This is an interesting question, I don't think it's clear int he spec. I have
sent an email to the blink-dev mailing list so that Tab Atkins can clarify it. 

The idea I had when implementing that was to cover the case of elements with no
parent, as the expect states. 

It seems that Document has not a style, so I considered that <html> fall into
this 'no parent" case, even that parent is not null. Some time ago @esprehn
suggested to implement the Document::virtualEnsureComputedStyle, returning the
LayoutView's ComputedStyle instance, so we can assume <html> has a valid parent
to resolve the style against. However, I think this solution is clearer, so if
parent is Document we assume we have no parent for style resolution.

In the have of ShadowDOM, I actually didn't know that ShadowRoot chilldren's
parent was the ShadowHost element instead. I'm not totally sure how to correctly
handle this ShadowDOM case. I guess we can somehow ask for the ShadowRoot' s
align-self computed style, which may have to be resolved against its parent (I
believe it doesn't exists). If we ask for the ShadowRoot children's align-self
computed style, should we use the ShadowHost's value or assume we have no
parent, as we do for the <html> node.

[jfernandez, 2016-03-23 15:05:54]

Patchset #10 (id:200001) has been deleted

[jfernandez, 2016-03-23 15:08:13]

On 2016/03/18 13:54:54, jfernandez wrote:
>
https://codereview.chromium.org/1709963002/diff/180001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp
> (right):
> 
>
https://codereview.chromium.org/1709963002/diff/180001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/css/ComputedStyleCSSValueMapping.cpp:382: if
> (isTreeScopeRoot(parent))
> On 2016/03/18 05:19:03, kochi wrote:
> > As FlatTreeTraversal::parent() returns shadow host instead of shadow root
> > for an immediate children of shadow root.
> > 
> > Thus this only becomes when parent is a Document, right?
> > 
> > (I'm just making sure if you intend to reset this style at shadow root or
not)
> 
> This is an interesting question, I don't think it's clear int he spec. I have
> sent an email to the blink-dev mailing list so that Tab Atkins can clarify it.

> 
> The idea I had when implementing that was to cover the case of elements with
no
> parent, as the expect states. 
> 
> It seems that Document has not a style, so I considered that <html> fall into
> this 'no parent" case, even that parent is not null. Some time ago @esprehn
> suggested to implement the Document::virtualEnsureComputedStyle, returning the
> LayoutView's ComputedStyle instance, so we can assume <html> has a valid
parent
> to resolve the style against. However, I think this solution is clearer, so if
> parent is Document we assume we have no parent for style resolution.
> 
> In the have of ShadowDOM, I actually didn't know that ShadowRoot chilldren's
> parent was the ShadowHost element instead. I'm not totally sure how to
correctly
> handle this ShadowDOM case. I guess we can somehow ask for the ShadowRoot' s
> align-self computed style, which may have to be resolved against its parent (I
> believe it doesn't exists). If we ask for the ShadowRoot children's align-self
> computed style, should we use the ShadowHost's value or assume we have no
> parent, as we do for the <html> node.

s/In the have of ShadowDOM/In the case of ShadowDOM

[jfernandez, 2016-03-23 15:22:37]

I've added a new layout tests to check out how the root elements, both in DOM
and SadowDOM worlds, behave when resolving the 'auto' values.

I hope the new test cases help to figure out what I really need to do with
ShadowDOM regarding computed style and 'auto' values resolution. 

There is an ongoing thread in the mailing list about the general issue, but
let's focus here on the specific issue of resolving computed style values, which
is what the patch is just addressing.

The first thing to discuss is why I'm getting different results in the case of
what I called, hopefully correctly, slotted elements for the justify-self
resolution (see comments related to the StyleAdjuster). 

The thing is that for the justify-items we are still using the StyleAdjuster for
resolvignt he 'auto' values. This is kind of temporary, because it requires to
Reattach the hole style tree whenever a justify-items property changes it value
(I'll talk more about this in the mailing list). Anyway, this is probably going
to change soon, but I wonder why I'm getting different values than the ones got
for align-items, which is just using the FlatTreeTraversal::parent api to access
the style to inherit from.

Related to the issue commented above, it seems that the slotted element has no
parent when the shadowTree is attached and before the slot property is actually
set. I'm not sure whether that's correct or not, and it it is, whether the auto
value resolution is what we would expect.

Finally, the last test cases, which are commented for the time being, violates
the assertion ASSERTION FAILED: node.canParticipateInFlatTree(). It seems that
the 'slot' element can be used in the FlatTreeraversal::parent function.

[esprehn, 2016-03-23 17:49:59]

I've been thinking about this recently and I think we might want to take a
slightly different approach.

The values the child needs to resolve the auto values should be propagated in
internal fields of ComputedStyle. This is what we do for visibility too
(inherited_flags._visibility). StyleAdjuster will just copy the values from the
parent style into the child style as needed.

When one of these values changes on the parent it should result in an Inherit
change. That'll recompute the style of the direct children, but if we do it
right shouldn't need to recompute the entire subtree.

Now the position getters on ComputedStyle can just consult the parent values
stored inside the style to know how to resolve the auto value.

[esprehn, 2016-03-23 17:53:58]

rune@ what do you think?

[rune, 2016-03-23 22:10:34]

On 2016/03/23 17:53:58, esprehn wrote:
> rune@ what do you think?

sgtm

I'm on a train, not too awake, but IIUC, it should only propagate one level down
to be used for resolving, so we could put it into inherited_flags to trigger
Inherit to the child, but we still need some special casing to avoid further
inheritance?

If I understood esprehn's idea, we can use the Inherit mechanism without
propagating more than one level down which includes two extra values in computed
style:

given the following flags:

non_inherited.parentAlignItems
inherited_flags.inheritedAlignItems

When you set align-items on the parent, also set inheritedAlignItems. If
inheritedAlignItems change, it'll trigger Inherit.

For the child, after inheritance set parentAlignItems (if not initial), and
reset inheritedAlignItems to avoid further Inherit propagation unless this
element also sets a different inheritedAlignItems based on its own align-items
property.

[rune, 2016-03-24 09:49:45]

On 2016/03/23 22:10:34, rune (Easter - slow) wrote:
> On 2016/03/23 17:53:58, esprehn wrote:
> > rune@ what do you think?
> 
> sgtm

... as long as it doesn't cause a lot of copy-on-write for what would otherwise
have been inherited data that could've been shared down the tree.

[jfernandez, 2016-03-28 11:16:22]

Thanks @esprhen and @rune for the feedback. I like the idea of using
inheritance to resolve the auto values. Actually, that was my first idea 
when I started to implement this, long time ago, but I wasn't able to do
it. Now I know more about Blink internals so I can try it again. 

In my opinion, the StyleAdjuster + inheritance is the more natural way 
to approach this issue. Specially, for the case of the "legacy" keyword
allowed only in the 'justify-iitems' property, which will propagate down
in the whole tree for resolving the 'auto' values until reaching a 
descendant with a non-auto value. 

The 'auto' issue in the Box Alignment spec, which comes from the 
Flexbox spec, has been debated a lot in the www-style mailing list. 
It's very complex and confusing, hence it was modified recently to 
simplify most of the cases. Content Alignment properties 
(align-content, justify-content) are now using 'normal' as default 
(auto is no valid anymore), which will be "resolved" at layout time. 
The 'aign-items" property has been simplified as well, so it does not 
allow 'auto' anymore and it defaults to 'normal', to be resolved at 
layout time as well. 

So, only the cases of the Self Alignment (justify-self, align-self) 
remain affected by the 'auto' value resolution issue (besides the 
special 'legacy' keyword case of the justify-items). These cases, as 
@rune pointed out, requires only to 1 level of inheritance. 

At this point, we have to consider 2 different scenarios where we
have to deal with 'auto' values: computed style vs layout. 

The ideal solution would be to implement a logic which could 
resolve both, and the StyleAdjuster + inheritance seems to be the 
approach to follow. It seems it deals better with the ShadowDOM 
issues, since we don't have to speculate with the parent we have to
use for the resolution. However, as @rune pointed out before, it may 
imply a lot of copy-on-write operations. As a matter of fact, this was 
the reason why the StyleAdjuster approach was rejected by the 
WebKit reviewers (see https://webkit.org/b/133359#c82).

In webkit we ended up implementing a simpler, but less elegant, 
approach of resolving the auto values duplicating code, both in 
Computed Style declaration and at layout time. This implies that 
we will have to do it for any specific layout model (grid, flex). We 
will have to do it, eventually, for regular layout objects as well.  
Since changing the alignment values doest not need to perform a 
new layout, it seems this approach is more efficient, but I admit 
the code is way uglier and harder to maintain. It's also true that 
WebKit does not have to deal with ShadowDOM, which adds some
complexity here.

So, sorry for this long analysis, but I wanted to be sure we all are 
on the same page. I'll proceed with the StyleAdjuster + inheritance 
(1 level) approach and let's see how it solves the problem and its 
impact on performance.

[jfernandez, 2016-03-28 11:50:22]

Oh, for got to mention, not sure if you all ware aware that this "inheritance"
would be kind of special because it will affect different CSS properties. Hence,
the value of 'align-items" on the parent will be propagated to the children's
'align-self" property (if current value is 'auto', of course).

[jfernandez, 2016-03-28 12:29:54]

Another issue to address with the StyleAdjuster + inheritance is that changing
the value of a box to 'auto' should also trigger the style adjustment.

[Timothy Loh, 2016-03-29 04:40:02]

Using StyleAdjuster sgtm. I don't think we need special values on the inherited
data structures though. I think it's sufficient to just use a single member for
align-self and just update that with the computed value of align-items from the
parent if it is normal. To avoid needing to copy the RareNonInheritedData, we
could repurporse the 'auto' flag to not just mean 'auto' prior to running the
StyleAdjuster but also mean 'normal' after running it (so if the parent's
align-items is the initial value we don't have to set anything). If this works,
I think it's less copying than rune@'s approach and probably not more
complicated.

On 2016/03/28 12:29:54, jfernandez wrote:
> Another issue to address with the StyleAdjuster + inheritance is that changing
> the value of a box to 'auto' should also trigger the style adjustment.

The StyleAdjuster is always run after we build up a ComputedStyle so there
should be no problem here?

[jfernandez, 2016-03-29 08:04:05]

On 2016/03/29 04:40:02, Timothy Loh wrote:
> Using StyleAdjuster sgtm. I don't think we need special values on the
inherited
> data structures though. I think it's sufficient to just use a single member
for
> align-self and just update that with the computed value of align-items from
the
> parent if it is normal. To avoid needing to copy the RareNonInheritedData, we
> could repurporse the 'auto' flag to not just mean 'auto' prior to running the
> StyleAdjuster but also mean 'normal' after running it (so if the parent's
> align-items is the initial value we don't have to set anything). If this
works,
> I think it's less copying than rune@'s approach and probably not more
> complicated.
> 
> On 2016/03/28 12:29:54, jfernandez wrote:
> > Another issue to address with the StyleAdjuster + inheritance is that
changing
> > the value of a box to 'auto' should also trigger the style adjustment.
> 
> The StyleAdjuster is always run after we build up a ComputedStyle so there
> should be no problem here?

The StyleAdjuster is not run after parent's align-items changes its value, or
children's align-self changes to/from 'auto'. That's why I had to implement
the Reattach logic, so the styles tree is built again whenever one of those
properties changes.

[Timothy Loh, 2016-03-30 00:32:19]

On 2016/03/29 08:04:05, jfernandez wrote:
> The StyleAdjuster is not run after parent's align-items changes its value, or
> children's align-self changes to/from 'auto'. That's why I had to implement
> the Reattach logic, so the styles tree is built again whenever one of those
> properties changes.

I'm not sure I understand here. If you change any CSS property for a given
element then we'll recalc styles for that element and run the StyleAdjuster. If
we see align-items has changed in stylePropagationDiff, we should return Inherit
and this will make us recalc the children nodes (and thus run the StyleAdjuster
on those).

[jfernandez, 2016-03-30 07:55:26]

On 2016/03/30 00:32:19, Timothy Loh wrote:
> On 2016/03/29 08:04:05, jfernandez wrote:
> > The StyleAdjuster is not run after parent's align-items changes its value,
or
> > children's align-self changes to/from 'auto'. That's why I had to implement
> > the Reattach logic, so the styles tree is built again whenever one of those
> > properties changes.
> 
> I'm not sure I understand here. If you change any CSS property for a given
> element then we'll recalc styles for that element and run the StyleAdjuster.
If
> we see align-items has changed in stylePropagationDiff, we should return
Inherit
> and this will make us recalc the children nodes (and thus run the
StyleAdjuster
> on those).

Well, I guess that depends on what we implement for the stylePropagationDiff.
When I implemented this logic the first time, looking at the
stylePropagationDiff
function made me think that Reattach was the right approach for
a change on align-items and justify-items property (specially for the later,
since
it might imply rebuilding several levels of the Style tree). The "Inherit"
result was
not obvious since those properties are not inheritable, as the spec states.

Even using that approach, we still have to deal with several corner cases, like
style changes at layout time (LayoutFullScreen so far, but there were many
others
at that time). Also, the spec at that time required to resolve auto values
depending
on the kind of element (Flexbox, grid, regular box, absolute positioned, ...)
Hence
the StyleAdjuster had to deal with that considering only the "display" property
to figure that out. There are objects that use the LayoutFlexBox class but they
don't have such 'display' value (now the spec got simplified, thanks god, and
we don't have to deal with this).

So, even using the StyleAdjuster and forcing a Reattach whenever the 
properties changed, we still needed to resolved the 'auto' values both, in
ComputedStyleCSSValueMapping and during layout, to be sure we don't
have unresolved 'auto' values. Quite a mess, indeed. That's why in WebKit's
implementation we decided to forget about the StyleAdjuster and just 
resolved the auto values as needed, either computed style or layout.

As I commented above,  I don't like WebKit's approach, for many reasons, but
I admit that it works, it's simple and avoids the copy-on-write issues. 

Additionally, even though that we assumed that Reattach could cause issues
with the plugins and some performance impact, we detected important
regressions with animations. It seems it breaks the animation flow when
the justify-items value changes. So we had to revert that approach. We
still have to do it for the justify-items property (there is a TODO for that
in the code), but since the property is only used for grid layout, not that
urgent for now (See issue #580070 for additional info). 

So, now you suggest to use Inherit, which I guess will trigger a similar
style recalc process, perhaps less aggressive than Rattach. I can give it
a try if you think it would not cause similar issues with animations.

[jfernandez, 2016-03-30 09:07:04]

Just realized that the approach suggested by @esprehn and @rune implies that
stylePropagationDiff will returning Inherit anyway, so perhaps we can just do it
for any rareNonInherited->m_alignItems change (instead of the old Reattach) and
just use the StyleAdjuster logic to resovle again the auto values. 

The approach of adding extra fields in the rareInherited structure would allow
use to resolve 'auto' values just using internal properties of ComputedStyle, so
we can rely on that for both, ComputedStyleCSSMapping and layout logic.

I' m still working on that approach, because I have some problems keeping
the new extra fields updated.

[jfernandez, 2016-03-30 14:35:30]

I'm having problems implementing @rune's idea. I've found out that StyleResolver
manages a styles cache to be applied on elements with the same CSS rules. 

cachedMatchedProperties && MatchedPropertiesCache::isCacheable()

This is causing that children in different blocks to share the parent's
align-items value, even being in different subtrees. This is because both
children has the same CSS rules to be applied, so we copy the new property
m_parentAlignItems, which contains the align-items value of a different parent. 

I guess this can be solved by adding a new condition that makes the CSS rule as
non- cachable. I'm still working on that, but perhaps someone has a better idea
or perhaps we want to abandon this approach.

[jfernandez, 2016-03-30 21:40:25]

I finally managed to do a preliminary implementation of the approach proposed by
@esprehn and @rune . It still needs some work and clean up, but it's a good
enough to evaluate it and continue discussing about the best solution. 

As a commented before, I had to add a new condition to avoid caching some
styles. Maybe we can improve it so we don't miss some caching opportunities, but
let's see.

The 'legacy' keyword, which propagates the "justify-items" value down in the
tree until we find a non-auto justify-items value, is still managed by the
StyleAdjuster. I guess we can use the last proposal by @timloh, which I start to
implement ASAP.

Another pending issue would be to remove the ComputedSyle::resolvedXXX functions
and just relay on the regular getter methods, which should return the resolved
value. We may find cases failing because the style is changed at layout time,
but I'll have to check it out. 

Anyway, those functions are also used to "resolve" the "normal' value, which is
different depending on the layout model. We could perhaps move that logic to the
Layout class.

[jfernandez, 2016-04-01 13:46:50]

On 2016/03/30 00:32:19, Timothy Loh wrote:
> On 2016/03/29 08:04:05, jfernandez wrote:
> > The StyleAdjuster is not run after parent's align-items changes its value,
or
> > children's align-self changes to/from 'auto'. That's why I had to implement
> > the Reattach logic, so the styles tree is built again whenever one of those
> > properties changes.
> 
> I'm not sure I understand here. If you change any CSS property for a given
> element then we'll recalc styles for that element and run the StyleAdjuster.
If
> we see align-items has changed in stylePropagationDiff, we should return
Inherit
> and this will make us recalc the children nodes (and thus run the
StyleAdjuster
> on those).

You are right; returning Inherit in stylePropagationDiff whenever align-items
and/or justify-items values change is enough to trigger style recalc and exec
the StyleAdjusment logic again. I confirmed as well that it does no cause the
same regression on animations than Reattach, as expected. I can even provide
a test for that. 

The thing now is to select the best approach for this issue. I've sent an email
to the blink-dev mailing list with some information I hope could be useful to
make a decision.

[jfernandez, 2016-05-20 13:14:09]

PatchSet 13 and 14 are the final agreed solution after discussing it with
@timloh . Perhaps I added excessive comments for the duality of the 'auto' flag,
so I'm open to changes.

[jfernandez, 2016-05-20 16:56:09]

Description was changed from

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be used
as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in the
parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. Hence, I removed
the StyleResolver logic because is not required now; the specific
behavior of the 'normal' value will be resolved at layout time.

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.

Finally, this patch takes the opportunity to avoid the Reattach whenever
justify-items changes, since it's the root cause of many issues, like
animations, plugins, ... (See bug 580070 for details).

BUG=565883
==========

to

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be used
as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in the
parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. However, it's
still valid for justify-items and both of Self Alignment properties. We
will use the StyleAdjuster to resolve these 'auto' values.

In order to deal with repaint and relayout whenever some alignment
property changes and resolve again the 'auto' values, current 
implementation relies on triggering a Reattach whenever any of
these properties changes. This is a source of many problems for
animations, plugins, ... (See bug 580070 for details). The correct
way of implementing this is using "Inherit" instead so we can
run the StyleAdjuster again only for the nodes affected by the
change. 

This approach reduces considerably the number of style
invalidations and let us unskip the align-items repaint and
relayout related tests. These, as well the justify-items related
ones, have to be rebaselined due the different invalidation areas
generated.

Because of resolving everything in the StyleAdjuster, we don't
need to duplicate the resolution logic for generating the
computed style. We only have to deal with the 'auto' flag
duality, which is not both 'auto' and 'normal' in any
post-adjustement logic. This also gives us the opportunity
to get rid of the ShadowDOM and slotted elements issues, 
since we don't have to deal with parent's style. 

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.


BUG=565883, 474798
==========

[jfernandez, 2016-05-20 17:03:00]

Description was changed from

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be used
as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in the
parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. However, it's
still valid for justify-items and both of Self Alignment properties. We
will use the StyleAdjuster to resolve these 'auto' values.

In order to deal with repaint and relayout whenever some alignment
property changes and resolve again the 'auto' values, current 
implementation relies on triggering a Reattach whenever any of
these properties changes. This is a source of many problems for
animations, plugins, ... (See bug 580070 for details). The correct
way of implementing this is using "Inherit" instead so we can
run the StyleAdjuster again only for the nodes affected by the
change. 

This approach reduces considerably the number of style
invalidations and let us unskip the align-items repaint and
relayout related tests. These, as well the justify-items related
ones, have to be rebaselined due the different invalidation areas
generated.

Because of resolving everything in the StyleAdjuster, we don't
need to duplicate the resolution logic for generating the
computed style. We only have to deal with the 'auto' flag
duality, which is not both 'auto' and 'normal' in any
post-adjustement logic. This also gives us the opportunity
to get rid of the ShadowDOM and slotted elements issues, 
since we don't have to deal with parent's style. 

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.


BUG=565883, 474798
==========

to

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be
used as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in
the parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. However, it's
still valid for justify-items and both of Self Alignment
properties. We will use the StyleAdjuster to resolve these 'auto'
values.

In order to deal with repaint and relayout whenever some alignment
property changes and resolve again the 'auto' values, current
implementation relies on triggering a Reattach whenever any of these
properties changes. This is a source of many problems for animations,
plugins, ... (See bug 580070 for details). The correct way of
implementing this is using "Inherit" instead so we can run the
StyleAdjuster again only for the nodes affected by the change.

This approach reduces considerably the number of style invalidations
and let us unskip the align-items repaint and relayout related
tests. These, as well the justify-items related ones, have to be
rebaselined due the different invalidation areas generated.

Because of resolving everything in the StyleAdjuster, we don't need to
duplicate the resolution logic for generating the computed style. We
only have to deal with the 'auto' flag duality, which is not both
'auto' and 'normal' in any post-adjustement logic. This also gives us
the opportunity to get rid of the ShadowDOM and slotted elements
issues, since we don't have to deal with parent's style.

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.

BUG=565883, 474798
==========

[jfernandez, 2016-05-20 17:03:42]

Description was changed from

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be
used as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in
the parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. However, it's
still valid for justify-items and both of Self Alignment
properties. We will use the StyleAdjuster to resolve these 'auto'
values.

In order to deal with repaint and relayout whenever some alignment
property changes and resolve again the 'auto' values, current
implementation relies on triggering a Reattach whenever any of these
properties changes. This is a source of many problems for animations,
plugins, ... (See bug 580070 for details). The correct way of
implementing this is using "Inherit" instead so we can run the
StyleAdjuster again only for the nodes affected by the change.

This approach reduces considerably the number of style invalidations
and let us unskip the align-items repaint and relayout related
tests. These, as well the justify-items related ones, have to be
rebaselined due the different invalidation areas generated.

Because of resolving everything in the StyleAdjuster, we don't need to
duplicate the resolution logic for generating the computed style. We
only have to deal with the 'auto' flag duality, which is not both
'auto' and 'normal' in any post-adjustement logic. This also gives us
the opportunity to get rid of the ShadowDOM and slotted elements
issues, since we don't have to deal with parent's style.

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.

BUG=565883, 474798
==========

to

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be
used as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in
the parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. However, it's
still valid for justify-items and both of Self Alignment
properties. We will use the StyleAdjuster to resolve these 'auto'
values.

In order to deal with repaint and relayout whenever some alignment
property changes and resolve again the 'auto' values, current
implementation relies on triggering a Reattach whenever any of these
properties changes. This is a source of many problems for animations,
plugins, ... (See bug 580070 for details). The correct way of
implementing this is using "Inherit" instead so we can run the
StyleAdjuster again only for the nodes affected by the change.

This approach reduces considerably the number of style invalidations
and let us unskip the align-items repaint and relayout related
tests. These, as well as the justify-items related ones, had to be
rebaselined due the different invalidation areas generated.

Because of resolving everything in the StyleAdjuster, we don't need to
duplicate the resolution logic for generating the computed style. We
only have to deal with the 'auto' flag duality, which is not both
'auto' and 'normal' in any post-adjustement logic. This also gives us
the opportunity to get rid of the ShadowDOM and slotted elements
issues, since we don't have to deal with parent's style.

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.

BUG=565883, 474798
==========

[Timothy Loh, 2016-05-23 05:45:40]

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/alignment/ensure-flexbox-compatibility-with-initial-values-expected.txt
(right):

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/alignment/ensure-flexbox-compatibility-with-initial-values-expected.txt:6:
PASS CSS.supports('align-items', 'normal') is true
Is this test still being useful? At least from the output it now looks like it
just tests the same things repeatedly :/

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:277: // To avoid
needing to copy the RareNonInheritedData, we could repurporse the 'auto'
The word 'could' here suggests we don't actually do this, I'd remove it from all
these comments.

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:223: if
(alignSelfPosition() == ItemPositionAuto)
I think this branch is no longer needed (or correct), and this function doesn't
need to take the parent style any more.

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:238: if
(justifySelfPosition() == ItemPositionAuto)
this one too

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyleConstants.h (right):

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyleConstants.h:549:
ItemPositionAuto,
This would be a good place for a comment :)

[jfernandez, 2016-05-25 18:26:49]

Submitted a new patch with the changes suggested in the last review.

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/alignment/ensure-flexbox-compatibility-with-initial-values-expected.txt
(right):

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/alignment/ensure-flexbox-compatibility-with-initial-values-expected.txt:6:
PASS CSS.supports('align-items', 'normal') is true
On 2016/05/23 05:45:39, Timothy Loh wrote:
> Is this test still being useful? At least from the output it now looks like it
> just tests the same things repeatedly :/

Probably not the best tests, indeed; and the expectations file is even worst. 
However, the tests is still needed. Actually, it spotted a bug in this patch.
I'll
try to explain the purpose of this test so perhaps we may decide to keep it
and necessarily improve it.

The tests has the goal of detecting regressions like the one described in bug
#447848 where the initial values of some alignment properties are invalid
when grid layout is not enabled. The reason is that the new parsing logic is
used only in that case.

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:277: // To avoid
needing to copy the RareNonInheritedData, we could repurporse the 'auto'
On 2016/05/23 05:45:39, Timothy Loh wrote:
> The word 'could' here suggests we don't actually do this, I'd remove it from
all
> these comments.

Done.

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.cpp (right):

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:223: if
(alignSelfPosition() == ItemPositionAuto)
On 2016/05/23 05:45:39, Timothy Loh wrote:
> I think this branch is no longer needed (or correct), and this function
doesn't
> need to take the parent style any more.

I think you're right. However, we'd may need this for the cases where the style
is modified during the layout. Perhaps we should deal with those cases in a
different way, so I'll follow your suggestion.

https://codereview.chromium.org/1709963002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.cpp:238: if
(justifySelfPosition() == ItemPositionAuto)
On 2016/05/23 05:45:39, Timothy Loh wrote:
> this one too

Done.

[jfernandez, 2016-05-25 22:00:53]

Patchset #18 (id:380001) has been deleted

[jfernandez, 2016-05-25 22:01:09]

Patchset #17 (id:360001) has been deleted

[jfernandez, 2016-05-25 23:19:16]

PatchSet 17 causes 2 layout tests to fail, precisely some FullScreen cases. the
FullScreen layout is using the FlexBox object, hence it needs the alignment
values to be resolved. The problem is that the FullScreen logic applies style
changes at layout time, which don't trigger the StyleAdjuster run to resolve
again the auto values after the style change.

[Timothy Loh, 2016-05-26 04:26:36]

On 2016/05/25 23:19:16, jfernandez wrote:
> PatchSet 17 causes 2 layout tests to fail, precisely some FullScreen cases.
the
> FullScreen layout is using the FlexBox object, hence it needs the alignment
> values to be resolved. The problem is that the FullScreen logic applies style
> changes at layout time, which don't trigger the StyleAdjuster run to resolve
> again the auto values after the style change.

I assume this is because of the initial auto values? Maybe for now we could move
the StyleAdjuster logic into something like
ComputedStyle::adjustAlignmentProperties and call it in the appropriate places.

It seems odd that we're creating ComputedStyles and not passing them through the
StyleAdjuster, although I don't really know what these particular ComputedStyle
objects are for. Maybe rune@ or esprehn@ have more thoughts.

[jfernandez, 2016-05-26 08:32:04]

On 2016/05/26 04:26:36, Timothy Loh wrote:
> On 2016/05/25 23:19:16, jfernandez wrote:
> > PatchSet 17 causes 2 layout tests to fail, precisely some FullScreen cases.
> the
> > FullScreen layout is using the FlexBox object, hence it needs the alignment
> > values to be resolved. The problem is that the FullScreen logic applies
style
> > changes at layout time, which don't trigger the StyleAdjuster run to resolve
> > again the auto values after the style change.
> 
> I assume this is because of the initial auto values? Maybe for now we could
move
> the StyleAdjuster logic into something like
> ComputedStyle::adjustAlignmentProperties and call it in the appropriate
places.
> 

That approach is equivalent to the one already implemented, resolving the 'auto'
values on demand. The problem with that approach, as described in the design
document, is that we would need to deal with objects hierarchy in different
context (layout, computed style) and we will have to take into account ShadowDOM
and slotted elements.  

The StyleAdjuster approach is the cleaner and simplest one, and I think that
after solving the copy-on-write approach, quite efficient. 

> It seems odd that we're creating ComputedStyles and not passing them through
the
> StyleAdjuster, although I don't really know what these particular
ComputedStyle
> objects are for. Maybe rune@ or esprehn@ have more thoughts.

Yeah, this ComputedStyle creation and tunning at layout time is really odd. It's
not
usual either, so perhaps we can try to solve each case independently. In the
case of
the FullScreen layout object, I think the purpose of the style manipulation is
to
ensure it behaves like a custom flexbox, setting the style on the parent to be
propagated to any item inside the fullscreen scene. 

I wonder whether we could do that through CSS, but if we can't, perhaps we can
implement something specific to propagate the parent's style to its children, on
demand.

[Timothy Loh, 2016-05-26 08:50:48]

On 2016/05/26 08:32:04, jfernandez wrote:
> On 2016/05/26 04:26:36, Timothy Loh wrote:
> > On 2016/05/25 23:19:16, jfernandez wrote:
> > > PatchSet 17 causes 2 layout tests to fail, precisely some FullScreen
cases.
> > the
> > > FullScreen layout is using the FlexBox object, hence it needs the
alignment
> > > values to be resolved. The problem is that the FullScreen logic applies
> style
> > > changes at layout time, which don't trigger the StyleAdjuster run to
resolve
> > > again the auto values after the style change.
> > 
> > I assume this is because of the initial auto values? Maybe for now we could
> move
> > the StyleAdjuster logic into something like
> > ComputedStyle::adjustAlignmentProperties and call it in the appropriate
> places.
> > 
> 
> That approach is equivalent to the one already implemented, resolving the
'auto'
> values on demand. The problem with that approach, as described in the design
> document, is that we would need to deal with objects hierarchy in different
> context (layout, computed style) and we will have to take into account
ShadowDOM
> and slotted elements.  

I meant that we could just call this in the various LayoutObject subclasses
which create ComputedStyles (or we could even try to just call
adjustComputedStyle for all of these, I wouldn't be surprised if there were
other issues due to not calling it)

[jfernandez, 2016-05-26 17:21:29]

On 2016/05/26 08:50:48, Timothy Loh wrote:
> On 2016/05/26 08:32:04, jfernandez wrote:
> > On 2016/05/26 04:26:36, Timothy Loh wrote:
> > > On 2016/05/25 23:19:16, jfernandez wrote:
> > > > PatchSet 17 causes 2 layout tests to fail, precisely some FullScreen
> cases.
> > > the
> > > > FullScreen layout is using the FlexBox object, hence it needs the
> alignment
> > > > values to be resolved. The problem is that the FullScreen logic applies
> > style
> > > > changes at layout time, which don't trigger the StyleAdjuster run to
> resolve
> > > > again the auto values after the style change.
> > > 
> > > I assume this is because of the initial auto values? Maybe for now we
could
> > move
> > > the StyleAdjuster logic into something like
> > > ComputedStyle::adjustAlignmentProperties and call it in the appropriate
> > places.
> > > 
> > 
> > That approach is equivalent to the one already implemented, resolving the
> 'auto'
> > values on demand. The problem with that approach, as described in the design
> > document, is that we would need to deal with objects hierarchy in different
> > context (layout, computed style) and we will have to take into account
> ShadowDOM
> > and slotted elements.  
> 
> I meant that we could just call this in the various LayoutObject subclasses
> which create ComputedStyles (or we could even try to just call
> adjustComputedStyle for all of these, I wouldn't be surprised if there were
> other issues due to not calling it)

The problem is, at least for this specific case, that we would need to call 
the StyleAdjuster for the FulScreen layout object's children. Actually, I 
could verify that it's called, indeed. The problem is that the LayoutFullScreen
logic is changing the LayoutTree structure, hence when running the
StyleAdjuster on children it will use the wrong parent. 

It seems that getting back the ComputedStyle::resolvedXXX branches you
suggested to remove will solve this specific case, since we will resolve the
Self Alignment properties with the initial 'auto' (or normal) during the
layout; hence we will use the new LayoutTree structure.

However, I wonder whether this approach may solve other cases, like the
ones where children's initial 'auto' values are resolved by the StyleAdjuster
because their parent in the DOM has a specific CSS rule setting a value
for the Default Alignment properties (align-items, justify-items). How the
"new parent" will propagate to the children if they don't have 'auto' values
anymore after the StyleAdjuster ? I'll try to investigate this.

[jfernandez, 2016-05-26 19:21:40]

On 2016/05/26 17:21:29, jfernandez wrote:
> However, I wonder whether this approach may solve other cases, like the
> ones where children's initial 'auto' values are resolved by the StyleAdjuster
> because their parent in the DOM has a specific CSS rule setting a value
> for the Default Alignment properties (align-items, justify-items). How the
> "new parent" will propagate to the children if they don't have 'auto' values
> anymore after the StyleAdjuster ? I'll try to investigate this.

As I suspected, resolving 'auto' values during layout is not enough if the
StyleAdjuster already resolved it using the old hierarchy. The new 
tree structure created by the LayoutFullScreen logic wont be used to
resolve children's Self Alignment 'auto' values because they are not
'auto' anymore.

I'm not sure if we can just call to the StyleAdjuster logic directly
using the new hierarchy, even copying it to ComputedStyle, because
the actual issue is that StyleAdjuster will resolve the 'auto' values
using the DOM tree while the LayoutTree is going to be modified
afterwards.

[Timothy Loh, 2016-05-30 06:05:02]

foolip@: Do you know about LayoutFullScreen modifications to the layout tree or
who might know about it? The short story is that we have some values on
ComputedStyle which need to go through the StyleAdjuster and I think either we
aren't calling it or the layout tree modifications means we're using the wrong
parent values?

[foolip, 2016-05-30 06:39:17]

On 2016/05/30 06:05:02, Timothy Loh wrote:
> foolip@: Do you know about LayoutFullScreen modifications to the layout tree
or
> who might know about it? The short story is that we have some values on
> ComputedStyle which need to go through the StyleAdjuster and I think either we
> aren't calling it or the layout tree modifications means we're using the wrong
> parent values?

I know *of* it and that it's a problem, and my current task is to reland
https://codereview.chromium.org/1410833004 and make sure it sticks, which will
remove that code. However, I haven't really dug deep into how it currently
works. jchaffraix@ or dsinclair@ who worked on this previously might have
learned a thing or two when ripping the code out, though.

[jfernandez, 2016-05-30 07:50:40]

On 2016/05/30 06:39:17, Philip Jägenstedt wrote:
> On 2016/05/30 06:05:02, Timothy Loh wrote:
> > foolip@: Do you know about LayoutFullScreen modifications to the layout tree
> or
> > who might know about it? The short story is that we have some values on
> > ComputedStyle which need to go through the StyleAdjuster and I think either
we
> > aren't calling it or the layout tree modifications means we're using the
wrong
> > parent values?
> 
> I know *of* it and that it's a problem, and my current task is to reland
> https://codereview.chromium.org/1410833004 and make sure it sticks, which will
> remove that code. However, I haven't really dug deep into how it currently
> works. jchaffraix@ or dsinclair@ who worked on this previously might have
> learned a thing or two when ripping the code out, though.

I guess we have two options to go ahead with the StyleAduster approach for
'auto' values resolution of Box Alignment properties: 

 - wait for the FullScreeen refactoring
 - implement some hack in the FullScreen logic to solve the test cases
currently failing, even though we know there are other cases which may
fail as well.

I think we can go with the second option, considering that after discussing it
deeply we are pretty sure the StyleAdjuster approach is the right one. We solved
all the known issues but the ones related to the LayoutTree changes, which is
something we must avoid, as far as I know. 

So, I can comment on the hacks I eventually do on the FullScreen logic so we
can prepare regressions tests for them, once the refactoring is completed.

[rune, 2016-05-30 07:54:36]

On 2016/05/30 06:05:02, Timothy Loh wrote:
> foolip@: Do you know about LayoutFullScreen modifications to the layout tree
or
> who might know about it? The short story is that we have some values on
> ComputedStyle which need to go through the StyleAdjuster and I think either we
> aren't calling it or the layout tree modifications means we're using the wrong
> parent values?

Parent values affecting computed style works on the flattened dom tree, not the
layout tree. If the StyleAdjuster is called with the parentStyle being the
computed style of the parent box, not element, it could go bananas for anonymous
table boxes, for instance. If the computed value of the justify-self property is
considering parent box computed style, it's pretty special in this regard.

Using flex in fullscreen.css seems like an implementation detail in Blink due to
the fact that we don't implement top layer?

[rune, 2016-05-30 07:55:09]

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

[rune, 2016-05-30 07:55:10]

https://codereview.chromium.org/1709963002/diff/400001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1709963002/diff/400001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:289: // any legacy
keywords), or 'normal' if the box has no parent.
I haven't looked at the details here, but I noticed this part reading the spec,
and might be what's causing problems in fullscreen.

Does the first "parent" mean parent element, or parent box? Normally, computed
style works on the flattened dom tree, not the layout tree. The "if the box has
no parent" indicates this works on the layout tree.

[jfernandez, 2016-05-30 10:12:09]

On 2016/05/30 07:54:36, rune wrote:
> On 2016/05/30 06:05:02, Timothy Loh wrote:
> > foolip@: Do you know about LayoutFullScreen modifications to the layout tree
> or
> > who might know about it? The short story is that we have some values on
> > ComputedStyle which need to go through the StyleAdjuster and I think either
we
> > aren't calling it or the layout tree modifications means we're using the
wrong
> > parent values?
> 
> Parent values affecting computed style works on the flattened dom tree, not
the
> layout tree. If the StyleAdjuster is called with the parentStyle being the
> computed style of the parent box, not element, it could go bananas for
anonymous
> table boxes, for instance. If the computed value of the justify-self property
is
> considering parent box computed style, it's pretty special in this regard.
> 

align-items property. Just that simple. The computed style logic was implemented
in a similar way (assuming DOM parent was indeed the Flex container).

Then, other specs wanted to use the alignment css properties, so the CSS Box
Alignment spec was defined to generalize this alignment properties to be applied
to any Box model. It states the following to resolve the 'auto' values:

https://drafts.csswg.org/css-align/#propdef-align-self
"The auto keyword computes to the computed value of align-items on the parent
or normal if the box has no parent."

I guess the "no parent" concept is intended for resolving 'auto' values of CSS
rules
applied to the 'html' element. I could ask for clarification on this regard to
the spec
editors, which cases had in mind for the "no parent".

The question of which parentStyle should be used during StyleAdjuster logic 
is perhaps the most important one. I've always thought it should be the the one
based on the DOM's hierarchy, but I agree it may lead to issues with Anonymous
Boxes.  

> Using flex in fullscreen.css seems like an implementation detail in Blink due
to
> the fact that we don't implement top layer?

That's my understanding.

[rune, 2016-05-30 12:02:10]

On 2016/05/30 10:12:09, jfernandez wrote:

> align-items property. Just that simple. The computed style logic was
implemented
> in a similar way (assuming DOM parent was indeed the Flex container).
> 
> Then, other specs wanted to use the alignment css properties, so the CSS Box
> Alignment spec was defined to generalize this alignment properties to be
applied
> to any Box model. It states the following to resolve the 'auto' values:
> 
> https://drafts.csswg.org/css-align/#propdef-align-self
> "The auto keyword computes to the computed value of align-items on the parent
> or normal if the box has no parent."
> 
> I guess the "no parent" concept is intended for resolving 'auto' values of CSS
> rules
> applied to the 'html' element. I could ask for clarification on this regard to
> the spec
> editors, which cases had in mind for the "no parent".

I was more interested in whether the parent is parent element or parent box.

> The question of which parentStyle should be used during StyleAdjuster logic 
> is perhaps the most important one. I've always thought it should be the the
one
> based on the DOM's hierarchy, but I agree it may lead to issues with Anonymous
> Boxes.  

StyleAdjuster expects the inheritance parent (flattened dom parent). If "parent"
means "parent element", having the code in StyleAdjuster would be fine. If it
means "parent box", StyleAdjuster is the wrong place for it.

[jfernandez, 2016-06-01 08:32:19]

On 2016/05/30 12:02:10, rune wrote:
> On 2016/05/30 10:12:09, jfernandez wrote:
> 
> > align-items property. Just that simple. The computed style logic was
> implemented
> > in a similar way (assuming DOM parent was indeed the Flex container).
> > 
> > Then, other specs wanted to use the alignment css properties, so the CSS Box
> > Alignment spec was defined to generalize this alignment properties to be
> applied
> > to any Box model. It states the following to resolve the 'auto' values:
> > 
> > https://drafts.csswg.org/css-align/#propdef-align-self
> > "The auto keyword computes to the computed value of align-items on the
parent
> > or normal if the box has no parent."
> > 
> > I guess the "no parent" concept is intended for resolving 'auto' values of
CSS
> > rules
> > applied to the 'html' element. I could ask for clarification on this regard
to
> > the spec
> > editors, which cases had in mind for the "no parent".
> 
> I was more interested in whether the parent is parent element or parent box.
> 
> > The question of which parentStyle should be used during StyleAdjuster logic 
> > is perhaps the most important one. I've always thought it should be the the
> one
> > based on the DOM's hierarchy, but I agree it may lead to issues with
Anonymous
> > Boxes.  
> 
> StyleAdjuster expects the inheritance parent (flattened dom parent). If
"parent"
> means "parent element", having the code in StyleAdjuster would be fine. If it
> means "parent box", StyleAdjuster is the wrong place for it.

I've asked to the spec editors and this is the answer:

https://lists.w3.org/Archives/Public/www-style/2016May/0259.html

I think the StyleAdjuster is the correct place to do it, so we will have to
find a solution for Anonymous objects and elements changing the
layout tree.

[Timothy Loh, 2016-06-02 06:14:59]

On 2016/05/30 07:50:40, jfernandez wrote:
> On 2016/05/30 06:39:17, Philip Jägenstedt wrote:
> > On 2016/05/30 06:05:02, Timothy Loh wrote:
> > > foolip@: Do you know about LayoutFullScreen modifications to the layout
tree
> > or
> > > who might know about it? The short story is that we have some values on
> > > ComputedStyle which need to go through the StyleAdjuster and I think
either
> we
> > > aren't calling it or the layout tree modifications means we're using the
> wrong
> > > parent values?
> > 
> > I know *of* it and that it's a problem, and my current task is to reland
> > https://codereview.chromium.org/1410833004 and make sure it sticks, which
will
> > remove that code. However, I haven't really dug deep into how it currently
> > works. jchaffraix@ or dsinclair@ who worked on this previously might have
> > learned a thing or two when ripping the code out, though.
> 
> I guess we have two options to go ahead with the StyleAduster approach for
> 'auto' values resolution of Box Alignment properties: 
> 
>  - wait for the FullScreeen refactoring
>  - implement some hack in the FullScreen logic to solve the test cases
> currently failing, even though we know there are other cases which may
> fail as well.
> 
> I think we can go with the second option, considering that after discussing it
> deeply we are pretty sure the StyleAdjuster approach is the right one. We
solved
> all the known issues but the ones related to the LayoutTree changes, which is
> something we must avoid, as far as I know. 
> 
> So, I can comment on the hacks I eventually do on the FullScreen logic so we
> can prepare regressions tests for them, once the refactoring is completed.

As long as the hacks aren't too bad, sure. I don't think your previous patch set
where resolveAlignSelf looked at the parent was correct though, since we
re-purpose the 'auto' enum for post-adjustment ComputedStyles.

[jfernandez, 2016-06-03 10:49:40]

On 2016/06/02 06:14:59, Timothy Loh wrote:
> On 2016/05/30 07:50:40, jfernandez wrote:
> > On 2016/05/30 06:39:17, Philip Jägenstedt wrote:
> > > On 2016/05/30 06:05:02, Timothy Loh wrote:
> > > > foolip@: Do you know about LayoutFullScreen modifications to the layout
> tree
> > > or
> > > > who might know about it? The short story is that we have some values on
> > > > ComputedStyle which need to go through the StyleAdjuster and I think
> either
> > we
> > > > aren't calling it or the layout tree modifications means we're using the
> > wrong
> > > > parent values?
> > > 
> > > I know *of* it and that it's a problem, and my current task is to reland
> > > https://codereview.chromium.org/1410833004 and make sure it sticks, which
> will
> > > remove that code. However, I haven't really dug deep into how it currently
> > > works. jchaffraix@ or dsinclair@ who worked on this previously might have
> > > learned a thing or two when ripping the code out, though.
> > 
> > I guess we have two options to go ahead with the StyleAduster approach for
> > 'auto' values resolution of Box Alignment properties: 
> > 
> >  - wait for the FullScreeen refactoring
> >  - implement some hack in the FullScreen logic to solve the test cases
> > currently failing, even though we know there are other cases which may
> > fail as well.
> > 
> > I think we can go with the second option, considering that after discussing
it
> > deeply we are pretty sure the StyleAdjuster approach is the right one. We
> solved
> > all the known issues but the ones related to the LayoutTree changes, which
is
> > something we must avoid, as far as I know. 
> > 
> > So, I can comment on the hacks I eventually do on the FullScreen logic so we
> > can prepare regressions tests for them, once the refactoring is completed.
> 
> As long as the hacks aren't too bad, sure. I don't think your previous patch
set
> where resolveAlignSelf looked at the parent was correct though, since we
> re-purpose the 'auto' enum for post-adjustment ComputedStyles.

As far as  I understand that "re-purpose", we wanted to avoid resolving
children's auto value with parent default values (normal), so we assume
that any auto value in post-adjustment ComputedStyle should be treated
as 'normal'. 

My hack idea would be that, instead of blindly follow such assumption, we
re-check whether parent's (in the LayoutTree) Default Alignment value is 
'normal' or not.  This would solve the specific case of the Full Screen tests
currently failing because of the new anonymous element added to the Layout
Tree, which expects its Default Value to be propagated to the re-parented 
children.  I admit that this approach would not work if, for instance, the
old parent had a non-initial Default Alignment value, since the children's 
auto values would have been resolved during the style adjustment logic.

I'll try to provide a better patch, though, now that at least you agree on
hacking the code to satisfy the requirements of the FullScreen current 
design.

[rune, 2016-06-03 11:28:28]

On 2016/06/03 10:49:40, jfernandez wrote:
> On 2016/06/02 06:14:59, Timothy Loh wrote:
> > On 2016/05/30 07:50:40, jfernandez wrote:
> > > On 2016/05/30 06:39:17, Philip Jägenstedt wrote:
> > > > On 2016/05/30 06:05:02, Timothy Loh wrote:
> > > > > foolip@: Do you know about LayoutFullScreen modifications to the
layout
> > tree
> > > > or
> > > > > who might know about it? The short story is that we have some values
on
> > > > > ComputedStyle which need to go through the StyleAdjuster and I think
> > either
> > > we
> > > > > aren't calling it or the layout tree modifications means we're using
the
> > > wrong
> > > > > parent values?
> > > > 
> > > > I know *of* it and that it's a problem, and my current task is to reland
> > > > https://codereview.chromium.org/1410833004 and make sure it sticks,
which
> > will
> > > > remove that code. However, I haven't really dug deep into how it
currently
> > > > works. jchaffraix@ or dsinclair@ who worked on this previously might
have
> > > > learned a thing or two when ripping the code out, though.
> > > 
> > > I guess we have two options to go ahead with the StyleAduster approach for
> > > 'auto' values resolution of Box Alignment properties: 
> > > 
> > >  - wait for the FullScreeen refactoring
> > >  - implement some hack in the FullScreen logic to solve the test cases
> > > currently failing, even though we know there are other cases which may
> > > fail as well.
> > > 
> > > I think we can go with the second option, considering that after
discussing
> it
> > > deeply we are pretty sure the StyleAdjuster approach is the right one. We
> > solved
> > > all the known issues but the ones related to the LayoutTree changes, which
> is
> > > something we must avoid, as far as I know. 
> > > 
> > > So, I can comment on the hacks I eventually do on the FullScreen logic so
we
> > > can prepare regressions tests for them, once the refactoring is completed.
> > 
> > As long as the hacks aren't too bad, sure. I don't think your previous patch
> set
> > where resolveAlignSelf looked at the parent was correct though, since we
> > re-purpose the 'auto' enum for post-adjustment ComputedStyles.
> 
> As far as  I understand that "re-purpose", we wanted to avoid resolving
> children's auto value with parent default values (normal), so we assume
> that any auto value in post-adjustment ComputedStyle should be treated
> as 'normal'. 
> 
> My hack idea would be that, instead of blindly follow such assumption, we
> re-check whether parent's (in the LayoutTree) Default Alignment value is 
> 'normal' or not.  This would solve the specific case of the Full Screen tests
> currently failing because of the new anonymous element added to the Layout
> Tree, which expects its Default Value to be propagated to the re-parented 
> children.  I admit that this approach would not work if, for instance, the
> old parent had a non-initial Default Alignment value, since the children's 
> auto values would have been resolved during the style adjustment logic.
> 
> I'll try to provide a better patch, though, now that at least you agree on
> hacking the code to satisfy the requirements of the FullScreen current 
> design.

I haven't studied the Fullscreen implementation in detail, but you should make
it work properly on "normal" anonymous boxes. If I understand the spec correctly
after Tab's comments, then for this example:

<div style="align-items: end">
  <div style="display: table-cell"></div>
</div>

align-self will be "end" for both the table-cell (due to parent being inherit
parent in [1]) and the anonymous display:table wrapper (because of the note
about anonymous boxes in [2]).

You know better than me the practical consequences of that for any given layout
model for the outer div.

[1] https://drafts.csswg.org/css-align/#align-self-property
[2] https://drafts.csswg.org/css-align/#align-items-property

[jfernandez, 2016-06-03 11:41:06]

On 2016/06/03 11:28:28, rune wrote:
> On 2016/06/03 10:49:40, jfernandez wrote:
> > On 2016/06/02 06:14:59, Timothy Loh wrote:
> > > On 2016/05/30 07:50:40, jfernandez wrote:
> > > > On 2016/05/30 06:39:17, Philip Jägenstedt wrote:
> > > > > On 2016/05/30 06:05:02, Timothy Loh wrote:
> > > > > > foolip@: Do you know about LayoutFullScreen modifications to the
> layout
> > > tree
> > > > > or
> > > > > > who might know about it? The short story is that we have some values
> on
> > > > > > ComputedStyle which need to go through the StyleAdjuster and I think
> > > either
> > > > we
> > > > > > aren't calling it or the layout tree modifications means we're using
> the
> > > > wrong
> > > > > > parent values?
> > > > > 
> > > > > I know *of* it and that it's a problem, and my current task is to
reland
> > > > > https://codereview.chromium.org/1410833004 and make sure it sticks,
> which
> > > will
> > > > > remove that code. However, I haven't really dug deep into how it
> currently
> > > > > works. jchaffraix@ or dsinclair@ who worked on this previously might
> have
> > > > > learned a thing or two when ripping the code out, though.
> > > > 
> > > > I guess we have two options to go ahead with the StyleAduster approach
for
> > > > 'auto' values resolution of Box Alignment properties: 
> > > > 
> > > >  - wait for the FullScreeen refactoring
> > > >  - implement some hack in the FullScreen logic to solve the test cases
> > > > currently failing, even though we know there are other cases which may
> > > > fail as well.
> > > > 
> > > > I think we can go with the second option, considering that after
> discussing
> > it
> > > > deeply we are pretty sure the StyleAdjuster approach is the right one.
We
> > > solved
> > > > all the known issues but the ones related to the LayoutTree changes,
which
> > is
> > > > something we must avoid, as far as I know. 
> > > > 
> > > > So, I can comment on the hacks I eventually do on the FullScreen logic
so
> we
> > > > can prepare regressions tests for them, once the refactoring is
completed.
> > > 
> > > As long as the hacks aren't too bad, sure. I don't think your previous
patch
> > set
> > > where resolveAlignSelf looked at the parent was correct though, since we
> > > re-purpose the 'auto' enum for post-adjustment ComputedStyles.
> > 
> > As far as  I understand that "re-purpose", we wanted to avoid resolving
> > children's auto value with parent default values (normal), so we assume
> > that any auto value in post-adjustment ComputedStyle should be treated
> > as 'normal'. 
> > 
> > My hack idea would be that, instead of blindly follow such assumption, we
> > re-check whether parent's (in the LayoutTree) Default Alignment value is 
> > 'normal' or not.  This would solve the specific case of the Full Screen
tests
> > currently failing because of the new anonymous element added to the Layout
> > Tree, which expects its Default Value to be propagated to the re-parented 
> > children.  I admit that this approach would not work if, for instance, the
> > old parent had a non-initial Default Alignment value, since the children's 
> > auto values would have been resolved during the style adjustment logic.
> > 
> > I'll try to provide a better patch, though, now that at least you agree on
> > hacking the code to satisfy the requirements of the FullScreen current 
> > design.
> 
> I haven't studied the Fullscreen implementation in detail, but you should make
> it work properly on "normal" anonymous boxes. If I understand the spec
correctly
> after Tab's comments, then for this example:
> 
> <div style="align-items: end">
>   <div style="display: table-cell"></div>
> </div>
> 
> align-self will be "end" for both the table-cell (due to parent being inherit
> parent in [1]) and the anonymous display:table wrapper (because of the note
> about anonymous boxes in [2]).

yeah, I agree the spec describes that case pretty clear. However, the full
screen
one is quite different, since the Anonymous box generated is a new parent box, 
which expects its align-items value to be propagated to its children, instead of

the one specified for the previous parent by the CSS style declaration.   

> 
> You know better than me the practical consequences of that for any given
layout
> model for the outer div.
> 

Yeah, I know. It's expectable that for some Layout models that generate a Layout
Tree that differ considerably from the DOM tree this "inheritance" will be a
challenge.

> [1] https://drafts.csswg.org/css-align/#align-self-property
> [2] https://drafts.csswg.org/css-align/#align-items-property

[jfernandez, 2016-06-04 11:40:47]

PatchSet 18 contain the hack, at least from point if view it indeed is, that 
seems to fix the FullScreen issues with layout Tree changes and Anonymous
boxes. The hack does not solve other Anonymous related issues, as the one
pointed out by @rune, since StyleAdjuster will not know about them. The 
only way to deal with Anonymous boxes is to implement 'auto' value 
resolution at layout time. Since CSS Box Alignment is only implemented, for
now, for Flexbox and Grid, I consider that these Anonymous issues have to
be addressed by each layout model when the spec is actually implemented.

PatchSet 19 solves the issues regarding align-items repaint tests. It was just
a matter of update these tests to the new ref test model, which was applied
to the rest of the repaint tests. As it states the issue's description, this
patch
solves, collaterally, most of the repainting and invalidation issues described
in bug #474798 thanks to avoid the Reattach when the Default Alignment
properties change. Since we know return Inherit, only the boxes affected by
the style change are invalidated. 

However, the bug is not totally fixed because the tests verifying that there
are no invalidations when there is no change in the painting area still fail
for grid. Those tests are still marked as fail in the TestExpectations file.

[jfernandez, 2016-06-13 09:39:25]

@rune @timloh any additional feedback on this ?

[rune, 2016-06-13 12:24:20]

https://codereview.chromium.org/1709963002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1709963002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:249:
adjustStyleForAlignment(style, parentStyle);
I still have a hard time understanding the spec here. The spec says align-items
applies to block level elements, yet the computed value is still the specified
value, and align-items does not affect the element itself. align-self refers to
the computed value for the parent align-items, which means the align-self:auto
can resolve its auto-value from an inline parent. I'm not sure if that is what
the spec was meant to say, but that means this code is wrong if you have:

<span style="align-items: center">
  <span style="align-self: auto"></span>
</span>

and the outer span for some reason is wrapped into an anonymous block box.

Another thing is that anonymous boxes are often created after you calculate the
style for its children. In the case of tables, you calculate the style of a
direct child of a table, if it's not some table-* display type you need to wrap
it in an anonymous table cell, anonymous row, etc. But, you don't know if you
have to do that before you have calculated and adjusted the style for that
element.

[jfernandez, 2016-06-13 13:00:36]

https://codereview.chromium.org/1709963002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp (right):

https://codereview.chromium.org/1709963002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/resolver/StyleAdjuster.cpp:249:
adjustStyleForAlignment(style, parentStyle);
On 2016/06/13 12:24:19, rune wrote:
> I still have a hard time understanding the spec here. The spec says
align-items
> applies to block level elements, yet the computed value is still the specified
> value, and align-items does not affect the element itself. align-self refers
to
> the computed value for the parent align-items, which means the align-self:auto
> can resolve its auto-value from an inline parent.

That's how I understand the spec, yes. An element's align-items property is
intended to
be used its children's align-self property to resolve any 'auto' value. I guess
this applies
as well to inline hierarchies, indeed. 

> I'm not sure if that is what
> the spec was meant to say, but that means this code is wrong if you have:
> 
> <span style="align-items: center">
>   <span style="align-self: auto"></span>
> </span>
> 
> and the outer span for some reason is wrapped into an anonymous block box.
>

After Tab's reply to my question (posted in an earlier comment in this review)
"Parent
element" refers to the element tree; it doesn't care about box-tree structure."
So, inner span's 'auto' value will be resolved from its original parent element.
The code above is intended to be triggered DURING layout, AFTER the style was
calculated. The idea
is that wrapping layout objects into Anonymous boxes will imply a style recalc
on 
those objects, so only in that case the new parent will be used. Clearly the
spec does
not mention that case, but we indeed want that behavior eg. FullScreen). 

The problem I see, with my current knowledge, with the Full Screen case is that
we
want a new parent to propagate its align-items value to its new children. We
could
resolve the opposite case (new children. anonymous or not, resolving its value
to
its new parent) but not a re-parenting case. The reason is that getter methods
for the
ComputedStyle are supposed to be const, as Layout is not the place to do this
kind
of style calculations. We could copy the styles of all the children, modifying
this new
ComputedStyle instance and assign it back again, but I guess it's a brute force
approach
with many problems.   

> Another thing is that anonymous boxes are often created after you calculate
the
> style for its children. In the case of tables, you calculate the style of a
> direct child of a table, if it's not some table-* display type you need to
wrap
> it in an anonymous table cell, anonymous row, etc. But, you don't know if you
> have to do that before you have calculated and adjusted the style for that
> element.

So far, Alignment is only implemented in Flexible Box and Grid layout models.
Even that
parsing and computed style resolution should be the common logic, I guess
specific
layout models will have to implement it at some point and dealing with its
particularities.

[jfernandez, 2016-06-17 07:36:50]

Patchset #21 (id:480001) has been deleted

[jfernandez, 2016-06-21 13:50:33]

Hi @rune and @timloh Could you please take a look at the last patch ? it solves
the FullScreen issues without having to touch the StyleAdjuster logic, which is
supposedly fulfilling the Box Alignment spec.

Regarding Anonymous boxes, the spec only mentions Anonymous box resolving Self
Alignment values when they are 'auto'. This only can be achieved during layout,
when we have a complete LayoutTree structure. The last patch addresses also this
issue. 

However, what was doing the FullScreen logic, pretending an Anonymous box to be
used for resolving its children Self Alignment 'auto' values goes against the
spec. I know the whole FullScreen logic is going to be implemented, but any
solution would have to deal with centering in a different way.

[rune, 2016-06-23 09:48:38]

On 2016/06/21 13:50:33, jfernandez wrote:
> Hi @rune and @timloh Could you please take a look at the last patch ?

looking ...

[rune, 2016-06-23 11:54:28]

On 2016/06/23 09:48:38, rune wrote:
> On 2016/06/21 13:50:33, jfernandez wrote:
> > Hi @rune and @timloh Could you please take a look at the last patch ?
> 
> looking ...

core/css and core/style lgtm

timloh@ might want to take a look.

[jfernandez, 2016-06-28 08:40:54]

Hi @timloh, even though @rune already approved the change, could you please take
a final look, please ?

[Timothy Loh, 2016-06-28 08:44:24]

On 2016/06/28 08:40:54, jfernandez wrote:
> Hi @timloh, even though @rune already approved the change, could you please
take
> a final look, please ?

sorry for the delay, will look first thing tomorrow morning.

[Timothy Loh, 2016-06-29 02:00:32]

lgtm (I didn't look closely at the core/layout code)

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h:4341:
m_value.valueID = CSSValueAuto;
Not sure if this case is still reached, if not, add a NOT_REACHED()

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1443: // its
childnre, expecting to propagate its align-items property's value to the Flex
items.
typo childnre

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:373: StyleSelfAlignmentData
resolvedAlignSelf(ItemPosition normalValueBehaviour, const ComputedStyle*
parentStyle = nullptr) const;
Can you add a TODO here to remove the parentStyle argument when it's no longer
needed?

[jfernandez, 2016-06-29 19:55:47]

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h:4341:
m_value.valueID = CSSValueAuto;
On 2016/06/29 02:00:32, Timothy Loh wrote:
> Not sure if this case is still reached, if not, add a NOT_REACHED()

Since we left unresolved the initial value of the Self Alignment properties
(justify-self, align-self) I can't ensure ItemPositionAuto is not reached.

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1443: // its
childnre, expecting to propagate its align-items property's value to the Flex
items.
On 2016/06/29 02:00:32, Timothy Loh wrote:
> typo childnre

Actually, this TODO is not needed anymore, since we resolved the FullScreeen
case by using "center" as "normal" behavior.

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/style/ComputedStyle.h:373: StyleSelfAlignmentData
resolvedAlignSelf(ItemPosition normalValueBehaviour, const ComputedStyle*
parentStyle = nullptr) const;
On 2016/06/29 02:00:32, Timothy Loh wrote:
> Can you add a TODO here to remove the parentStyle argument when it's no longer
> needed?

We can't, actually. the only way to deal with Anonymous boxes is to resolve the
'auto' values during the layout. The parent relationship is only known
inspecting the Layout Tree, hence we need to pass the specific parent style.

[jfernandez, 2016-06-29 21:01:23]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2016-06-29 21:01:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, rune@opera.com, timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1709963002/#ps560001
(title: "Patch for landing.")

[commit-bot: I haz the power, 2016-06-29 21:02:37]

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, 2016-06-29 21:08:56]

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

[commit-bot: I haz the power, 2016-06-29 21:08:59]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[jfernandez, 2016-06-29 21:14:02]

The CQ bit was checked by jfernandez@igalia.com

[commit-bot: I haz the power, 2016-06-29 21:17:19]

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, 2016-06-29 21:24:27]

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

[commit-bot: I haz the power, 2016-06-29 21:24:30]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Timothy Loh, 2016-06-30 01:50:05]

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h:4341:
m_value.valueID = CSSValueAuto;
On 2016/06/29 19:55:47, jfernandez wrote:
> On 2016/06/29 02:00:32, Timothy Loh wrote:
> > Not sure if this case is still reached, if not, add a NOT_REACHED()
> 
> Since we left unresolved the initial value of the Self Alignment properties
> (justify-self, align-self) I can't ensure ItemPositionAuto is not reached. 

Isn't this only used by ComputedStyleCSSValueMapping though, where you're
explicitly handling this?

[jfernandez, 2016-06-30 10:17:53]

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h (right):

https://codereview.chromium.org/1709963002/diff/540001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/css/CSSPrimitiveValueMappings.h:4341:
m_value.valueID = CSSValueAuto;
On 2016/06/30 01:50:05, Timothy Loh wrote:
> On 2016/06/29 19:55:47, jfernandez wrote:
> > On 2016/06/29 02:00:32, Timothy Loh wrote:
> > > Not sure if this case is still reached, if not, add a NOT_REACHED()
> > 
> > Since we left unresolved the initial value of the Self Alignment properties
> > (justify-self, align-self) I can't ensure ItemPositionAuto is not reached. 
> 
> Isn't this only used by ComputedStyleCSSValueMapping though, where you're
> explicitly handling this?

Yes, you are right. It's only used by the computed style mapping logic, so I
guess
we can add the assert as you suggested.

[jfernandez, 2016-06-30 10:19:49]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2016-06-30 10:19:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, rune@opera.com, timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1709963002/#ps580001
(title: "Applied suggested changed and using testharness instead of js-test.")

[commit-bot: I haz the power, 2016-06-30 10:20:05]

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, 2016-06-30 11:13:04]

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

[commit-bot: I haz the power, 2016-06-30 11:13:07]

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

[jfernandez, 2016-06-30 12:51:27]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2016-06-30 12:51:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, rune@opera.com, timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1709963002/#ps600001
(title: "Fixed layout tests.")

[commit-bot: I haz the power, 2016-06-30 12:51:43]

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, 2016-06-30 13:38:58]

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

[commit-bot: I haz the power, 2016-06-30 13:39:01]

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_...)

[jfernandez, 2016-06-30 22:22:34]

Patchset #26 (id:600001) has been deleted

[jfernandez, 2016-07-11 15:28:52]

Patchset #27 (id:640001) has been deleted

[jfernandez, 2016-07-11 19:04:46]

Patchset #27 (id:660001) has been deleted

[jfernandez, 2016-07-11 21:44:20]

Patchset #27 (id:680001) has been deleted

[jfernandez, 2016-07-12 09:15:05]

Hi @cbiesinger , could you help me to understand why the StyleAdjuster approach
we finally agreed on implementing causes a few forms/numbers tests to fail just
in the mac bot ? 

Summarizing, this patch just relies on the StyleAdjuster to resolve the 'auto'
values on the CSS Alignment properties. We still check for the parent's
align-items during layout but only in the case of Anonymous boxes, which can't
be resolved during the StyleAdjuster logic.

It seems that the TextInputFieldType class creates a container that is based on
FlexBox, implemented by the TextControlInnerContainer class.  Apparently, this
container is represented in the DOM tree as well, so it should be handled by the
StyleAdjuster. 

The last patchset removes the restriction of being an anonymous child for
checking our the parent's align-items during layout for auto values resolution.
That changes made the test to pass again, but I don't fully understand why and,
even more strange, why the tests only fail in the mac bots.

[cbiesinger, 2016-07-13 21:13:23]

On 2016/07/12 09:15:05, jfernandez wrote:
> Hi @cbiesinger , could you help me to understand why the StyleAdjuster
approach
> we finally agreed on implementing causes a few forms/numbers tests to fail
just
> in the mac bot ? 
> 
> Summarizing, this patch just relies on the StyleAdjuster to resolve the 'auto'
> values on the CSS Alignment properties. We still check for the parent's
> align-items during layout but only in the case of Anonymous boxes, which can't
> be resolved during the StyleAdjuster logic.
> 
> It seems that the TextInputFieldType class creates a container that is based
on
> FlexBox, implemented by the TextControlInnerContainer class.  Apparently, this
> container is represented in the DOM tree as well, so it should be handled by
the
> StyleAdjuster. 
> 
> The last patchset removes the restriction of being an anonymous child for
> checking our the parent's align-items during layout for auto values
resolution.
> That changes made the test to pass again, but I don't fully understand why
and,
> even more strange, why the tests only fail in the mac bots.

For my reference, the test results in question are
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...

That is weird -- are you sure that wasn't an unrelated flakiness? I've read
through this code a bit now, and I can't immediately find out why this would
happen.

[jfernandez, 2016-07-13 21:27:13]

On 2016/07/13 21:13:23, cbiesinger wrote:
> On 2016/07/12 09:15:05, jfernandez wrote:
> > Hi @cbiesinger , could you help me to understand why the StyleAdjuster
> approach
> > we finally agreed on implementing causes a few forms/numbers tests to fail
> just
> > in the mac bot ? 
> > 
> > Summarizing, this patch just relies on the StyleAdjuster to resolve the
'auto'
> > values on the CSS Alignment properties. We still check for the parent's
> > align-items during layout but only in the case of Anonymous boxes, which
can't
> > be resolved during the StyleAdjuster logic.
> > 
> > It seems that the TextInputFieldType class creates a container that is based
> on
> > FlexBox, implemented by the TextControlInnerContainer class.  Apparently,
this
> > container is represented in the DOM tree as well, so it should be handled by
> the
> > StyleAdjuster. 
> > 
> > The last patchset removes the restriction of being an anonymous child for
> > checking our the parent's align-items during layout for auto values
> resolution.
> > That changes made the test to pass again, but I don't fully understand why
> and,
> > even more strange, why the tests only fail in the mac bots.
> 
> For my reference, the test results in question are
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
> 

Even more specific, these 3 ones:

https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...

> That is weird -- are you sure that wasn't an unrelated flakiness? I've read
> through this code a bit now, and I can't immediately find out why this would
> happen.

Well, the root cause of the failures seems to be the change that avoid checking
out the parent's align-items value in the LayoutFelxibleBox::alignmentForChild
function. The last PatchSet revert just that change and it seems the mentioned
tests pass now. 

As I commented before the <input> element seems to use a Flexbox as container.
However, it creates the proper DOM element as TextControlInnerContainer, which
is then rendered by LayoutTextControlInnerContainer (the flexbox). So, as I
understand how CSS styles are resolved, it should be passed through the
StyleAdjuster logic, hence Flexibox's items shouldn't need to check out their
parent, as any 'auto' value should have been resolved already.

[cbiesinger, 2016-07-13 21:33:49]

On 2016/07/13 21:27:13, jfernandez wrote:
> On 2016/07/13 21:13:23, cbiesinger wrote:
> > On 2016/07/12 09:15:05, jfernandez wrote:
> > > Hi @cbiesinger , could you help me to understand why the StyleAdjuster
> > approach
> > > we finally agreed on implementing causes a few forms/numbers tests to fail
> > just
> > > in the mac bot ? 
> > > 
> > > Summarizing, this patch just relies on the StyleAdjuster to resolve the
> 'auto'
> > > values on the CSS Alignment properties. We still check for the parent's
> > > align-items during layout but only in the case of Anonymous boxes, which
> can't
> > > be resolved during the StyleAdjuster logic.
> > > 
> > > It seems that the TextInputFieldType class creates a container that is
based
> > on
> > > FlexBox, implemented by the TextControlInnerContainer class.  Apparently,
> this
> > > container is represented in the DOM tree as well, so it should be handled
by
> > the
> > > StyleAdjuster. 
> > > 
> > > The last patchset removes the restriction of being an anonymous child for
> > > checking our the parent's align-items during layout for auto values
> > resolution.
> > > That changes made the test to pass again, but I don't fully understand why
> > and,
> > > even more strange, why the tests only fail in the mac bots.
> > 
> > For my reference, the test results in question are
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
> > 
> 
> Even more specific, these 3 ones:
> 
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...
> 
> > That is weird -- are you sure that wasn't an unrelated flakiness? I've read
> > through this code a bit now, and I can't immediately find out why this would
> > happen.
> 
> Well, the root cause of the failures seems to be the change that avoid
checking
> out the parent's align-items value in the LayoutFelxibleBox::alignmentForChild
> function. The last PatchSet revert just that change and it seems the mentioned
> tests pass now. 
> 
> As I commented before the <input> element seems to use a Flexbox as container.
> However, it creates the proper DOM element as TextControlInnerContainer, which
> is then rendered by LayoutTextControlInnerContainer (the flexbox). So, as I
> understand how CSS styles are resolved, it should be passed through the
> StyleAdjuster logic, hence Flexibox's items shouldn't need to check out their
> parent, as any 'auto' value should have been resolved already.

Yes, I understood that :)

Hmm... actually, one theory: LayoutTextControl.cpp does this:
        innerEditorLayoutObject->setStyle(createInnerEditorStyle(styleRef()));

That may well not go through the adjustStyle logic. Maybe that code should call
adjustStyle on the returned ComputedStyle object?

[jfernandez, 2016-07-13 21:40:58]

On 2016/07/13 21:33:49, cbiesinger wrote:
> On 2016/07/13 21:27:13, jfernandez wrote:
> > On 2016/07/13 21:13:23, cbiesinger wrote:
> > > On 2016/07/12 09:15:05, jfernandez wrote:
> > > > Hi @cbiesinger , could you help me to understand why the StyleAdjuster
> > > approach
> > > > we finally agreed on implementing causes a few forms/numbers tests to
fail
> > > just
> > > > in the mac bot ? 
> > > > 
> > > > Summarizing, this patch just relies on the StyleAdjuster to resolve the
> > 'auto'
> > > > values on the CSS Alignment properties. We still check for the parent's
> > > > align-items during layout but only in the case of Anonymous boxes, which
> > can't
> > > > be resolved during the StyleAdjuster logic.
> > > > 
> > > > It seems that the TextInputFieldType class creates a container that is
> based
> > > on
> > > > FlexBox, implemented by the TextControlInnerContainer class. 
Apparently,
> > this
> > > > container is represented in the DOM tree as well, so it should be
handled
> by
> > > the
> > > > StyleAdjuster. 
> > > > 
> > > > The last patchset removes the restriction of being an anonymous child
for
> > > > checking our the parent's align-items during layout for auto values
> > > resolution.
> > > > That changes made the test to pass again, but I don't fully understand
why
> > > and,
> > > > even more strange, why the tests only fail in the mac bots.
> > > 
> > > For my reference, the test results in question are
> > >
> >
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel...
> > > 
> > 
> > Even more specific, these 3 ones:
> > 
> >
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...
> > 
> > > That is weird -- are you sure that wasn't an unrelated flakiness? I've
read
> > > through this code a bit now, and I can't immediately find out why this
would
> > > happen.
> > 
> > Well, the root cause of the failures seems to be the change that avoid
> checking
> > out the parent's align-items value in the
LayoutFelxibleBox::alignmentForChild
> > function. The last PatchSet revert just that change and it seems the
mentioned
> > tests pass now. 
> > 
> > As I commented before the <input> element seems to use a Flexbox as
container.
> > However, it creates the proper DOM element as TextControlInnerContainer,
which
> > is then rendered by LayoutTextControlInnerContainer (the flexbox). So, as I
> > understand how CSS styles are resolved, it should be passed through the
> > StyleAdjuster logic, hence Flexibox's items shouldn't need to check out
their
> > parent, as any 'auto' value should have been resolved already.
> 
> Yes, I understood that :)
> 
> Hmm... actually, one theory: LayoutTextControl.cpp does this:
>         innerEditorLayoutObject->setStyle(createInnerEditorStyle(styleRef()));
> 

Umm, good point. 

> That may well not go through the adjustStyle logic. Maybe that code should
call
> adjustStyle on the returned ComputedStyle object?

It seems a good idea to explore. Thanks, let' see where I go with this.

[jfernandez, 2016-07-20 13:37:45]

I think I finally managed to solve the issues with the InputFieldType elements
and shadowDOM; see PatchSet 30 and 31 focusing on the changes applied to the
TextControllerInnerElements class. 

It seems that when an element has custom styles, it does not run the
StyleAdjuster::adjustComputedStyle logic. For certain input types we are calling
directly to adjustStyleForEditing, but we must run the adjustStyleForAlignment
as well.

The mentioned patches address just the issues identified by the mac bot failures
on a few forms/number layout tests. I guess there might be other issues whenever
we use the Flexbox display and we expect an align-items value to be propagated
to the align-self 'auto' values in its children. 

Even though @timloh has approved already the patch, it might be a good
idea that either @hayato or @kochi have a look at it.

Finally, I haven't managed to figure out why the mentioned forms/number
tests failed only in the mac bot.

[jfernandez, 2016-08-17 19:54:18]

Description was changed from

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be
used as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in
the parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. However, it's
still valid for justify-items and both of Self Alignment
properties. We will use the StyleAdjuster to resolve these 'auto'
values.

In order to deal with repaint and relayout whenever some alignment
property changes and resolve again the 'auto' values, current
implementation relies on triggering a Reattach whenever any of these
properties changes. This is a source of many problems for animations,
plugins, ... (See bug 580070 for details). The correct way of
implementing this is using "Inherit" instead so we can run the
StyleAdjuster again only for the nodes affected by the change.

This approach reduces considerably the number of style invalidations
and let us unskip the align-items repaint and relayout related
tests. These, as well as the justify-items related ones, had to be
rebaselined due the different invalidation areas generated.

Because of resolving everything in the StyleAdjuster, we don't need to
duplicate the resolution logic for generating the computed style. We
only have to deal with the 'auto' flag duality, which is not both
'auto' and 'normal' in any post-adjustement logic. This also gives us
the opportunity to get rid of the ShadowDOM and slotted elements
issues, since we don't have to deal with parent's style.

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.

BUG=565883, 474798
==========

to

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be
used as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in
the parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. However, it's
still valid for justify-items and both of Self Alignment
properties. We will use the StyleAdjuster to resolve these 'auto'
values.

In order to deal with repaint and relayout whenever some alignment
property changes and resolve again the 'auto' values, current
implementation relies on triggering a Reattach whenever any of these
properties changes. This is a source of many problems for animations,
plugins, ... (See bug 580070 for details). The correct way of
implementing this is using "Inherit" instead so we can run the
StyleAdjuster again only for the nodes affected by the change.

This approach reduces considerably the number of style invalidations
and let us unskip the align-items repaint and relayout related
tests. These, as well as the justify-items related ones, had to be
rebaselined due the different invalidation areas generated.

Because of resolving everything in the StyleAdjuster, we don't need to
duplicate the resolution logic for generating the computed style. We
only have to deal with the 'auto' flag duality, which is not both
'auto' and 'normal' in any post-adjustement logic. This also gives us
the opportunity to get rid of the ShadowDOM and slotted elements
issues, since we don't have to deal with parent's style.

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.

BUG=565883, 474798,  448371, 582230
==========

[jfernandez, 2016-08-17 23:52:33]

The CQ bit was checked by jfernandez@igalia.com

[jfernandez, 2016-08-17 23:52:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
cbiesinger@chromium.org, rune@opera.com, timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1709963002/#ps820001
(title: "Getting back the FullScreen fix, missed during the rebase.")

[commit-bot: I haz the power, 2016-08-17 23:53:03]

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, 2016-08-18 03:55:31]

Description was changed from

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be
used as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in
the parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. However, it's
still valid for justify-items and both of Self Alignment
properties. We will use the StyleAdjuster to resolve these 'auto'
values.

In order to deal with repaint and relayout whenever some alignment
property changes and resolve again the 'auto' values, current
implementation relies on triggering a Reattach whenever any of these
properties changes. This is a source of many problems for animations,
plugins, ... (See bug 580070 for details). The correct way of
implementing this is using "Inherit" instead so we can run the
StyleAdjuster again only for the nodes affected by the change.

This approach reduces considerably the number of style invalidations
and let us unskip the align-items repaint and relayout related
tests. These, as well as the justify-items related ones, had to be
rebaselined due the different invalidation areas generated.

Because of resolving everything in the StyleAdjuster, we don't need to
duplicate the resolution logic for generating the computed style. We
only have to deal with the 'auto' flag duality, which is not both
'auto' and 'normal' in any post-adjustement logic. This also gives us
the opportunity to get rid of the ShadowDOM and slotted elements
issues, since we don't have to deal with parent's style.

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.

BUG=565883, 474798,  448371, 582230
==========

to

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be
used as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in
the parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. However, it's
still valid for justify-items and both of Self Alignment
properties. We will use the StyleAdjuster to resolve these 'auto'
values.

In order to deal with repaint and relayout whenever some alignment
property changes and resolve again the 'auto' values, current
implementation relies on triggering a Reattach whenever any of these
properties changes. This is a source of many problems for animations,
plugins, ... (See bug 580070 for details). The correct way of
implementing this is using "Inherit" instead so we can run the
StyleAdjuster again only for the nodes affected by the change.

This approach reduces considerably the number of style invalidations
and let us unskip the align-items repaint and relayout related
tests. These, as well as the justify-items related ones, had to be
rebaselined due the different invalidation areas generated.

Because of resolving everything in the StyleAdjuster, we don't need to
duplicate the resolution logic for generating the computed style. We
only have to deal with the 'auto' flag duality, which is not both
'auto' and 'normal' in any post-adjustement logic. This also gives us
the opportunity to get rid of the ShadowDOM and slotted elements
issues, since we don't have to deal with parent's style.

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.

BUG=565883, 474798,  448371, 582230
==========

[commit-bot: I haz the power, 2016-08-18 03:55:33]

Committed patchset #33 (id:820001)

[commit-bot: I haz the power, 2016-08-18 03:58:23]

Description was changed from

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be
used as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in
the parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. However, it's
still valid for justify-items and both of Self Alignment
properties. We will use the StyleAdjuster to resolve these 'auto'
values.

In order to deal with repaint and relayout whenever some alignment
property changes and resolve again the 'auto' values, current
implementation relies on triggering a Reattach whenever any of these
properties changes. This is a source of many problems for animations,
plugins, ... (See bug 580070 for details). The correct way of
implementing this is using "Inherit" instead so we can run the
StyleAdjuster again only for the nodes affected by the change.

This approach reduces considerably the number of style invalidations
and let us unskip the align-items repaint and relayout related
tests. These, as well as the justify-items related ones, had to be
rebaselined due the different invalidation areas generated.

Because of resolving everything in the StyleAdjuster, we don't need to
duplicate the resolution logic for generating the computed style. We
only have to deal with the 'auto' flag duality, which is not both
'auto' and 'normal' in any post-adjustement logic. This also gives us
the opportunity to get rid of the ShadowDOM and slotted elements
issues, since we don't have to deal with parent's style.

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.

BUG=565883, 474798,  448371, 582230
==========

to

==========
[css-align] New CSS Value 'normal' for Self Alignment

The Box Alignment specification defines a new value 'normal' to be
used as default for the different layout models, which will define the
specific behavior for each case. This patch adds a new CSS value in
the parsing logic and adapts the Self Alignment properties to the new
value.

The 'auto' value is no longer valid for the 'align-items' property and
the Computed Value will be always the specified value. However, it's
still valid for justify-items and both of Self Alignment
properties. We will use the StyleAdjuster to resolve these 'auto'
values.

In order to deal with repaint and relayout whenever some alignment
property changes and resolve again the 'auto' values, current
implementation relies on triggering a Reattach whenever any of these
properties changes. This is a source of many problems for animations,
plugins, ... (See bug 580070 for details). The correct way of
implementing this is using "Inherit" instead so we can run the
StyleAdjuster again only for the nodes affected by the change.

This approach reduces considerably the number of style invalidations
and let us unskip the align-items repaint and relayout related
tests. These, as well as the justify-items related ones, had to be
rebaselined due the different invalidation areas generated.

Because of resolving everything in the StyleAdjuster, we don't need to
duplicate the resolution logic for generating the computed style. We
only have to deal with the 'auto' flag duality, which is not both
'auto' and 'normal' in any post-adjustement logic. This also gives us
the opportunity to get rid of the ShadowDOM and slotted elements
issues, since we don't have to deal with parent's style.

Additionally, this patch updates the layout logic as well, for both
Flexbox and Grid layout models.

BUG=565883, 474798,  448371, 582230

Committed: https://crrev.com/fe59cb90e7b08f1faa146088ddbd7589d87eebb5
Cr-Commit-Position: refs/heads/master@{#412728}
==========

[commit-bot: I haz the power, 2016-08-18 03:58:25]

Patchset 33 (id:??) landed as
https://crrev.com/fe59cb90e7b08f1faa146088ddbd7589d87eebb5
Cr-Commit-Position: refs/heads/master@{#412728}