From 88f38ce0e6fc299fc0675dcb787ba712bc423990 Mon Sep 17 00:00:00 2001 From: Ojan Vafai Date: Mon, 6 Apr 2015 16:44:12 -0700 Subject: [PATCH] Remove all uses of display:block and display:inline-block. -Make display:flex, flex-direction: column, flex-shrink: 1 the default. -Simplify StyleAdjuster::adjustStyleForAlignment to remove special cases we won't need as we make flex the default and remove absolute positioning. -Fix a bug this exposed in column flexboxes where we'd apply the wrong edge of border/padding/margin. -For now leave the default of align-items:stretch. The main change here is that iframe/img will do width:auto the same as blocks (i.e. the width of the parent). I think this is a good change, but we'll have to see how it feels in practice. R=eseidel@chromium.org Review URL: https://codereview.chromium.org/1061163002 --- engine/core/css/parser/CSSPropertyParser.cpp | 1 + engine/core/css/resolver/StyleAdjuster.cpp | 46 +++---------------- engine/core/rendering/RenderFlexibleBox.cpp | 15 ++---- engine/core/rendering/style/RenderStyle.h | 6 +-- examples/stocks/lib/stock_arrow.dart | 1 + examples/stocks/lib/stock_row.dart | 1 + framework/components/action_bar.dart | 1 + framework/components/checkbox.dart | 1 + .../components/floating_action_button.dart | 1 + framework/components/menu_item.dart | 2 + framework/components/radio.dart | 4 +- framework/editing/editable_text.dart | 2 +- framework/elements/sky-checkbox.sky | 2 +- framework/elements/sky-input.sky | 1 + framework/elements/sky-menu-item.sky | 2 + framework/elements/sky-radio.sky | 2 +- framework/elements/sky-toolbar.sky | 1 + ...ound-opaque-clipped-gradients-expected.txt | 4 +- tests/editing/backspace.sky | 1 + tests/editing/replace.sky | 1 + tests/editing/selection.sky | 1 + tests/editing/typing.sky | 1 + tests/layout/continuations-expected.txt | 6 +-- ...ent-elementFromPoint-absolute-position.sky | 2 +- tests/layout/document-elementFromPoint.sky | 10 ++-- tests/layout/ellipsis-expected.txt | 4 +- tests/layout/fixed-width-expected.txt | 4 +- tests/layout/position-absolute-expected.txt | 20 ++++---- tests/layout/position-absolute-pixels.sky | 1 + tests/layout/position-absolute.sky | 1 + tests/lowlevel/abarth-expected.txt | 14 +++--- tests/lowlevel/hello-world-expected.txt | 2 +- tests/lowlevel/home-expected.txt | 6 +-- tests/lowlevel/iframe-expected.txt | 16 +++---- tests/lowlevel/img-expected.txt | 4 +- 35 files changed, 83 insertions(+), 104 deletions(-) diff --git a/engine/core/css/parser/CSSPropertyParser.cpp b/engine/core/css/parser/CSSPropertyParser.cpp index 4beb299fc4f..2361012e06e 100644 --- a/engine/core/css/parser/CSSPropertyParser.cpp +++ b/engine/core/css/parser/CSSPropertyParser.cpp @@ -4257,6 +4257,7 @@ static bool isFlexBasisMiddleArg(double flexGrow, double flexShrink, double unse return flexGrow != unsetValue && flexShrink == unsetValue && argSize == 3; } +// TODO(ojan): Make this have reasonable defaults. bool CSSPropertyParser::parseFlex(CSSParserValueList* args) { if (!args || !args->size() || args->size() > 3) diff --git a/engine/core/css/resolver/StyleAdjuster.cpp b/engine/core/css/resolver/StyleAdjuster.cpp index 8e0110d5837..8788d7450e1 100644 --- a/engine/core/css/resolver/StyleAdjuster.cpp +++ b/engine/core/css/resolver/StyleAdjuster.cpp @@ -184,53 +184,19 @@ void StyleAdjuster::adjustRenderStyle(RenderStyle* style, RenderStyle* parentSty void StyleAdjuster::adjustStyleForAlignment(RenderStyle& style, const RenderStyle& parentStyle) { - bool isFlex = style.isDisplayFlexibleBox(); - bool absolutePositioned = style.position() == AbsolutePosition; - - // If the inherited value of justify-items includes the legacy keyword, 'auto' - // computes to the the inherited value. - // Otherwise, auto computes to: - // - 'stretch' for flex containers. - // - 'start' for everything else. if (style.justifyItems() == ItemPositionAuto) { - if (parentStyle.justifyItemsPositionType() == LegacyPosition) { - style.setJustifyItems(parentStyle.justifyItems()); - style.setJustifyItemsPositionType(parentStyle.justifyItemsPositionType()); - } else if (isFlex) { - style.setJustifyItems(ItemPositionStretch); - } + style.setJustifyItems(parentStyle.justifyItems()); + style.setJustifyItemsPositionType(parentStyle.justifyItemsPositionType()); } - // The 'auto' keyword computes to 'stretch' on absolutely-positioned elements, - // and to the computed value of justify-items on the parent (minus - // any legacy keywords) on all other boxes. if (style.justifySelf() == ItemPositionAuto) { - if (absolutePositioned) { - style.setJustifySelf(ItemPositionStretch); - } else { - style.setJustifySelf(parentStyle.justifyItems()); - style.setJustifySelfOverflowAlignment(parentStyle.justifyItemsOverflowAlignment()); - } + style.setJustifySelf(parentStyle.justifyItems()); + style.setJustifySelfOverflowAlignment(parentStyle.justifyItemsOverflowAlignment()); } - // The 'auto' keyword computes to: - // - 'stretch' for flex containers, - // - 'start' for everything else. - if (style.alignItems() == ItemPositionAuto) { - if (isFlex) - style.setAlignItems(ItemPositionStretch); - } - - // 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. if (style.alignSelf() == ItemPositionAuto) { - if (absolutePositioned) { - style.setAlignSelf(ItemPositionStretch); - } else { - style.setAlignSelf(parentStyle.alignItems()); - style.setAlignSelfOverflowAlignment(parentStyle.alignItemsOverflowAlignment()); - } + style.setAlignSelf(parentStyle.alignItems()); + style.setAlignSelfOverflowAlignment(parentStyle.alignItemsOverflowAlignment()); } } diff --git a/engine/core/rendering/RenderFlexibleBox.cpp b/engine/core/rendering/RenderFlexibleBox.cpp index afd8a76ac45..d98680541ca 100644 --- a/engine/core/rendering/RenderFlexibleBox.cpp +++ b/engine/core/rendering/RenderFlexibleBox.cpp @@ -418,14 +418,12 @@ LayoutUnit RenderFlexibleBox::flowAwareBorderEnd() const LayoutUnit RenderFlexibleBox::flowAwareBorderBefore() const { - // FIXME(sky): Remove - return borderTop(); + return isHorizontalFlow() ? borderTop() : borderLeft(); } LayoutUnit RenderFlexibleBox::flowAwareBorderAfter() const { - // FIXME(sky): Remove - return borderBottom(); + return isHorizontalFlow() ? borderBottom() : borderRight(); } LayoutUnit RenderFlexibleBox::flowAwarePaddingStart() const @@ -444,14 +442,12 @@ LayoutUnit RenderFlexibleBox::flowAwarePaddingEnd() const LayoutUnit RenderFlexibleBox::flowAwarePaddingBefore() const { - // FIXME(sky): Remove - return paddingTop(); + return isHorizontalFlow() ? paddingTop() : paddingLeft(); } LayoutUnit RenderFlexibleBox::flowAwarePaddingAfter() const { - // FIXME(sky): Remove - return paddingBottom(); + return isHorizontalFlow() ? paddingBottom() : paddingRight(); } LayoutUnit RenderFlexibleBox::flowAwareMarginStartForChild(RenderBox* child) const @@ -470,8 +466,7 @@ LayoutUnit RenderFlexibleBox::flowAwareMarginEndForChild(RenderBox* child) const LayoutUnit RenderFlexibleBox::flowAwareMarginBeforeForChild(RenderBox* child) const { - // FIXME(sky): Remove - return child->marginTop(); + return isHorizontalFlow() ? child->marginTop() : child->marginLeft(); } LayoutUnit RenderFlexibleBox::crossAxisMarginExtentForChild(RenderBox* child) const diff --git a/engine/core/rendering/style/RenderStyle.h b/engine/core/rendering/style/RenderStyle.h index 2c6c3621dfa..afcfee88528 100644 --- a/engine/core/rendering/style/RenderStyle.h +++ b/engine/core/rendering/style/RenderStyle.h @@ -1111,7 +1111,7 @@ public: static TextOrientation initialTextOrientation() { return TextOrientationVerticalRight; } static ObjectFit initialObjectFit() { return ObjectFitFill; } static LengthPoint initialObjectPosition() { return LengthPoint(Length(50.0, Percent), Length(50.0, Percent)); } - static EDisplay initialDisplay() { return BLOCK; } + static EDisplay initialDisplay() { return FLEX; } static EEmptyCell initialEmptyCells() { return SHOW; } static EListStylePosition initialListStylePosition() { return OUTSIDE; } static EListStyleType initialListStyleType() { return Disc; } @@ -1164,7 +1164,7 @@ public: static unsigned initialBoxOrdinalGroup() { return 1; } static EBoxSizing initialBoxSizing() { return CONTENT_BOX; } static float initialFlexGrow() { return 0; } - static float initialFlexShrink() { return 1; } + static float initialFlexShrink() { return 0; } static Length initialFlexBasis() { return Length(Auto); } static int initialOrder() { return 0; } static EAlignContent initialAlignContent() { return AlignContentStretch; } @@ -1172,7 +1172,7 @@ public: static OverflowAlignment initialAlignItemsOverflowAlignment() { return OverflowAlignmentDefault; } static ItemPosition initialAlignSelf() { return ItemPositionAuto; } static OverflowAlignment initialAlignSelfOverflowAlignment() { return OverflowAlignmentDefault; } - static EFlexDirection initialFlexDirection() { return FlowRow; } + static EFlexDirection initialFlexDirection() { return FlowColumn; } static EFlexWrap initialFlexWrap() { return FlexNoWrap; } static EJustifyContent initialJustifyContent() { return JustifyFlexStart; } static ItemPosition initialJustifyItems() { return ItemPositionAuto; } diff --git a/examples/stocks/lib/stock_arrow.dart b/examples/stocks/lib/stock_arrow.dart index 9ca249a4a9e..78a4b66ee7d 100644 --- a/examples/stocks/lib/stock_arrow.dart +++ b/examples/stocks/lib/stock_arrow.dart @@ -10,6 +10,7 @@ class StockArrow extends Component { width: 40px; height: 40px; display: flex; + flex-direction: row; align-items: center; justify-content: center; border-radius: 40px; diff --git a/examples/stocks/lib/stock_row.dart b/examples/stocks/lib/stock_row.dart index 235183b3c67..1b4578aff00 100644 --- a/examples/stocks/lib/stock_row.dart +++ b/examples/stocks/lib/stock_row.dart @@ -11,6 +11,7 @@ import 'stock_data.dart'; class StockRow extends Component { static final Style _style = new Style(''' display: flex; + flex-direction: row; align-items: center; border-bottom: 1px solid #F4F4F4; padding-top: 16px; diff --git a/framework/components/action_bar.dart b/framework/components/action_bar.dart index 069e7031ae6..0e34fbaeefc 100644 --- a/framework/components/action_bar.dart +++ b/framework/components/action_bar.dart @@ -9,6 +9,7 @@ import 'material.dart'; class ActionBar extends Component { static final Style _style = new Style(''' display: flex; + flex-direction: row; align-items: center; height: 56px; padding: 0 8px; diff --git a/framework/components/checkbox.dart b/framework/components/checkbox.dart index 0708e749b9b..67d451d60e7 100644 --- a/framework/components/checkbox.dart +++ b/framework/components/checkbox.dart @@ -11,6 +11,7 @@ class Checkbox extends ButtonBase { static final Style _style = new Style(''' transform: translateX(0); display: flex; + flex-direction: row; justify-content: center; align-items: center; -webkit-user-select: none; diff --git a/framework/components/floating_action_button.dart b/framework/components/floating_action_button.dart index 6cfe773fd07..65dae6b82ca 100644 --- a/framework/components/floating_action_button.dart +++ b/framework/components/floating_action_button.dart @@ -21,6 +21,7 @@ class FloatingActionButton extends Component { transform: translateX(0); position: absolute; display: flex; + flex-direction: row; justify-content: center; align-items: center; top: 0; diff --git a/framework/components/menu_item.dart b/framework/components/menu_item.dart index f9fbec78855..1539651c2be 100644 --- a/framework/components/menu_item.dart +++ b/framework/components/menu_item.dart @@ -11,6 +11,7 @@ class MenuItem extends ButtonBase { static final Style _style = new Style(''' transform: translateX(0); display: flex; + flex-direction: row; align-items: center; height: 48px; -webkit-user-select: none;''' @@ -19,6 +20,7 @@ class MenuItem extends ButtonBase { static final Style _highlightStyle = new Style(''' transform: translateX(0); display: flex; + flex-direction: row; align-items: center; height: 48px; background: rgba(153, 153, 153, 0.4); diff --git a/framework/components/radio.dart b/framework/components/radio.dart index e5024e1ef5e..e812034467d 100644 --- a/framework/components/radio.dart +++ b/framework/components/radio.dart @@ -16,7 +16,7 @@ class Radio extends ButtonBase { static final Style _style = new Style(''' transform: translateX(0); - display: inline-block; + display: inline-flex; -webkit-user-select: none; width: 14px; height: 14px; @@ -27,7 +27,7 @@ class Radio extends ButtonBase { static final Style _highlightStyle = new Style(''' transform: translateX(0); - display: inline-block; + display: inline-flex; -webkit-user-select: none; width: 14px; height: 14px; diff --git a/framework/editing/editable_text.dart b/framework/editing/editable_text.dart index ae493e26538..384c1b45f61 100644 --- a/framework/editing/editable_text.dart +++ b/framework/editing/editable_text.dart @@ -13,7 +13,7 @@ class EditableText extends Component { ); static final Style _cusorStyle = new Style(''' - display: inline-block; + display: inline-flex; width: 2px; height: 1.2em; vertical-align: top; diff --git a/framework/elements/sky-checkbox.sky b/framework/elements/sky-checkbox.sky index 623dcc08173..4d52d95494f 100644 --- a/framework/elements/sky-checkbox.sky +++ b/framework/elements/sky-checkbox.sky @@ -10,7 +10,7 @@