Bug 172707 – [css-align][css-flex][css-grid] 'auto' values of align-self and justify-self must not be resolved

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Javier Fernandez]

The 'auto' values on align-self and justify-self must not be resolved using the parent's align-items and justify-items computed value. 

https://lists.w3.org/Archives/Public/www-style/2015Dec/0061.html

[Javier Fernandez]

Created attachment 311550 [details]
Patch

[Build Bot]

Comment on attachment 311550 [details]
Patch

Attachment 311550 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3844993

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 311554 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311550 [details]
Patch

Attachment 311550 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3845064

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 311557 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311550 [details]
Patch

Attachment 311550 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3845069

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 311558 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311550 [details]
Patch

Attachment 311550 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3845019

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 311559 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Javier Fernandez]

Created attachment 311637 [details]
Patch

[Build Bot]

Comment on attachment 311637 [details]
Patch

Attachment 311637 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3850504

New failing tests:
fast/repaint/align-items-overflow-change.html
svg/css/getComputedStyle-basic.xhtml
css3/parse-alignment-of-root-elements.html
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/repaint/align-items-change.html
fast/repaint/justify-items-overflow-change.html

[Build Bot]

Created attachment 311645 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311637 [details]
Patch

Attachment 311637 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3850538

New failing tests:
fast/repaint/align-items-overflow-change.html
svg/css/getComputedStyle-basic.xhtml
css3/parse-alignment-of-root-elements.html
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html
fast/repaint/align-items-change.html
fast/repaint/justify-items-overflow-change.html

[Build Bot]

Created attachment 311646 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311637 [details]
Patch

Attachment 311637 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3850509

New failing tests:
fast/css/getComputedStyle/computed-style-without-renderer.html
svg/css/getComputedStyle-basic.xhtml
css3/parse-alignment-of-root-elements.html
fast/css/getComputedStyle/computed-style.html
fast/repaint/align-items-overflow-change.html
fast/repaint/align-items-change.html
fast/repaint/justify-items-overflow-change.html

[Build Bot]

Created attachment 311647 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311637 [details]
Patch

Attachment 311637 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3850513

New failing tests:
css3/parse-alignment-of-root-elements.html

[Build Bot]

Created attachment 311649 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Javier Fernandez]

Created attachment 311658 [details]
Patch

[Build Bot]

Comment on attachment 311658 [details]
Patch

Attachment 311658 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3851105

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 311667 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311658 [details]
Patch

Attachment 311658 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3851196

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 311668 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311658 [details]
Patch

Attachment 311658 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3851241

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 311669 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311658 [details]
Patch

Attachment 311658 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3851304

New failing tests:
compositing/masks/compositing-clip-path-change-no-repaint.html

[Build Bot]

Created attachment 311670 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Javier Fernandez]

Created attachment 311696 [details]
Patch

[Build Bot]

Comment on attachment 311696 [details]
Patch

Attachment 311696 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3853731

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 311698 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311696 [details]
Patch

Attachment 311696 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3853745

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 311699 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311696 [details]
Patch

Attachment 311696 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3854614

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 311731 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Javier Fernandez]

Created attachment 311768 [details]
Patch

[Build Bot]

Comment on attachment 311768 [details]
Patch

Attachment 311768 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3856178

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 311773 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311768 [details]
Patch

Attachment 311768 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3856180

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 311775 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 311768 [details]
Patch

Attachment 311768 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3856340

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 311785 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Javier Fernandez]

Created attachment 311807 [details]
Patch

[Antti Koivisto]

Comment on attachment 311807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311807&action=review

This cleans up things nicely, r=me

> Source/WebCore/rendering/RenderFlexibleBox.cpp:249
> +        for (auto* child = firstChildBox(); child; child = child->nextSiblingBox()) {

You should be able to use

    for (auto& child : childrenOfType<RenderBox>(*this)) {

> Source/WebCore/rendering/RenderFlexibleBox.cpp:250
> +            ItemPosition previousAlignment =child->style().resolvedAlignSelf(oldStyle, selfAlignmentNormalBehavior()).position();

missing space after =

> Source/WebCore/rendering/RenderGrid.cpp:96
> +StyleSelfAlignmentData RenderGrid::selfAlignmentForChild(GridAxis axis, const RenderBox& child, const RenderStyle* style) const

Why is the style a pointer? I don't see this being called with null style.

> Source/WebCore/rendering/RenderGrid.cpp:128
> +        for (RenderBox* child = firstChildBox(); child; child = child->nextSiblingBox()) {

Here too

> Source/WebCore/rendering/RenderGrid.cpp:1205
> +StyleSelfAlignmentData RenderGrid::alignSelfForChild(const RenderBox& child, const RenderStyle* style) const

It is bit unclear whose style the 'style' is and when it is ok to call this will null style.

> Source/WebCore/rendering/RenderGrid.cpp:1212
> +StyleSelfAlignmentData RenderGrid::justifySelfForChild(const RenderBox& child, const RenderStyle* style) const

Same here.

[Javier Fernandez]

Comment on attachment 311807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=311807&action=review

>> Source/WebCore/rendering/RenderGrid.cpp:96
>> +StyleSelfAlignmentData RenderGrid::selfAlignmentForChild(GridAxis axis, const RenderBox& child, const RenderStyle* style) const
> 
> Why is the style a pointer? I don't see this being called with null style.

Actually it's default to nullptr in the function declaration, since the regular use of this function doesn't require an explicit style to resolve the 'auto' values. We should just uses the grid's style for resolving grid item's 'auto' values.

>> Source/WebCore/rendering/RenderGrid.cpp:1205
>> +StyleSelfAlignmentData RenderGrid::alignSelfForChild(const RenderBox& child, const RenderStyle* style) const
> 
> It is bit unclear whose style the 'style' is and when it is ok to call this will null style.

This follows the same patten that the selfAlignmentForChild function, so we allow passing nullptr so it uses the caller's style for resolving any 'auto' values.

[Antti Koivisto]

> Actually it's default to nullptr in the function declaration, since the
> regular use of this function doesn't require an explicit style to resolve
> the 'auto' values. We should just uses the grid's style for resolving grid
> item's 'auto' values.

You just added the function and as far as I see all call sites call it with non-null style. Are you planning to add more call sites?

> This follows the same patten that the selfAlignmentForChild function, so we
> allow passing nullptr so it uses the caller's style for resolving any 'auto'
> values.

Maybe there is a more descriptive name for it than just 'style'? There are two styles involved in

return child.style().resolvedJustifySelf(style, selfAlignmentNormalBehavior(&child)); 

Would the latter be for example 'gridStyle' or 'parentStyle'?

[Javier Fernandez]

(In reply to Antti Koivisto from comment #45)
> > Actually it's default to nullptr in the function declaration, since the
> > regular use of this function doesn't require an explicit style to resolve
> > the 'auto' values. We should just uses the grid's style for resolving grid
> > item's 'auto' values.
> 
> You just added the function and as far as I see all call sites call it with
> non-null style. Are you planning to add more call sites?


Yes, I've got another patch that will call that function several times with the nullptr style. I'm still working on it, so I'd be ok with simplifying this function now if you think it's clearer.  

> 
> > This follows the same patten that the selfAlignmentForChild function, so we
> > allow passing nullptr so it uses the caller's style for resolving any 'auto'
> > values.
> 
> Maybe there is a more descriptive name for it than just 'style'? There are
> two styles involved in
> 
> return child.style().resolvedJustifySelf(style,
> selfAlignmentNormalBehavior(&child)); 
> 
> Would the latter be for example 'gridStyle' or 'parentStyle'?

Yes, those would be definitively better names.

[Javier Fernandez]

Created attachment 315036 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 315036 [details]
Patch

Clearing flags on attachment: 315036

Committed r219315: <http://trac.webkit.org/changeset/219315>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.