From 946b5679cdf9b42eb4002e438d7683d7761fb7be Mon Sep 17 00:00:00 2001 From: "jfernandez@igalia.com" Date: Wed, 19 Oct 2016 13:42:43 +0000 Subject: [PATCH] Web Inspector: Debugger buttons positioned incorrectly, align-content default value is unexpected https://bugs.webkit.org/show_bug.cgi?id=163572 Reviewed by Sergio Villar Senin. Source/WebCore: We only allow the new CSS Box Alignment syntax when the Grid Layout feature is enabled. Due to flexbox backward compatibility we have implemented a different code path for the style initial/default values assignment. However, we have incorrectly resolved both align-content and justify-content to 'flex-start' when grid layout is disabled. This patch changes the approach, so we set 'normal' (the value specified by the new syntax) for both properties, but using the values defined in the old syntax (Flexbox specification) at computed style resolution. Since 'stretch' is the default value for the align-content property, this issue implies that any flexbox line with an undefined height will be laid out incorrectly, if not explicitly set via CSS, because flex items can't use the available height, even though they use 'stretch' for their 'align-self' properties. Test: css3/flexbox/flexbox-lines-must-be-stretched-by-default.html * css/CSSComputedStyleDeclaration.cpp: (WebCore::valueForContentPositionAndDistributionWithOverflowAlignment): (WebCore::ComputedStyleExtractor::propertyValue): * rendering/style/RenderStyle.h: (WebCore::RenderStyle::initialContentAlignment): LayoutTests: Modified test cases for initial values. Added regression test for the align-content issue. * css3/flexbox/flexbox-lines-must-be-stretched-by-default-expected.txt: Added. * css3/flexbox/flexbox-lines-must-be-stretched-by-default.html: Added. * fast/css/ensure-flexbox-compatibility-with-initial-values-expected.txt: git-svn-id: https://svn.webkit.org/repository/webkit/trunk@207535 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- LayoutTests/ChangeLog | 14 +++++ ...lines-must-be-stretched-by-default-expected.txt | 7 +++ ...flexbox-lines-must-be-stretched-by-default.html | 63 ++++++++++++++++++++++ ...-compatibility-with-initial-values-expected.txt | 8 +-- Source/WebCore/ChangeLog | 31 +++++++++++ Source/WebCore/css/CSSComputedStyleDeclaration.cpp | 19 +++++-- Source/WebCore/rendering/style/RenderStyle.h | 2 +- 7 files changed, 134 insertions(+), 10 deletions(-) create mode 100644 LayoutTests/css3/flexbox/flexbox-lines-must-be-stretched-by-default-expected.txt create mode 100644 LayoutTests/css3/flexbox/flexbox-lines-must-be-stretched-by-default.html diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index fca34cf..6692065 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,3 +1,17 @@ +2016-10-19 Javier Fernandez + + Web Inspector: Debugger buttons positioned incorrectly, align-content default value is unexpected + https://bugs.webkit.org/show_bug.cgi?id=163572 + + Reviewed by Sergio Villar Senin. + + Modified test cases for initial values. + Added regression test for the align-content issue. + + * css3/flexbox/flexbox-lines-must-be-stretched-by-default-expected.txt: Added. + * css3/flexbox/flexbox-lines-must-be-stretched-by-default.html: Added. + * fast/css/ensure-flexbox-compatibility-with-initial-values-expected.txt: + 2016-10-19 Jer Noble [Mac][MSE] Movies with a 'mvex' box have a zero-duration diff --git a/LayoutTests/css3/flexbox/flexbox-lines-must-be-stretched-by-default-expected.txt b/LayoutTests/css3/flexbox/flexbox-lines-must-be-stretched-by-default-expected.txt new file mode 100644 index 0000000..7a29d9f --- /dev/null +++ b/LayoutTests/css3/flexbox/flexbox-lines-must-be-stretched-by-default-expected.txt @@ -0,0 +1,7 @@ +'Test for BUG=647694 - align-content "stretch" is not applied by default when grid is disabled.' + +PASS + +PASS + + diff --git a/LayoutTests/css3/flexbox/flexbox-lines-must-be-stretched-by-default.html b/LayoutTests/css3/flexbox/flexbox-lines-must-be-stretched-by-default.html new file mode 100644 index 0000000..c0ae6ac --- /dev/null +++ b/LayoutTests/css3/flexbox/flexbox-lines-must-be-stretched-by-default.html @@ -0,0 +1,63 @@ + + + + +

'Test for BUG=647694 - align-content "stretch" is not applied by default when grid is disabled.'

+ + diff --git a/LayoutTests/fast/css/ensure-flexbox-compatibility-with-initial-values-expected.txt b/LayoutTests/fast/css/ensure-flexbox-compatibility-with-initial-values-expected.txt index 74e9fd8..b1a3b03 100644 --- a/LayoutTests/fast/css/ensure-flexbox-compatibility-with-initial-values-expected.txt +++ b/LayoutTests/fast/css/ensure-flexbox-compatibility-with-initial-values-expected.txt @@ -25,19 +25,19 @@ PASS CSS.supports('justify-content', 'normal') is true Verifying initial values are supported when grid is DISABLED. PASS CSS.supports('align-items', 'stretch') is true PASS CSS.supports('align-self', 'stretch') is true -PASS CSS.supports('align-content', 'flex-start') is true +PASS CSS.supports('align-content', 'stretch') is true PASS CSS.supports('justify-content', 'flex-start') is true PASS CSS.supports('align-items', 'stretch') is true PASS CSS.supports('align-self', 'stretch') is true -PASS CSS.supports('align-content', 'flex-start') is true +PASS CSS.supports('align-content', 'stretch') is true PASS CSS.supports('justify-content', 'flex-start') is true PASS CSS.supports('align-items', 'stretch') is true PASS CSS.supports('align-self', 'stretch') is true -PASS CSS.supports('align-content', 'flex-start') is true +PASS CSS.supports('align-content', 'stretch') is true PASS CSS.supports('justify-content', 'flex-start') is true PASS CSS.supports('align-items', 'stretch') is true PASS CSS.supports('align-self', 'stretch') is true -PASS CSS.supports('align-content', 'flex-start') is true +PASS CSS.supports('align-content', 'stretch') is true PASS CSS.supports('justify-content', 'flex-start') is true PASS successfullyParsed is true diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index 9638948..cb5c1e7 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,34 @@ +2016-10-19 Javier Fernandez + + Web Inspector: Debugger buttons positioned incorrectly, align-content default value is unexpected + https://bugs.webkit.org/show_bug.cgi?id=163572 + + Reviewed by Sergio Villar Senin. + + We only allow the new CSS Box Alignment syntax when the Grid Layout + feature is enabled. Due to flexbox backward compatibility we have + implemented a different code path for the style initial/default values + assignment. However, we have incorrectly resolved both align-content + and justify-content to 'flex-start' when grid layout is disabled. + + This patch changes the approach, so we set 'normal' (the value specified + by the new syntax) for both properties, but using the values defined in + the old syntax (Flexbox specification) at computed style resolution. + + Since 'stretch' is the default value for the align-content property, this + issue implies that any flexbox line with an undefined height will be + laid out incorrectly, if not explicitly set via CSS, because flex items + can't use the available height, even though they use 'stretch' for their + 'align-self' properties. + + Test: css3/flexbox/flexbox-lines-must-be-stretched-by-default.html + + * css/CSSComputedStyleDeclaration.cpp: + (WebCore::valueForContentPositionAndDistributionWithOverflowAlignment): + (WebCore::ComputedStyleExtractor::propertyValue): + * rendering/style/RenderStyle.h: + (WebCore::RenderStyle::initialContentAlignment): + 2016-10-19 Carlos Alberto Lopez Perez [GTK] REGRESSION(r207396) Build broken with Clang. diff --git a/Source/WebCore/css/CSSComputedStyleDeclaration.cpp b/Source/WebCore/css/CSSComputedStyleDeclaration.cpp index 40f1e18..a70a871 100644 --- a/Source/WebCore/css/CSSComputedStyleDeclaration.cpp +++ b/Source/WebCore/css/CSSComputedStyleDeclaration.cpp @@ -62,6 +62,7 @@ #include "RenderBlock.h" #include "RenderBox.h" #include "RenderStyle.h" +#include "RuntimeEnabledFeatures.h" #include "SVGElement.h" #include "Settings.h" #include "ShapeValue.h" @@ -2414,14 +2415,22 @@ static Ref valueForItemPositionWithOverflowAlignment(const StyleSe return result; } -static Ref valueForContentPositionAndDistributionWithOverflowAlignment(const StyleContentAlignmentData& data) +static Ref valueForContentPositionAndDistributionWithOverflowAlignment(const StyleContentAlignmentData& data, CSSValueID normalBehaviorValueID) { auto& cssValuePool = CSSValuePool::singleton(); auto result = CSSValueList::createSpaceSeparated(); if (data.distribution() != ContentDistributionDefault) result.get().append(cssValuePool.createValue(data.distribution())); - if (data.distribution() == ContentDistributionDefault || data.position() != ContentPositionNormal) - result.get().append(cssValuePool.createValue(data.position())); + if (data.distribution() == ContentDistributionDefault || data.position() != ContentPositionNormal) { + bool gridEnabled = false; +#if ENABLE(CSS_GRID_LAYOUT) + gridEnabled = RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled(); +#endif + if (data.position() != ContentPositionNormal || gridEnabled) + result.get().append(cssValuePool.createValue(data.position())); + else + result.get().append(cssValuePool.createIdentifierValue(normalBehaviorValueID)); + } if ((data.position() >= ContentPositionCenter || data.distribution() != ContentDistributionDefault) && data.overflow() != OverflowAlignmentDefault) result.get().append(cssValuePool.createValue(data.overflow())); ASSERT(result.get().length() > 0); @@ -2803,7 +2812,7 @@ RefPtr ComputedStyleExtractor::propertyValue(CSSPropertyID propertyID, case CSSPropertyEmptyCells: return cssValuePool.createValue(style->emptyCells()); case CSSPropertyAlignContent: - return valueForContentPositionAndDistributionWithOverflowAlignment(style->alignContent()); + return valueForContentPositionAndDistributionWithOverflowAlignment(style->alignContent(), CSSValueStretch); case CSSPropertyAlignItems: return valueForItemPositionWithOverflowAlignment(style->alignItems()); case CSSPropertyAlignSelf: @@ -2823,7 +2832,7 @@ RefPtr ComputedStyleExtractor::propertyValue(CSSPropertyID propertyID, case CSSPropertyFlexWrap: return cssValuePool.createValue(style->flexWrap()); case CSSPropertyJustifyContent: - return valueForContentPositionAndDistributionWithOverflowAlignment(style->justifyContent()); + return valueForContentPositionAndDistributionWithOverflowAlignment(style->justifyContent(), CSSValueFlexStart); #if ENABLE(CSS_GRID_LAYOUT) case CSSPropertyJustifyItems: return valueForItemPositionWithOverflowAlignment(resolveJustifyItemsAuto(style->justifyItems(), styledNode->parentNode())); diff --git a/Source/WebCore/rendering/style/RenderStyle.h b/Source/WebCore/rendering/style/RenderStyle.h index 39fe327..87fa746 100644 --- a/Source/WebCore/rendering/style/RenderStyle.h +++ b/Source/WebCore/rendering/style/RenderStyle.h @@ -2004,7 +2004,7 @@ public: static int initialOrder() { return 0; } static StyleSelfAlignmentData initialSelfAlignment() { return StyleSelfAlignmentData(ItemPositionAuto, OverflowAlignmentDefault); } static StyleSelfAlignmentData initialDefaultAlignment() { return StyleSelfAlignmentData(isCSSGridLayoutEnabled() ? ItemPositionNormal : ItemPositionStretch, OverflowAlignmentDefault); } - static StyleContentAlignmentData initialContentAlignment() { return StyleContentAlignmentData(isCSSGridLayoutEnabled() ? ContentPositionNormal : ContentPositionFlexStart, ContentDistributionDefault, OverflowAlignmentDefault); } + static StyleContentAlignmentData initialContentAlignment() { return StyleContentAlignmentData(ContentPositionNormal, ContentDistributionDefault, OverflowAlignmentDefault); } static EFlexDirection initialFlexDirection() { return FlowRow; } static EFlexWrap initialFlexWrap() { return FlexNoWrap; } static int initialMarqueeLoopCount() { return -1; } -- 1.8.3.1