From d79ea98da2831efd0f37381c470148f9710ffba7 Mon Sep 17 00:00:00 2001 From: Hixie Date: Wed, 1 Jul 2015 12:24:45 -0700 Subject: [PATCH] Make popup menus line up to their baseline per the Material spec. This entails: - Making the baseline logic cache results. - Making the baseline logic track who used its information. - Making the baseline logic mark all ancestors up to whoever used its information wheneven its node gets markNeedsLayout. - Making RenderShrinkWrapWidth make its child respect the step width and step height, rather than just sizing the child then snapping. This is required to make the ink splashes render right on menus that are snapped. - Adding debugDescribeSettings() to RenderShrinkWrapWidth. - Introducing a RenderBaseline class that offsets its child to a certain baseline. - Factoring out some common code from RenderBaseline and RenderPositionedBox to RenderShiftedBox. - Redoing all the menu layout logic. BUG= R=abarth@chromium.org Review URL: https://codereview.chromium.org/1219113003. --- sdk/example/stocks/lib/stock_menu.dart | 4 +- sdk/lib/rendering/block.dart | 4 +- sdk/lib/rendering/box.dart | 219 +++++++++++++++++++------ sdk/lib/rendering/flex.dart | 7 +- sdk/lib/rendering/object.dart | 8 +- sdk/lib/rendering/paragraph.dart | 2 +- sdk/lib/rendering/stack.dart | 4 +- sdk/lib/widgets/basic.dart | 24 +++ sdk/lib/widgets/popup_menu.dart | 45 ++--- sdk/lib/widgets/popup_menu_item.dart | 10 +- specs/style-guide.md | 10 +- 11 files changed, 248 insertions(+), 89 deletions(-) diff --git a/sdk/example/stocks/lib/stock_menu.dart b/sdk/example/stocks/lib/stock_menu.dart index 4fb0e0d8808..9fc5721f5d5 100644 --- a/sdk/example/stocks/lib/stock_menu.dart +++ b/sdk/example/stocks/lib/stock_menu.dart @@ -36,8 +36,8 @@ class StockMenu extends Component { ], level: 4 ), - right: 8.0, - top: 8.0 + kStatusBarHeight + right: 0.0, + top: 0.0 + kStatusBarHeight ); } } diff --git a/sdk/lib/rendering/block.dart b/sdk/lib/rendering/block.dart index a143acda0c9..939e62ceaba 100644 --- a/sdk/lib/rendering/block.dart +++ b/sdk/lib/rendering/block.dart @@ -77,8 +77,8 @@ class RenderBlock extends RenderBox with ContainerRenderObjectMixin _cachedBaselines; + bool _ancestorUsesBaseline = false; + static bool _debugDoingBaseline = false; + static bool _debugSetDoingBaseline(bool value) { + _debugDoingBaseline = value; + return true; + } + // getDistanceToBaseline() returns the distance from the // y-coordinate of the position of the box to the y-coordinate of // the first given baseline in the box's contents. This is used by // certain layout models to align adjacent boxes on a common // baseline, regardless of padding, font size differences, etc. If - // there is no baseline, then it should return the distance from the + // there is no baseline, then it returns the distance from the // y-coordinate of the position of the box to the y-coordinate of - // the bottom of the box, i.e., the height of the box. - // Only call this after layout has been performed. + // the bottom of the box, i.e., the height of the box. Only call + // this after layout has been performed. You are only allowed to + // call this from the parent of this node, and only during + // that parent's performLayout(). double getDistanceToBaseline(TextBaseline baseline) { assert(!needsLayout); + assert(!_debugDoingBaseline); + final parent = this.parent; // TODO(ianh): Remove this once the analyzer is cleverer + assert(parent is RenderObject); + assert(parent == RenderObject.debugActiveLayout); + assert(parent.debugDoingThisLayout); + assert(_debugSetDoingBaseline(true)); double result = getDistanceToActualBaseline(baseline); + assert(_debugSetDoingBaseline(false)); + assert(parent == this.parent); // TODO(ianh): Remove this once the analyzer is cleverer if (result == null) return size.height; return result; } - // getDistanceToActualBaseline() should return the distance from the - // y-coordinate of the position of the box to the y-coordinate of - // the first given baseline in the box's contents, if any, or null - // otherwise. + // getDistanceToActualBaseline() must only be called from + // getDistanceToBaseline() and computeDistanceToActualBaseline(). Do + // not call it directly from outside those two methods. It just + // calls computeDistanceToActualBaseline() and caches the result. double getDistanceToActualBaseline(TextBaseline baseline) { - assert(!needsLayout); + assert(_debugDoingBaseline); + _ancestorUsesBaseline = true; + if (_cachedBaselines == null) + _cachedBaselines = new Map(); + _cachedBaselines.putIfAbsent(baseline, () => computeDistanceToActualBaseline(baseline)); + return _cachedBaselines[baseline]; + } + // computeDistanceToActualBaseline() should return the distance from + // the y-coordinate of the position of the box to the y-coordinate + // of the first given baseline in the box's contents, if any, or + // null otherwise. This is the method that you should override in + // subclasses. This method (computeDistanceToActualBaseline()) + // should not be called directly. Use getDistanceToBaseline() if you + // need to know the baseline of a child from performLayout(). If you + // need the baseline during paint, cache it during performLayout(). + // Use getDistanceToActualBaseline() if you are implementing + // computeDistanceToActualBaseline() and need to defer to a child. + double computeDistanceToActualBaseline(TextBaseline baseline) { + assert(_debugDoingBaseline); return null; } @@ -326,6 +361,26 @@ abstract class RenderBox extends RenderObject { print("${this.runtimeType} does not meet its constraints. Constraints: $constraints, size: $_size"); return result; } + + void markNeedsLayout() { + if (_cachedBaselines != null && _cachedBaselines.isNotEmpty) { + // if we have cached data, then someone must have used our data + assert(_ancestorUsesBaseline); + final parent = this.parent; // TODO(ianh): Remove this once the analyzer is cleverer + assert(parent is RenderObject); + parent.markNeedsLayout(); + assert(parent == this.parent); // TODO(ianh): Remove this once the analyzer is cleverer + // Now that they're dirty, we can forget that they used the + // baseline. If they use it again, then we'll set the bit + // again, and if we get dirty again, we'll notify them again. + _ancestorUsesBaseline = false; + _cachedBaselines.clear(); + } else { + // if we've never cached any data, then nobody can have used it + assert(!_ancestorUsesBaseline); + } + super.markNeedsLayout(); + } void performResize() { // default behaviour for subclasses that have sizedByParent = true size = constraints.constrain(Size.zero); @@ -410,10 +465,10 @@ class RenderProxyBox extends RenderBox with RenderObjectWithChildMixin '${super.debugDescribeSettings(prefix)}${prefix}stepWidth: ${stepWidth}\n${prefix}stepHeight: ${stepHeight}\n'; + } class RenderOpacity extends RenderProxyBox { @@ -719,12 +788,31 @@ abstract class RenderShiftedBox extends RenderBox with RenderObjectWithChildMixi this.child = child; } - void paint(PaintingCanvas canvas, Offset offset) { + double getMinIntrinsicWidth(BoxConstraints constraints) { if (child != null) - canvas.paintChild(child, child.parentData.position + offset); + return child.getMinIntrinsicWidth(constraints); + return super.getMinIntrinsicWidth(constraints); } - double getDistanceToActualBaseline(TextBaseline baseline) { + double getMaxIntrinsicWidth(BoxConstraints constraints) { + if (child != null) + return child.getMaxIntrinsicWidth(constraints); + return super.getMaxIntrinsicWidth(constraints); + } + + double getMinIntrinsicHeight(BoxConstraints constraints) { + if (child != null) + return child.getMinIntrinsicHeight(constraints); + return super.getMinIntrinsicHeight(constraints); + } + + double getMaxIntrinsicHeight(BoxConstraints constraints) { + if (child != null) + return child.getMaxIntrinsicHeight(constraints); + return super.getMaxIntrinsicHeight(constraints); + } + + double computeDistanceToActualBaseline(TextBaseline baseline) { double result; if (child != null) { assert(!needsLayout); @@ -733,11 +821,16 @@ abstract class RenderShiftedBox extends RenderBox with RenderObjectWithChildMixi if (result != null) result += child.parentData.position.y; } else { - result = super.getDistanceToActualBaseline(baseline); + result = super.computeDistanceToActualBaseline(baseline); } return result; } + void paint(PaintingCanvas canvas, Offset offset) { + if (child != null) + canvas.paintChild(child, child.parentData.position + offset); + } + void hitTestChildren(HitTestResult result, { Point position }) { if (child != null) { assert(child.parentData is BoxParentData); @@ -820,6 +913,12 @@ class RenderPadding extends RenderShiftedBox { class RenderPositionedBox extends RenderShiftedBox { + // This box aligns a child box within itself. It's only useful for + // children that don't always size to fit their parent. For example, + // to align a box at the bottom right, you would pass this box a + // tight constraint that is bigger than the child's natural size, + // with horizontal and vertical set to 1.0. + RenderPositionedBox({ RenderBox child, double horizontal: 0.5, @@ -851,30 +950,6 @@ class RenderPositionedBox extends RenderShiftedBox { markNeedsLayout(); } - double getMinIntrinsicWidth(BoxConstraints constraints) { - if (child != null) - return child.getMinIntrinsicWidth(constraints); - return super.getMinIntrinsicWidth(constraints); - } - - double getMaxIntrinsicWidth(BoxConstraints constraints) { - if (child != null) - return child.getMaxIntrinsicWidth(constraints); - return super.getMaxIntrinsicWidth(constraints); - } - - double getMinIntrinsicHeight(BoxConstraints constraints) { - if (child != null) - return child.getMinIntrinsicHeight(constraints); - return super.getMinIntrinsicHeight(constraints); - } - - double getMaxIntrinsicHeight(BoxConstraints constraints) { - if (child != null) - return child.getMaxIntrinsicHeight(constraints); - return super.getMaxIntrinsicHeight(constraints); - } - void performLayout() { if (child != null) { child.layout(constraints.loosen(), parentUsesSize: true); @@ -890,6 +965,54 @@ class RenderPositionedBox extends RenderShiftedBox { String debugDescribeSettings(String prefix) => '${super.debugDescribeSettings(prefix)}${prefix}horizontal: ${horizontal}\n${prefix}vertical: ${vertical}\n'; } +class RenderBaseline extends RenderShiftedBox { + + RenderBaseline({ + RenderBox child, + double baseline, + TextBaseline baselineType + }) : _baseline = baseline, + _baselineType = baselineType, + super(child) { + assert(baseline != null); + assert(baselineType != null); + } + + double _baseline; + double get baseline => _baseline; + void set baseline (double value) { + assert(value != null); + if (_baseline == value) + return; + _baseline = value; + markNeedsLayout(); + } + + TextBaseline _baselineType; + TextBaseline get baselineType => _baselineType; + void set baselineType (TextBaseline value) { + assert(value != null); + if (_baselineType == value) + return; + _baselineType = value; + markNeedsLayout(); + } + + void performLayout() { + if (child != null) { + child.layout(constraints.loosen(), parentUsesSize: true); + size = constraints.constrain(child.size); + assert(child.parentData is BoxParentData); + double delta = baseline - child.getDistanceToBaseline(baselineType); + child.parentData.position = new Point(0.0, delta); + } else { + performResize(); + } + } + + String debugDescribeSettings(String prefix) => '${super.debugDescribeSettings(prefix)}${prefix}baseline: ${baseline}\nbaselineType: ${baselineType}'; +} + class RenderImage extends RenderBox { RenderImage(String url, Size dimensions) { @@ -1255,7 +1378,7 @@ abstract class RenderBoxContainerDefaultsMixin c.getMaxIntrinsicHeight(innerConstraints)); } - double getDistanceToActualBaseline(TextBaseline baseline) { - assert(!needsLayout); + double computeDistanceToActualBaseline(TextBaseline baseline) { if (_direction == FlexDirection.horizontal) - return defaultGetDistanceToHighestActualBaseline(baseline); - return defaultGetDistanceToFirstActualBaseline(baseline); + return defaultComputeDistanceToHighestActualBaseline(baseline); + return defaultComputeDistanceToFirstActualBaseline(baseline); } int _getFlex(RenderBox child) { diff --git a/sdk/lib/rendering/object.dart b/sdk/lib/rendering/object.dart index 9624dc32ee0..b384d029deb 100644 --- a/sdk/lib/rendering/object.dart +++ b/sdk/lib/rendering/object.dart @@ -69,8 +69,8 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { assert(!debugDoingPaint); assert(child != null); assert(child.parentData != null); - child.parentData.detach(); child._cleanRelayoutSubtreeRoot(); + child.parentData.detach(); super.dropChild(child); markNeedsLayout(); } @@ -122,7 +122,6 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { assert(parent == this.parent); // TODO(ianh): Remove this once the analyzer is cleverer } else { _nodesNeedingLayout.add(this); - scheduler.ensureVisualUpdate(); } } void _cleanRelayoutSubtreeRoot() { @@ -168,11 +167,12 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { _debugDoingThisLayout = false; _debugCanParentUseSize = null; } catch (e, stack) { - print('Exception raised during layout of ${this}: ${e}'); + print('Exception raised during layout:\n${e}\nContext:\n${this}'); print(stack); return; } _needsLayout = false; + markNeedsPaint(); } void layout(Constraints constraints, { bool parentUsesSize: false }) { final parent = this.parent; // TODO(ianh): Remove this once the analyzer is cleverer @@ -217,7 +217,7 @@ abstract class RenderObject extends AbstractNode implements HitTestTarget { // // When calling layout() on your children, pass in // "parentUsesSize: true" if your size or layout is dependent on - // your child's size. + // your child's size or intrinsic dimensions. // when the parent has rotated (e.g. when the screen has been turned // 90 degrees), immediately prior to layout() being called for the diff --git a/sdk/lib/rendering/paragraph.dart b/sdk/lib/rendering/paragraph.dart index dcce468cb7b..605e0950f7b 100644 --- a/sdk/lib/rendering/paragraph.dart +++ b/sdk/lib/rendering/paragraph.dart @@ -154,7 +154,7 @@ class RenderParagraph extends RenderBox { return _getIntrinsicHeight(constraints); } - double getDistanceToActualBaseline(TextBaseline baseline) { + double computeDistanceToActualBaseline(TextBaseline baseline) { assert(!needsLayout); _layout(constraints); sky.Element root = _layoutRoot.rootElement; diff --git a/sdk/lib/rendering/stack.dart b/sdk/lib/rendering/stack.dart index 449b967e3c2..c28a59f93e9 100644 --- a/sdk/lib/rendering/stack.dart +++ b/sdk/lib/rendering/stack.dart @@ -106,8 +106,8 @@ class RenderStack extends RenderBox with ContainerRenderObjectMixin super.root; + + final double baseline; + final TextBaseline baselineType; + + RenderBaseline createNode() => new RenderBaseline(baseline: baseline, baselineType: baselineType); + + void syncRenderObject(Baseline old) { + super.syncRenderObject(old); + root.baseline = baseline; + root.baselineType = baselineType; + } + +} + class SizeObserver extends OneChildRenderObjectWrapper { SizeObserver({ String key, this.callback, Widget child }) diff --git a/sdk/lib/widgets/popup_menu.dart b/sdk/lib/widgets/popup_menu.dart index 6fdf366dc30..bf357eece39 100644 --- a/sdk/lib/widgets/popup_menu.dart +++ b/sdk/lib/widgets/popup_menu.dart @@ -18,7 +18,9 @@ const double _kMenuOpenDuration = 300.0; const double _kMenuCloseDuration = 200.0; const double _kMenuCloseDelay = 100.0; const double _kMenuWidthStep = 56.0; -const double _kMenuMinWidth = 1.5 * _kMenuWidthStep; +const double _kMenuMargin = 16.0; // 24.0 on tablet +const double _kMenuMinWidth = 2.0 * _kMenuWidthStep; +const double _kMenuMaxWidth = 5.0 * _kMenuWidthStep; const double _kMenuHorizontalPadding = 16.0; const double _kMenuVerticalPadding = 8.0; @@ -82,6 +84,7 @@ class PopupMenu extends AnimatedComponent { BoxPainter _painter; double _opacityFor(int i) { + assert(controller.position.value != null); if (controller.position.value == null || controller.position.value == 1.0) return 1.0; double unit = 1.0 / items.length; @@ -99,24 +102,28 @@ class PopupMenu extends AnimatedComponent { return new Opacity( opacity: math.min(1.0, controller.position.value * 3.0), - child: new CustomPaint( - callback: (sky.Canvas canvas, Size size) { - double width = math.min(size.width, size.width * (0.5 + controller.position.value * 2.0)); - double height = math.min(size.height, size.height * controller.position.value * 1.5); - _painter.paint(canvas, new Rect.fromLTRB(size.width - width, 0.0, width, height)); - }, - child: new ConstrainedBox( - constraints: new BoxConstraints( - minWidth: _kMenuMinWidth - ), - child: new ShrinkWrapWidth( - stepWidth: _kMenuWidthStep, - child: new Container( - padding: const EdgeDims.symmetric( - horizontal: _kMenuHorizontalPadding, - vertical: _kMenuVerticalPadding - ), - child: new Block(children) + child: new Container( + margin: new EdgeDims.all(_kMenuMargin), + child: new CustomPaint( + callback: (sky.Canvas canvas, Size size) { + double width = math.min(size.width, size.width * (0.5 + controller.position.value * 2.0)); + double height = math.min(size.height, size.height * controller.position.value * 1.5); + _painter.paint(canvas, new Rect.fromLTRB(size.width - width, 0.0, width, height)); + }, + child: new ConstrainedBox( + constraints: new BoxConstraints( + minWidth: _kMenuMinWidth, + maxWidth: _kMenuMaxWidth + ), + child: new ShrinkWrapWidth( + stepWidth: _kMenuWidthStep, + child: new Container( + padding: const EdgeDims.symmetric( + horizontal: _kMenuHorizontalPadding, + vertical: _kMenuVerticalPadding + ), + child: new Block(children) + ) ) ) ) diff --git a/sdk/lib/widgets/popup_menu_item.dart b/sdk/lib/widgets/popup_menu_item.dart index 17147cd10a8..c87dc0d5ca9 100644 --- a/sdk/lib/widgets/popup_menu_item.dart +++ b/sdk/lib/widgets/popup_menu_item.dart @@ -8,6 +8,9 @@ import 'default_text_style.dart'; import 'ink_well.dart'; import 'theme.dart'; +const double kMenuItemHeight = 48.0; +const double kBaselineOffsetFromBottom = 20.0; + class PopupMenuItem extends Component { PopupMenuItem({ String key, this.child, this.opacity}) : super(key: key); @@ -21,10 +24,13 @@ class PopupMenuItem extends Component { opacity: opacity, child: new InkWell( child: new Container( - height: 48.0, + height: kMenuItemHeight, child: new DefaultTextStyle( style: textStyle, - child: child + child: new Baseline( + baseline: kMenuItemHeight - kBaselineOffsetFromBottom, + child: child + ) ) ) ) diff --git a/specs/style-guide.md b/specs/style-guide.md index 123acd453eb..e475fa810ab 100644 --- a/specs/style-guide.md +++ b/specs/style-guide.md @@ -87,16 +87,16 @@ the expression, even if it is short. (Doing so makes it unobvious that there is relevant code there. This is especially important for early returns.) -If a flow control structure has just one statement, then don't use -braces around it except where doing so would help readability or avoid -the dangling-else problem. Keeping the code free of boilerplate or -redundant punctuation keeps it concise and readable. +If a flow control structure's statement is one line long, then don't +use braces around it. (Keeping the code free of boilerplate or +redundant punctuation keeps it concise and readable.) > For example, > ```dart -> if (children != null) +> if (children != null) { > for (RenderBox child in children) > add(child); +> } > ``` > ...rather than: > ```dart