Bug 133359

Summary: [CSS Box Alignment] Upgrade align-self and align-items parsing to CSS 3
Product: WebKit Reporter: Javier Fernandez <jfernandez>
Component: CSSAssignee: Javier Fernandez <jfernandez>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, cdumez, commit-queue, darin, dino, hyatt, jfernandez, kling, koivisto, rego, rniwa, svillar, syoichi
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 136291, 138095, 138827    
Bug Blocks: 91512, 133224, 111616    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Patch
none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Patch
none
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
none
Patch
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Patch
none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Patch
none
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Patch
none
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
none
Patch
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
none
Patch
none
Patch
none
Archive of layout-test-results from ews105 for mac-mavericks-wk2
none
Patch
none
Archive of layout-test-results from ews105 for mac-mavericks-wk2
none
Archive of layout-test-results from ews102 for mac-mavericks
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-mavericks
none
Archive of layout-test-results from ews106 for mac-mavericks-wk2
none
Patch none

Javier Fernandez
Reported 2014-05-28 15:11:35 PDT
The new specification CSS Box Alignment Module Level 3 requires several changes in both properties.
Attachments
Patch (80.62 KB, patch)
2014-07-11 15:23 PDT, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (497.44 KB, application/zip)
2014-07-11 16:33 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (525.28 KB, application/zip)
2014-07-11 17:23 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (554.79 KB, application/zip)
2014-07-11 18:23 PDT, Build Bot
no flags
Patch (81.93 KB, patch)
2014-07-12 03:16 PDT, Javier Fernandez
no flags
Patch (115.88 KB, patch)
2014-07-30 14:59 PDT, Javier Fernandez
no flags
Patch (115.92 KB, patch)
2014-07-30 15:53 PDT, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (570.00 KB, application/zip)
2014-07-30 17:44 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (622.30 KB, application/zip)
2014-07-30 18:26 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (629.95 KB, application/zip)
2014-07-30 19:00 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (597.35 KB, application/zip)
2014-07-30 19:51 PDT, Build Bot
no flags
Patch (116.15 KB, patch)
2014-08-01 14:03 PDT, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (558.10 KB, application/zip)
2014-08-01 15:20 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (552.24 KB, application/zip)
2014-08-01 15:57 PDT, Build Bot
no flags
Patch (116.18 KB, patch)
2014-08-01 16:13 PDT, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (802.29 KB, application/zip)
2014-08-01 17:22 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (887.62 KB, application/zip)
2014-08-01 18:25 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (805.03 KB, application/zip)
2014-08-01 18:41 PDT, Build Bot
no flags
Patch (117.18 KB, patch)
2014-08-21 08:32 PDT, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (499.26 KB, application/zip)
2014-08-21 10:19 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (483.19 KB, application/zip)
2014-08-21 10:51 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (500.90 KB, application/zip)
2014-08-21 11:20 PDT, Build Bot
no flags
Patch (117.68 KB, patch)
2014-08-25 06:04 PDT, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (483.07 KB, application/zip)
2014-08-25 08:24 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (499.49 KB, application/zip)
2014-08-25 08:57 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (502.23 KB, application/zip)
2014-08-25 09:54 PDT, Build Bot
no flags
Patch (118.36 KB, patch)
2014-08-27 06:39 PDT, Javier Fernandez
no flags
Patch (118.73 KB, patch)
2014-10-27 16:03 PDT, Javier Fernandez
no flags
Patch (118.95 KB, patch)
2014-11-12 01:57 PST, Javier Fernandez
no flags
Patch (118.95 KB, patch)
2014-11-12 02:04 PST, Javier Fernandez
no flags
Patch (118.80 KB, patch)
2014-11-12 06:39 PST, Javier Fernandez
no flags
Patch (59.03 KB, patch)
2014-11-12 07:56 PST, Javier Fernandez
no flags
Patch (118.91 KB, patch)
2014-11-13 02:40 PST, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (662.03 KB, application/zip)
2014-11-13 20:46 PST, Build Bot
no flags
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (683.32 KB, application/zip)
2014-11-13 22:58 PST, Build Bot
no flags
Patch (118.94 KB, patch)
2014-11-14 08:38 PST, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (488.79 KB, application/zip)
2014-11-14 10:09 PST, Build Bot
no flags
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (505.94 KB, application/zip)
2014-11-14 10:52 PST, Build Bot
no flags
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (506.09 KB, application/zip)
2014-11-14 13:00 PST, Build Bot
no flags
Patch (59.95 KB, patch)
2014-11-17 06:46 PST, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (592.31 KB, application/zip)
2014-11-17 08:17 PST, Build Bot
no flags
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (604.24 KB, application/zip)
2014-11-17 08:38 PST, Build Bot
no flags
Patch (119.06 KB, patch)
2014-11-17 09:30 PST, Javier Fernandez
no flags
Patch (116.92 KB, patch)
2014-11-19 10:19 PST, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (936.82 KB, application/zip)
2014-11-19 11:42 PST, Build Bot
no flags
Patch (139.16 KB, patch)
2014-11-19 12:16 PST, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (810.34 KB, application/zip)
2014-11-19 13:51 PST, Build Bot
no flags
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (783.26 KB, application/zip)
2014-11-19 14:38 PST, Build Bot
no flags
Patch (140.51 KB, patch)
2014-11-19 15:00 PST, Javier Fernandez
no flags
Patch (145.43 KB, patch)
2015-03-12 16:55 PDT, Javier Fernandez
no flags
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (676.44 KB, application/zip)
2015-03-12 17:40 PDT, Build Bot
no flags
Patch (145.43 KB, patch)
2015-03-12 17:50 PDT, Javier Fernandez
no flags
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (574.84 KB, application/zip)
2015-03-12 18:37 PDT, Build Bot
no flags
Archive of layout-test-results from ews102 for mac-mavericks (609.85 KB, application/zip)
2015-03-12 19:10 PDT, Build Bot
no flags
Patch (145.94 KB, patch)
2015-03-12 23:03 PDT, Javier Fernandez
no flags
Patch (145.92 KB, patch)
2015-03-18 07:46 PDT, Javier Fernandez
no flags
Archive of layout-test-results from ews102 for mac-mavericks (538.91 KB, application/zip)
2015-03-18 08:56 PDT, Build Bot
no flags
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (585.44 KB, application/zip)
2015-03-18 09:00 PDT, Build Bot
no flags
Patch (145.92 KB, patch)
2015-03-18 09:17 PDT, Javier Fernandez
no flags
Javier Fernandez
Comment 1 2014-07-11 15:23:39 PDT
Build Bot
Comment 2 2014-07-11 16:33:30 PDT
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
Comment 3 2014-07-11 16:33:32 PDT
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 4 2014-07-11 17:23:53 PDT
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
Comment 5 2014-07-11 17:23:56 PDT
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 6 2014-07-11 18:23:12 PDT
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
Comment 7 2014-07-11 18:23:15 PDT
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
Comment 8 2014-07-12 03:16:51 PDT
Benjamin Poulain
Comment 9 2014-07-14 23:19:31 PDT
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
Comment 10 2014-07-15 14:42:08 PDT
(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
Comment 11 2014-07-15 15:01:01 PDT
(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
Comment 12 2014-07-30 14:59:40 PDT
Created attachment 235769 [details] Patch Patch rebased and adding some additional changes to adapt the implementation to the last CSS specs.
Javier Fernandez
Comment 13 2014-07-30 15:53:08 PDT
Created attachment 235775 [details] Patch Fixed build error because of last-baseline not being handled in switch.
Build Bot
Comment 14 2014-07-30 17:44:15 PDT
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
Comment 15 2014-07-30 17:44:19 PDT
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 16 2014-07-30 18:26:51 PDT
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
Comment 17 2014-07-30 18:26:55 PDT
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 18 2014-07-30 19:00:38 PDT
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
Comment 19 2014-07-30 19:00:41 PDT
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 20 2014-07-30 19:51:22 PDT
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
Comment 21 2014-07-30 19:51:26 PDT
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 22 2014-07-30 21:15:29 PDT
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
Comment 23 2014-07-31 17:15:27 PDT
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
Comment 24 2014-08-01 14:03:29 PDT
Created attachment 235903 [details] Patch Don't resolve align-self during css cascade, but during layout.
Build Bot
Comment 25 2014-08-01 15:20:53 PDT
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
Comment 26 2014-08-01 15:20:57 PDT
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 27 2014-08-01 15:57:21 PDT
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
Comment 28 2014-08-01 15:57:25 PDT
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
Comment 29 2014-08-01 16:13:40 PDT
Created attachment 235920 [details] Patch Do nothing with the unimplemented aligment values, insteead of defaulting to 'stretch.'
Build Bot
Comment 30 2014-08-01 17:22:29 PDT
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
Comment 31 2014-08-01 17:22:33 PDT
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 32 2014-08-01 18:25:05 PDT
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
Comment 33 2014-08-01 18:25:10 PDT
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 34 2014-08-01 18:41:06 PDT
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
Comment 35 2014-08-01 18:41:10 PDT
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
Comment 36 2014-08-21 08:32:55 PDT
Created attachment 236927 [details] Patch Fixed some mathml related tests.
Build Bot
Comment 37 2014-08-21 10:19:39 PDT
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
Comment 38 2014-08-21 10:19:45 PDT
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 39 2014-08-21 10:51:27 PDT
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
Comment 40 2014-08-21 10:51:32 PDT
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 41 2014-08-21 11:20:02 PDT
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
Comment 42 2014-08-21 11:20:07 PDT
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
Comment 43 2014-08-25 06:04:07 PDT
Created attachment 237080 [details] Patch Fixed some issues in the accessibility tests.
Build Bot
Comment 44 2014-08-25 08:24:07 PDT
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
Comment 45 2014-08-25 08:24:12 PDT
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 46 2014-08-25 08:57:38 PDT
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
Comment 47 2014-08-25 08:57:43 PDT
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 48 2014-08-25 09:54:34 PDT
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
Comment 49 2014-08-25 09:54:39 PDT
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
Comment 50 2014-08-27 06:39:50 PDT
Created attachment 237224 [details] Patch Still WIP patch, trying to fix the accessibility issues.
Javier Fernandez
Comment 51 2014-08-27 10:23:20 PDT
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
Comment 52 2014-10-27 13:32:57 PDT
(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
Comment 53 2014-10-27 16:03:56 PDT
Created attachment 240511 [details] Patch Patch rebased.
Benjamin Poulain
Comment 54 2014-11-07 19:42:01 PST
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 55 2014-11-11 11:19:28 PST
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
Comment 56 2014-11-12 01:57:10 PST
Created attachment 241419 [details] Patch Applied suggested changes.
Javier Fernandez
Comment 57 2014-11-12 02:04:00 PST
Created attachment 241420 [details] Patch Updated changelog.
Javier Fernandez
Comment 58 2014-11-12 06:39:59 PST
Created attachment 241423 [details] Patch Path rebased.
Javier Fernandez
Comment 59 2014-11-12 07:56:13 PST
Created attachment 241426 [details] Patch Adding FALLTHROUGH macros to avoid building errors.
Javier Fernandez
Comment 60 2014-11-13 02:40:04 PST
Created attachment 241479 [details] Patch Fixed some build errors.
Build Bot
Comment 61 2014-11-13 20:46:45 PST
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
Comment 62 2014-11-13 20:46:50 PST
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 63 2014-11-13 22:58:13 PST
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
Comment 64 2014-11-13 22:58:18 PST
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
Comment 65 2014-11-14 08:38:44 PST
Created attachment 241591 [details] Patch Fixed layout tests.
Build Bot
Comment 66 2014-11-14 10:09:36 PST
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
Comment 67 2014-11-14 10:09:42 PST
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 68 2014-11-14 10:52:10 PST
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
Comment 69 2014-11-14 10:52:15 PST
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 70 2014-11-14 13:00:06 PST
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
Comment 71 2014-11-14 13:00:10 PST
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
Comment 72 2014-11-17 06:46:52 PST
Build Bot
Comment 73 2014-11-17 08:17:40 PST
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
Comment 74 2014-11-17 08:17:46 PST
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 75 2014-11-17 08:38:12 PST
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
Comment 76 2014-11-17 08:38:17 PST
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
Comment 77 2014-11-17 09:30:21 PST
Created attachment 241719 [details] Patch Patch rebased.
WebKit Commit Bot
Comment 78 2014-11-17 11:52:38 PST
Comment on attachment 241719 [details] Patch Clearing flags on attachment: 241719 Committed r176218: <http://trac.webkit.org/changeset/176218>
WebKit Commit Bot
Comment 79 2014-11-17 11:52:53 PST
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 80 2014-11-17 20:19:33 PST
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 81 2014-11-17 20:32:06 PST
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
Comment 82 2014-11-17 22:01:27 PST
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 83 2014-11-17 22:07:19 PST
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
Comment 84 2014-11-18 02:43:10 PST
Re-opened since this is blocked by bug 138827
Javier Fernandez
Comment 85 2014-11-19 10:19:25 PST
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
Comment 86 2014-11-19 10:26:18 PST
(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
Comment 87 2014-11-19 10:27:34 PST
(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 88 2014-11-19 11:42:31 PST
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
Comment 89 2014-11-19 11:42:37 PST
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
Comment 90 2014-11-19 12:16:00 PST
Created attachment 241874 [details] Patch Rebaseline some layout tests.
Chris Dumez
Comment 91 2014-11-19 13:42:17 PST
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 92 2014-11-19 13:51:51 PST
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
Comment 93 2014-11-19 13:51:58 PST
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 94 2014-11-19 14:38:23 PST
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
Comment 95 2014-11-19 14:38:29 PST
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
Comment 96 2014-11-19 15:00:07 PST
Created attachment 241893 [details] Patch Resolving 'auto' values for the alignment properties used in flexbox.
Javier Fernandez
Comment 97 2014-12-01 09:32:53 PST
Gently ping, reviewers.
Antti Koivisto
Comment 98 2014-12-08 01:01:06 PST
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
Comment 99 2014-12-08 07:25:33 PST
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
Comment 100 2014-12-12 07:11:59 PST
How are you going to solve these failures? I think Hyatt should review this.
Javier Fernandez
Comment 101 2014-12-15 02:13:53 PST
(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
Comment 102 2014-12-18 05:44:27 PST
Any feedback on this ?
Sergio Villar Senin
Comment 103 2015-02-27 07:00:37 PST
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
Comment 104 2015-02-27 10:38:10 PST
(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
Comment 105 2015-03-12 16:55:40 PDT
Build Bot
Comment 106 2015-03-12 17:40:23 PDT
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
Comment 107 2015-03-12 17:40:30 PDT
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
Comment 108 2015-03-12 17:50:29 PDT
Build Bot
Comment 109 2015-03-12 18:37:42 PDT
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
Comment 110 2015-03-12 18:37:50 PDT
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 111 2015-03-12 19:10:43 PDT
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
Comment 112 2015-03-12 19:10:51 PDT
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
Comment 113 2015-03-12 23:03:54 PDT
Dave Hyatt
Comment 114 2015-03-17 09:28:54 PDT
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
Comment 115 2015-03-18 07:46:55 PDT
Build Bot
Comment 116 2015-03-18 08:56:39 PDT
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
Comment 117 2015-03-18 08:56:48 PDT
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 118 2015-03-18 08:59:50 PDT
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
Comment 119 2015-03-18 09:00:01 PDT
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
Comment 120 2015-03-18 09:17:03 PDT
Dave Hyatt
Comment 121 2015-03-26 09:33:08 PDT
Comment on attachment 248938 [details] Patch r=me
WebKit Commit Bot
Comment 122 2015-03-30 11:34:28 PDT
Comment on attachment 248938 [details] Patch Clearing flags on attachment: 248938 Committed r182147: <http://trac.webkit.org/changeset/182147>
WebKit Commit Bot
Comment 123 2015-03-30 11:34:44 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.