The new specification CSS Box Alignment Module Level 3 requires several changes in both properties.
Created attachment 234786 [details] Patch
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
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
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
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
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
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
Created attachment 234798 [details] Patch
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.
(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.
(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.
Created attachment 235769 [details] Patch Patch rebased and adding some additional changes to adapt the implementation to the last CSS specs.
Created attachment 235775 [details] Patch Fixed build error because of last-baseline not being handled in switch.
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
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
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
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
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
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
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
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
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.
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.
Created attachment 235903 [details] Patch Don't resolve align-self during css cascade, but during layout.
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
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
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
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
Created attachment 235920 [details] Patch Do nothing with the unimplemented aligment values, insteead of defaulting to 'stretch.'
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
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
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
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
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
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
Created attachment 236927 [details] Patch Fixed some mathml related tests.
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
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
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
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
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
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
Created attachment 237080 [details] Patch Fixed some issues in the accessibility tests.
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
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
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
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
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
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
Created attachment 237224 [details] Patch Still WIP patch, trying to fix the accessibility issues.
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.
(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.
Created attachment 240511 [details] Patch Patch rebased.
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?
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+.
Created attachment 241419 [details] Patch Applied suggested changes.
Created attachment 241420 [details] Patch Updated changelog.
Created attachment 241423 [details] Patch Path rebased.
Created attachment 241426 [details] Patch Adding FALLTHROUGH macros to avoid building errors.
Created attachment 241479 [details] Patch Fixed some build errors.
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
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
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
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
Created attachment 241591 [details] Patch Fixed layout tests.
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
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
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
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
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
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
Created attachment 241707 [details] Patch
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
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
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
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
Created attachment 241719 [details] Patch Patch rebased.
Comment on attachment 241719 [details] Patch Clearing flags on attachment: 241719 Committed r176218: <http://trac.webkit.org/changeset/176218>
All reviewed patches have been landed. Closing bug.
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.
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.
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.
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.
Re-opened since this is blocked by bug 138827
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.
(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.
(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.
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
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
Created attachment 241874 [details] Patch Rebaseline some layout tests.
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.
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
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
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
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
Created attachment 241893 [details] Patch Resolving 'auto' values for the alignment properties used in flexbox.
Gently ping, reviewers.
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?
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.
How are you going to solve these failures? I think Hyatt should review this.
(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.
Any feedback on this ?
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.
(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.
Created attachment 248555 [details] Patch
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
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
Created attachment 248559 [details] Patch
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
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
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
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
Created attachment 248569 [details] Patch
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)?
Created attachment 248930 [details] Patch
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
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
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
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
Created attachment 248938 [details] Patch
Comment on attachment 248938 [details] Patch r=me
Comment on attachment 248938 [details] Patch Clearing flags on attachment: 248938 Committed r182147: <http://trac.webkit.org/changeset/182147>