Bug 133359 – [CSS Box Alignment] Upgrade align-self and align-items parsing to CSS 3

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

[Javier Fernandez]

The new specification CSS Box Alignment Module Level 3 requires several changes in both properties.

[Javier Fernandez]

Created attachment 234786 [details]
Patch

[Build Bot]

Comment on attachment 234786 [details]
Patch

Attachment 234786 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6592067010560000

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

[Build Bot]

Created attachment 234791 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 234786 [details]
Patch

Attachment 234786 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4548818729172992

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

[Build Bot]

Created attachment 234793 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

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

[Build Bot]

Comment on attachment 234786 [details]
Patch

Attachment 234786 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6265415420346368

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

[Build Bot]

Created attachment 234794 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

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

[Javier Fernandez]

Created attachment 234798 [details]
Patch

[Benjamin Poulain]

Comment on attachment 234798 [details]
Patch

I did not get the time to do a review today :(

I like that you are removing EAlignItems. I am not sure I like the name ItemPosition to replace it...

But honestly, given it touches code outside CSS GRID, I would prefer if this patch waited 1-2 weeks so that it does not land in the middle of a Safari-WebKit branch.

[Benjamin Poulain]

(In reply to comment #9)
> But honestly, given it touches code outside CSS GRID, I would prefer if this patch waited 1-2 weeks so that it does not land in the middle of a Safari-WebKit branch.

I am told we just have to wait until the end of this week to avoid messing with the branches.

[Javier Fernandez]

(In reply to comment #10)
> (In reply to comment #9)
> > But honestly, given it touches code outside CSS GRID, I would prefer if this patch waited 1-2 weeks so that it does not land in the middle of a Safari-WebKit branch.
> 
> I am told we just have to wait until the end of this week to avoid messing with the branches.

Sure, no problem. I still have other patches related to alignment and justification to work on.
Thanks anyway.

[Javier Fernandez]

Created attachment 235769 [details]
Patch

Patch rebased and adding some additional changes to adapt the implementation to the last CSS specs.

[Javier Fernandez]

Created attachment 235775 [details]
Patch

Fixed build error because of last-baseline not being handled in switch.

[Build Bot]

Comment on attachment 235775 [details]
Patch

Attachment 235775 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6339971052994560

New failing tests:
svg/css/getComputedStyle-basic.xhtml
mathml/presentation/inferred-mrow-baseline.html
fast/css/getComputedStyle/computed-style.html
mathml/presentation/scripts-vertical-alignment.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 235786 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

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

[Build Bot]

Comment on attachment 235775 [details]
Patch

Attachment 235775 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5547652666097664

New failing tests:
svg/css/getComputedStyle-basic.xhtml
mathml/presentation/inferred-mrow-baseline.html
fast/css/getComputedStyle/computed-style.html
mathml/presentation/scripts-vertical-alignment.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 235793 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

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

[Build Bot]

Comment on attachment 235775 [details]
Patch

Attachment 235775 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5889672723038208

New failing tests:
svg/css/getComputedStyle-basic.xhtml
mathml/presentation/inferred-mrow-baseline.html
fast/css/getComputedStyle/computed-style.html
mathml/presentation/scripts-vertical-alignment.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 235798 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 235775 [details]
Patch

Attachment 235775 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6132796863021056

New failing tests:
svg/css/getComputedStyle-basic.xhtml
mathml/presentation/inferred-mrow-baseline.html
fast/css/getComputedStyle/computed-style.html
mathml/presentation/scripts-vertical-alignment.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 235803 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

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

[Benjamin Poulain]

Comment on attachment 235775 [details]
Patch

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

I only had a quick look :(
Some comments:

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1650
> +static ItemPosition resolveAlignmentAuto(ItemPosition position, Node* element)
> +{
> +    if (position != ItemPositionAuto || !element || !element->computedStyle())
> +        return position;
> +
> +    return element->computedStyle()->isDisplayFlexibleOrGridBox() ? ItemPositionStretch : ItemPositionStart;
> +}

I don't understand this little dance with the computed style. You are using this from ComputedStyleExtractor, you should not get the style directly from the element, you take the style as an argument from the ComputedStyleExtractor.

The one taking the parent node is another problem, in that case you need to test "element" and "element->computedStyle()".

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1657
> +    result->append(CSSPrimitiveValue::create(itemPosition));
> +    if (itemPosition >= ItemPositionCenter && overflowAlignment != OverflowAlignmentDefault)
> +        result->append(CSSPrimitiveValue::create(overflowAlignment));

Why is this code avoiding the Value Pool?

> Source/WebCore/css/CSSParser.cpp:3085
> +static bool isBaselinePositionKeyword(CSSValueID id)

static inline

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1305
> +                // FIXME: File a bug about implementing that. The extended grammar
> +                // is not enabled by default so we shouldn't hit this codepath.

File the bug, put the bugzilla number here :)

> Source/WebCore/rendering/style/RenderStyle.h:1583
> +

Blank line.

[Javier Fernandez]

It seems that my approach of resolving the "auto" values during the CSS Cascade have some limitations, which caused some mathhml layout tests to fail.

This is the Box Aligment specification I'm following in my mplementation:

  - http://dev.w3.org/csswg/css-align-3/

The root cause of these failires is that mathml layout code changes the value of the alignItems property on several renders. I expected that to force a style recalculation, as it happens when properties are changed through javascript. This is controlled by the WebCore::Style::determineChange, which now it forces a Dettach whenever the change affects to the alignItems property.

The intention of my patch was to avoid logic duplication of this "auto" resolution, since both grid an flex share the same logic. Also, the CSScomputedStyleDeclaration needs to implement exactly the same logic. Besides, now, according to the last specs, the default value of all the properties is "auto", so when resolving the "alignSelf", it's not enough to refer to the parent's alignItem; this has to be resolved too.

To complicate things even more,  there is now a "legacy"  keyword that forces the "justifyItems" property to be inherited by all the descendants. All this logic seems to be naturally implemented in the css cascade, which also would simplify the resolution implemented by the renders. 

However, there are several issues on this approach; first of all, the anonymous renderers are not part of the css cascade resolution. Also, several renders derive from RenderFelxibleBoxm but they don't specify the appropriated display value, which is what the css cascade use to discriminate between different elements. Finally, changes in the alignItem performed in the layout phase should trigger, somehow, a style recalc at least in the subtree.

[Javier Fernandez]

Created attachment 235903 [details]
Patch

Don't resolve align-self during css cascade, but during layout.

[Build Bot]

Comment on attachment 235903 [details]
Patch

Attachment 235903 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4617926464241664

New failing tests:
platform/mac/accessibility/webkit-alt-for-css-content.html
mathml/presentation/style-changed.html

[Build Bot]

Created attachment 235908 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 235903 [details]
Patch

Attachment 235903 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5095897268486144

New failing tests:
platform/mac/accessibility/webkit-alt-for-css-content.html
mathml/presentation/style-changed.html

[Build Bot]

Created attachment 235915 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

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

[Javier Fernandez]

Created attachment 235920 [details]
Patch

Do nothing with the unimplemented aligment values, insteead of defaulting to 'stretch.'

[Build Bot]

Comment on attachment 235920 [details]
Patch

Attachment 235920 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5135179039375360

New failing tests:
fast/forms/range/range-hit-test-with-padding.html
media/video-no-audio.html
media/controls-strict.html
fast/multicol/newmulticol/client-rects.html
platform/mac/accessibility/webkit-alt-for-css-content.html
media/audio-controls-rendering.html
fast/multicol/client-rects-spanners.html
media/controls-without-preload.html
fast/forms/range/input-appearance-range.html
fast/multicol/client-rects.html
fast/forms/range/range-thumb-height-percentage.html
fast/multicol/client-rects-spanners-complex.html

[Build Bot]

Created attachment 235926 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 235920 [details]
Patch

Attachment 235920 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6401816434573312

New failing tests:
fast/forms/range/range-hit-test-with-padding.html
media/video-no-audio.html
media/controls-strict.html
fast/multicol/newmulticol/client-rects.html
platform/mac/accessibility/webkit-alt-for-css-content.html
media/audio-controls-rendering.html
fast/multicol/client-rects-spanners.html
media/controls-without-preload.html
fast/forms/range/input-appearance-range.html
fast/multicol/client-rects.html
fast/forms/range/range-thumb-height-percentage.html
media/controls-after-reload.html
fast/multicol/client-rects-spanners-complex.html

[Build Bot]

Created attachment 235927 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

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

[Build Bot]

Comment on attachment 235920 [details]
Patch

Attachment 235920 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6057806734032896

New failing tests:
fast/forms/range/range-hit-test-with-padding.html
media/video-no-audio.html
media/controls-strict.html
fast/multicol/newmulticol/client-rects.html
platform/mac/accessibility/webkit-alt-for-css-content.html
media/audio-controls-rendering.html
fast/multicol/client-rects-spanners.html
media/controls-without-preload.html
fast/forms/range/input-appearance-range.html
fast/multicol/client-rects.html
fast/forms/range/range-thumb-height-percentage.html
fast/multicol/client-rects-spanners-complex.html

[Build Bot]

Created attachment 235928 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

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

[Javier Fernandez]

Created attachment 236927 [details]
Patch

Fixed some mathml related tests.

[Build Bot]

Comment on attachment 236927 [details]
Patch

Attachment 236927 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5499295025332224

New failing tests:
platform/mac/accessibility/webkit-alt-for-css-content.html

[Build Bot]

Created attachment 236928 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

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

[Build Bot]

Comment on attachment 236927 [details]
Patch

Attachment 236927 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6264888783536128

New failing tests:
platform/mac/accessibility/webkit-alt-for-css-content.html

[Build Bot]

Created attachment 236929 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 236927 [details]
Patch

Attachment 236927 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4668026653769728

New failing tests:
platform/mac/accessibility/webkit-alt-for-css-content.html

[Build Bot]

Created attachment 236930 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

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

[Javier Fernandez]

Created attachment 237080 [details]
Patch

Fixed some issues in the accessibility tests.

[Build Bot]

Comment on attachment 237080 [details]
Patch

Attachment 237080 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6213398568108032

New failing tests:
platform/mac/accessibility/webkit-alt-for-css-content.html

[Build Bot]

Created attachment 237085 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 237080 [details]
Patch

Attachment 237080 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4910035712868352

New failing tests:
platform/mac/accessibility/webkit-alt-for-css-content.html

[Build Bot]

Created attachment 237088 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

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

[Build Bot]

Comment on attachment 237080 [details]
Patch

Attachment 237080 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5743889117872128

New failing tests:
platform/mac/accessibility/webkit-alt-for-css-content.html

[Build Bot]

Created attachment 237091 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

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

[Javier Fernandez]

Created attachment 237224 [details]
Patch

Still WIP patch, trying to fix the accessibility issues.

[Javier Fernandez]

For some reason, I still don't fully understand (I have some ideas, though) my patch triggers the issue described in the bug #136291. That's why my first patches caused the platform/mac/accessibility/webkit-alt-for-css-content.html test to fail. 

I've filled that bug to track the issue and make this dependent of it; I'll do the same with the mathml related test failing. My last patch provides fixes for both tests, but I think separated bugs for them are more convenient.

[Javier Fernandez]

(In reply to comment #51)
> For some reason, I still don't fully understand (I have some ideas, though)
> my patch triggers the issue described in the bug #136291. That's why my
> first patches caused the
> platform/mac/accessibility/webkit-alt-for-css-content.html test to fail. 
> 
> I've filled that bug to track the issue and make this dependent of it; I'll
> do the same with the mathml related test failing. My last patch provides
> fixes for both tests, but I think separated bugs for them are more
> convenient.

Filed bug #138095 to track the MathML issue, which will be noticeable once this patch lands.

[Javier Fernandez]

Created attachment 240511 [details]
Patch

Patch rebased.

[Benjamin Poulain]

Comment on attachment 240511 [details]
Patch

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

I don't think I should r+. I would like Dave Hyatt to have a look.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1275
> +                // FIXME: https://webkit.org/b/135460 - The extended grammar is not supported
> +                // yet for FlexibleBox.
> +                // Defaulting to Stretch for now, as it what most of FlexBox based renders
> +                // expect as default.

I would add notImplemented() here.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1309
> +                // FIXME: https://webkit.org/b/135460 - The extended grammar is not supported
> +                // yet for FlexibleBox.

ditto

> Source/WebCore/style/StyleResolveTree.cpp:125
> +    if (s1->alignItems() != s2->alignItems())

Wait, what?

How did that not break tests?

[Dave Hyatt]

Comment on attachment 240511 [details]
Patch

Looks fine. I'm not really a fan of "ItemPosition" though. It just sounds too generic. I don't have a better suggestion though, so I guess r+.

[Javier Fernandez]

Created attachment 241419 [details]
Patch

Applied suggested changes.

[Javier Fernandez]

Created attachment 241420 [details]
Patch

Updated changelog.

[Javier Fernandez]

Created attachment 241423 [details]
Patch

Path rebased.

[Javier Fernandez]

Created attachment 241426 [details]
Patch

Adding FALLTHROUGH macros to avoid building errors.

[Javier Fernandez]

Created attachment 241479 [details]
Patch

Fixed some build errors.

[Build Bot]

Comment on attachment 241479 [details]
Patch

Attachment 241479 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6572785912512512

New failing tests:
fast/css/parse-justify-self.html
platform/mac/accessibility/alt-for-css-content.html

[Build Bot]

Created attachment 241547 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 241479 [details]
Patch

Attachment 241479 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6142428041969664

New failing tests:
fast/css/parse-justify-self.html
platform/mac/accessibility/alt-for-css-content.html

[Build Bot]

Created attachment 241560 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

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

[Javier Fernandez]

Created attachment 241591 [details]
Patch

Fixed layout tests.

[Build Bot]

Comment on attachment 241591 [details]
Patch

Attachment 241591 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5821991269433344

New failing tests:
platform/mac/accessibility/webkit-alt-for-css-content.html

[Build Bot]

Created attachment 241596 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 241591 [details]
Patch

Attachment 241591 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5504761763725312

New failing tests:
platform/mac/accessibility/webkit-alt-for-css-content.html

[Build Bot]

Created attachment 241597 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

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

[Build Bot]

Comment on attachment 241591 [details]
Patch

Attachment 241591 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6366499707027456

New failing tests:
platform/mac/accessibility/webkit-alt-for-css-content.html

[Build Bot]

Created attachment 241615 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

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

[Javier Fernandez]

Created attachment 241707 [details]
Patch

[Build Bot]

Comment on attachment 241707 [details]
Patch

Attachment 241707 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6105128599814144

New failing tests:
svg/css/getComputedStyle-basic.xhtml
css3/flexbox/css-properties.html
fast/css/getComputedStyle/computed-style.html
css3/parse-align-self.html
fast/css/getComputedStyle/computed-style-without-renderer.html
css3/parse-align-items.html

[Build Bot]

Created attachment 241716 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 241707 [details]
Patch

Attachment 241707 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5053329763729408

New failing tests:
svg/css/getComputedStyle-basic.xhtml
css3/flexbox/css-properties.html
fast/css/getComputedStyle/computed-style.html
css3/parse-align-self.html
fast/css/getComputedStyle/computed-style-without-renderer.html
css3/parse-align-items.html

[Build Bot]

Created attachment 241718 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

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

[Javier Fernandez]

Created attachment 241719 [details]
Patch

Patch rebased.

[WebKit Commit Bot]

Comment on attachment 241719 [details]
Patch

Clearing flags on attachment: 241719

Committed r176218: <http://trac.webkit.org/changeset/176218>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Comment on attachment 241719 [details]
Patch

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

> Source/WebCore/css/CSSPropertyNames.in:-363
> -align-items [NewStyleBuilder]

The idea is really to go in the other direction. We are working to port everything to the new StyleBuilder. It is going to be difficult if people move properties back to the old style builder :(

> Source/WebCore/css/CSSPropertyNames.in:-365
> -align-self [NewStyleBuilder, TypeName=EAlignItems]

Ditto.

[Chris Dumez]

Comment on attachment 241719 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:2907
> +    case CSSPropertyAlignSelf: {

Since you are now calling several setters on RenderStyle, you'll likely need to use [NewStyleBuilder, Custom=All] and move this code from StyleResolver.cpp to StyleBuilderCustom.h.

[Chris Dumez]

This patch seems to have caused regressions on the perf bots:
https://perf.webkit.org/#mode=dashboard

- DoYouEvenBench is ~1.6% worse
- html5 full parsing is ~3.9% worse

The regression range is:
http://trac.webkit.org/log/?rev=176222&stop_rev=176207&verbose=on

I tried reverting this patch locally and I confirmed a 1.6% progression on DoYouEvenBench and a 4% progression on html5 full parsing.

[Andreas Kling]

Comment on attachment 241719 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:1377
> +    if (style.alignItems() == ItemPositionAuto) {
> +        if (style.isDisplayFlexibleOrGridBox())
> +            style.setAlignItems(ItemPositionStretch);
> +        else
> +            style.setAlignItems(ItemPositionStart);
> +    }
> +
> +    // The 'auto' keyword computes to 'stretch' on absolutely-positioned elements,
> +    // and to the computed value of align-items on the parent (minus
> +    // any 'legacy' keywords) on all other boxes (to be resolved during the layout).
> +    if (style.alignSelf() == ItemPositionAuto) {
> +        if ((style.position() == AbsolutePosition))
> +            style.setAlignSelf(ItemPositionStretch);
> +    }

Looks like we will always overwrite values in RenderStyle now. That's not good since it will cause us to copy-on-write the StyleRareNonInheritedData.
This would be my first guess for the source of the performance regression.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 138827

[Javier Fernandez]

Created attachment 241867 [details]
Patch

Using now the new Style Builder, as requested. Also, removed the adjustStyleForAlignment method to avoid the performance regression; the required logic was moved to the computed style.

[Javier Fernandez]

(In reply to comment #83)
> Comment on attachment 241719 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241719&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:1377
> > +    if (style.alignItems() == ItemPositionAuto) {
> > +        if (style.isDisplayFlexibleOrGridBox())
> > +            style.setAlignItems(ItemPositionStretch);
> > +        else
> > +            style.setAlignItems(ItemPositionStart);
> > +    }
> > +
> > +    // The 'auto' keyword computes to 'stretch' on absolutely-positioned elements,
> > +    // and to the computed value of align-items on the parent (minus
> > +    // any 'legacy' keywords) on all other boxes (to be resolved during the layout).
> > +    if (style.alignSelf() == ItemPositionAuto) {
> > +        if ((style.position() == AbsolutePosition))
> > +            style.setAlignSelf(ItemPositionStretch);
> > +    }
> 
> Looks like we will always overwrite values in RenderStyle now. That's not
> good since it will cause us to copy-on-write the StyleRareNonInheritedData.
> This would be my first guess for the source of the performance regression.

You are right; this was the root cause of the performance regression. I've submitted a new patch which avoids this, but I had to move the required logic to the computed style declaration bits. 

I must mention, though, that current approach is enough for this patch but I think we will have to find other solutions when advancing on the implementation of the Box Alignment spec. The fact that 'auto' values are resolved to different values depending on the kind of elements, plus the hierarchic nature of the CSS logic makes some of the specification statements difficult to fulfil.

[Javier Fernandez]

(In reply to comment #81)
> Comment on attachment 241719 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241719&action=review
> 
> > Source/WebCore/css/StyleResolver.cpp:2907
> > +    case CSSPropertyAlignSelf: {
> 
> Since you are now calling several setters on RenderStyle, you'll likely need
> to use [NewStyleBuilder, Custom=All] and move this code from
> StyleResolver.cpp to StyleBuilderCustom.h.

Implemented that way in the last patch. Could you please review it ? Thanks.

[Build Bot]

Comment on attachment 241867 [details]
Patch

Attachment 241867 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5393353533292544

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/parse-justify-self.html
fast/css/getComputedStyle/computed-style.html
css3/flexbox/child-overflow.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[Build Bot]

Created attachment 241872 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

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

[Javier Fernandez]

Created attachment 241874 [details]
Patch

Rebaseline some layout tests.

[Chris Dumez]

Comment on attachment 241874 [details]
Patch

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

Looks like you might have one layout test failing on EWS?

> Source/WebCore/css/CSSPropertyNames.in:367
> +align-items [NewStyleBuilder, Custom=All]

Changes to this file look fine to me, but I am not the right person to review this change as a whole.

> Source/WebCore/css/StyleBuilderCustom.h:402
> +inline void applyInheritAlignSelf(StyleResolver& styleResolver)

Changes to this file look fine to me, but I am not the right person to review this change as a whole.

[Build Bot]

Comment on attachment 241874 [details]
Patch

Attachment 241874 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6464125387407360

New failing tests:
css3/flexbox/child-overflow.html

[Build Bot]

Created attachment 241886 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

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

[Build Bot]

Comment on attachment 241874 [details]
Patch

Attachment 241874 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5557438665195520

New failing tests:
css3/flexbox/child-overflow.html

[Build Bot]

Created attachment 241889 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

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

[Javier Fernandez]

Created attachment 241893 [details]
Patch

Resolving 'auto' values for the alignment properties used in flexbox.

[Javier Fernandez]

Gently ping, reviewers.

[Antti Koivisto]

Comment on attachment 241893 [details]
Patch

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

> LayoutTests/TestExpectations:72
> +webkit.org/b/133359 mathml/presentation/style-changed.html [ ImageOnlyFailure ]
> +webkit.org/b/136291 platform/mac/accessibility/webkit-alt-for-css-content.html [ Failure ]
> +webkit.org/b/136291 platform/mac/accessibility/alt-for-css-content.html [ Failure ]

Why is this patch making these tests fail?

[Javier Fernandez]

Hi Antii, 

(In reply to comment #98)
> > LayoutTests/TestExpectations:72
> > +webkit.org/b/133359 mathml/presentation/style-changed.html [ ImageOnlyFailure ]
> > +webkit.org/b/136291 platform/mac/accessibility/webkit-alt-for-css-content.html [ Failure ]
> > +webkit.org/b/136291 platform/mac/accessibility/alt-for-css-content.html [ Failure ]
> 
> Why is this patch making these tests fail?


The mathml patch fails because of the spec change which specifies 'auto' as default for the alignment properties, instead of 'stretch'. Such 'auto' value has to be resolved during the style cascade depending on the kind of element; 'stretch' for flex and grid containers, 'start' otherwise. The MathML implementation relies on the RenderFlexibleBox render to get such 'stretch' effect but it's not an actual flex element, from the CSS point of view. Also, it artificially manipulates the styles to get the desired behavior.

In the case of the accessibility patches it's a collateral effect of the style adjust by the StyleResolver, to resolve the auto' values, which forced a copy of the StlyeRareNonInheritedData structure (note that this copy was removed in the last patch because of the performance regression it introduces). The problem as, described in bug #136291, is that the m_altText field is not being copied as it should.

[Antti Koivisto]

How are you going to solve these failures? 

I think Hyatt should review this.

[Javier Fernandez]

(In reply to comment #100)
> How are you going to solve these failures? 

The accessibility tests failure would be solved with the patch attached in bug ##136291. Also worth mentioning that my path does not cause those test to fail, but just triggering an already present bug. Additionally, the root cause which triggered the accessibility related bug is the code added, in previous versions of this patch, into the StyleResolver::adjustStyleForAlignment function. This logic, even I consider it as a good approach, caused some performance regression so it was removed in the last version of the patch. Anyway, the accessibility bugs are still present because it's something it was not implemented correctly for the atlText property.

The mathml tests fail because the stretch CSS properties are not being handled correctly, specifically regarding the 'stretch' value and how it was implemented for mathml. The solution might need an important re-design of the mathml layout logic, which it was already discussed with some of the reviewers of such module and agreeing on that approach; it's just that it will take time so I think it's better to skip the test for the time being. 

> 
> I think Hyatt should review this.

Actually hyatt reviewed the previous version of the patch, the one using the StyleResolver::adjustStyleForAlignment logic, which I think is the proper way to resolve the 'auto' values of the css alignment properties. That's indeed the key issue to clarify in this discussion, since the original approach of updating the RenderStyle properties caused a performance regression, as kling pointed out in comment #83. 

The Box Alignment specification introduces 2 aspects that makes this patch more complicated than it should: 

1- The initial value for all the CSS properties is 'auto', which will be resolved to 'stretch' for Grid/Flex elements, or 'start' otherwise.

2- The 'auto' value of {align, justify}-self properties will be resolved to the '{align, justify}-items resolved value in the parent. 

The final version of the patch avoids the performance regression duplicating the 'auto' value resolution logic in the CSSCOmputedStyleDeclaration and in the specific RenderObjects (Flex or Grid, in this case). 

So, this is an issue about CSS parsing and how we would like to face the current behavior of the CSS alignment properties, based from now on on the CSS Box Alignment specification.

[Javier Fernandez]

Any feedback on this ?

[Sergio Villar Senin]

Are you planning to work on a refined version of the patch? If so, please remove the r? cq? flags as they will make the patch appear in the review queue.

[Javier Fernandez]

(In reply to comment #103)
> Are you planning to work on a refined version of the patch? If so, please
> remove the r? cq? flags as they will make the patch appear in the review
> queue.

Well, last patch fixes the bug and avoids the performance regression. While it might be not the best approach, it's still valid for continue the discussion here. I can't continue working on this without the proper feedback about the issues commented above, so I think 'r' flag still applies.

[Javier Fernandez]

Created attachment 248555 [details]
Patch

[Build Bot]

Comment on attachment 248555 [details]
Patch

Attachment 248555 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5052244311408640

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

[Build Bot]

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

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

[Javier Fernandez]

Created attachment 248559 [details]
Patch

[Build Bot]

Comment on attachment 248559 [details]
Patch

Attachment 248559 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6351142279708672

New failing tests:
svg/css/getComputedStyle-basic.xhtml

[Build Bot]

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

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

[Build Bot]

Comment on attachment 248559 [details]
Patch

Attachment 248559 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5583048582103040

New failing tests:
svg/css/getComputedStyle-basic.xhtml

[Build Bot]

Created attachment 248563 [details]
Archive of layout-test-results from ews102 for mac-mavericks

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

[Javier Fernandez]

Created attachment 248569 [details]
Patch

[Dave Hyatt]

Comment on attachment 248569 [details]
Patch

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

r-

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2133
> +                if (style->position() != AbsolutePosition) {
> +                    Node* parent = styledNode->parentNode();
> +                    if (parent && parent->computedStyle()) {
> +                        alignSelf = resolveAlignmentAuto(parent->computedStyle()->alignItems(), parent);
> +                        overflow = parent->computedStyle()->alignItemsOverflowAlignment();
> +                    }
> +                }

Why not use the render tree here? This code just looks completely wrong to me because it's trying to look at the DOM. For example, you left out FixedPositioned (render tree would just use isOutOfFlowPositioned), and you're going to the DOM parent (what if that is not in the same place in the render tree)?

[Javier Fernandez]

Created attachment 248930 [details]
Patch

[Build Bot]

Comment on attachment 248930 [details]
Patch

Attachment 248930 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5742453608939520

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

[Build Bot]

Created attachment 248934 [details]
Archive of layout-test-results from ews102 for mac-mavericks

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

[Build Bot]

Comment on attachment 248930 [details]
Patch

Attachment 248930 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6312140218564608

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

[Build Bot]

Created attachment 248935 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

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

[Javier Fernandez]

Created attachment 248938 [details]
Patch

[Dave Hyatt]

Comment on attachment 248938 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 248938 [details]
Patch

Clearing flags on attachment: 248938

Committed r182147: <http://trac.webkit.org/changeset/182147>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.