ButtonBar aligns in column when it overflows horizontally #43193

shihaohong · 2019-10-21T19:10:52Z

Description

When AlertDialogs are used with buttons that are too wide, they simply overflow through the dialog, which is a possible situation when the text scale factor is bumped up to account for accessibility needs. This change provides ButtonBar provides behavior for laying out in a column in the event that the desired width of the ButtonBar exceeds the maximum width constraints it is provided.

Original behavior:

Overflow behavior, when text scale factor is increased:

In this implementation, when the buttons overflow, MainAxisAlignment is still treated as horizontal alignment instead of flipping it into vertical alignment. This makes sense in the button bar case since it's more likely that developers care about how buttons align horizontally over having the buttons align towards the vertical bottom of widgets that use ButtonBar, like AlertDialog. This means that the spaceAround, spaceBetween and spaceEvenly default to start's layout algorithm.

MainAxisAlignment.center:

MainAxisAlignment.start:

Related Issues

Fixes part of #42696

Tests

I added the following tests:

Tests verifying that the ButtonBar correctly aligns in a column
Tests verifying that the ButtonBar uses MainAxisAlignment for horizontal alignment in the overflow case.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

… wrap around case

…ender-flex

darrenaustin

Main concern for me here is the public interfaces, but otherwise looks good.

darrenaustin · 2019-10-21T19:30:11Z

packages/flutter/lib/src/material/button_bar.dart

+class ButtonBarRow extends Flex {
+  /// Creates a button bar that attempts to display in a row, but displays in
+  /// a column if there is insufficient horizontal space.
+  ButtonBarRow({


Do we really want this as a public class? Do we envision apps using this directly? Seems like this is just an implementation detail. It might be confusing to have both ButtonBarandButtonBarRow` as part of the public API.

Good point. I made this private. We can always make this public if the need arises

darrenaustin · 2019-10-21T19:34:20Z

packages/flutter/lib/src/material/button_bar.dart

+/// cross-axis/horizontal alignment. For example, if the buttons overflow and
+/// [ButtonBar.alignment] was set to [MainAxisAligment.start], the buttons would
+/// align to the horizontal start of the button bar.
+class RenderButtonBarRow extends RenderFlex {


Do we want/need this to be public?

I made this class private as well

darrenaustin · 2019-10-21T19:51:45Z

packages/flutter/lib/src/material/button_bar.dart

+          case TextDirection.ltr:
+            switch (mainAxisAlignment) {
+              case MainAxisAlignment.center:
+                final double midpoint = constraints.maxWidth / 2.0 - child.size.width / 2.0;


Really minor style thing, but it might be better to express this as:

(constraints.maxWidth - child.size.width) / 2.0

As it is probably less likely to loose precision with one division instead of two. Might be faster too.

darrenaustin · 2019-10-21T19:54:01Z

packages/flutter/test/material/button_bar_test.dart

@@ -95,7 +95,7 @@ void main() {
      );

      // ButtonBar uses a Row internally to implement this


It now uses a ButtonBarRow instead of a Row now. :)

I was just trying to write tests for ButtonBarRow and MainAxisAlignment and cannot seem to find how this parameter is used. As far as I understand, it seems to only be valuable when Flexible children are present, but since each button is wrapped in a Padding widget, there is no way to make a child of ButtonBar flexible. I tried playing around with the param in a small app and see no difference between setting it to min and max.

Deleting the parameter would be a breaking change for apps that define this param, so maybe we could delete the tests for now, and then in a follow-up PR, try removing the parameter and see if anything breaks?

Nevermind - I misunderstood how MainAxisSize works. It turns out that this basically tells its own parent how much space it intends to take. However, this just means that if ButtonBar.mainAxisSize was set to MainAxisSize.min, it sort of ignores alignment since there's minimal space and is aligned based on how the parent itself chooses to align itself.

HansMuller

LGTM, with a few cleanups.

HansMuller · 2019-10-21T23:53:17Z

packages/flutter/lib/src/material/button_bar.dart

+/// widget, it aligns its buttons in a column. The key difference here
+/// is that the [MainAxisAlignment] will then be treated as a
+/// cross-axis/horizontal alignment. For example, if the buttons overflow and
+/// [ButtonBar.alignment] was set to [MainAxisAligment.start], the buttons would


the buttons would => the column of buttons would

HansMuller · 2019-10-21T23:54:05Z

packages/flutter/lib/src/material/button_bar.dart

+/// the widget, it then aligns its buttons in a column. The key difference here
+/// is that the [MainAxisAlignment] will then be treated as a
+/// cross-axis/horizontal alignment. For example, if the buttons overflow and
+/// [ButtonBar.alignment] was set to [MainAxisAligment.start], the buttons would


the buttons => the column of buttons

HansMuller · 2019-10-22T00:01:31Z

packages/flutter/lib/src/material/button_bar.dart

+        child = childParentData.nextSibling;
+      }
+      size = constraints.constrain(Size(constraints.maxWidth, currentHeight));
+    } else {


Is it really necessary to performLayout() again, if the row fit? If so, this would be a good place to explain why.

It might be clearer if this clause was first.

It is, since the original performLayout() call uses infinity as the maximum width constraint. The second layout is performed to ensure that we use the original maximum width. I'll add a comment to explain this and have it as the first clause.

HansMuller · 2019-10-22T00:03:04Z

packages/flutter/lib/src/material/button_bar.dart

+
+        // Lay out the child with the button bar's original constraints, but
+        // with minimum width set to zero.
+        child.layout(constraints.copyWith(minWidth: 0.0), parentUsesSize: true);


Could just bind childConstraints to a final var outside of the loop.

HansMuller · 2019-10-22T00:03:56Z

packages/flutter/lib/src/material/button_bar.dart

+      double currentHeight = 0.0;
+      while (child != null) {
+        final FlexParentData childParentData = child.parentData;
+        assert(child.parentData == childParentData);


Presumably, this assert has already been applied by super.performLayout() or somewhere else in RenderFlex or its superclasses.

I believe it has already. I'll remove it from here then

HansMuller · 2019-10-22T00:05:56Z

packages/flutter/lib/src/material/button_bar.dart

+                break;
+            }
+            break;
+          default:


Could move this assert to the constructor. We don't really expect it to be null, even if there's no overflow.

) * Custom layout for ButtonBar column when it overflows horizontally

shihaohong added 9 commits October 18, 2019 11:09

WIP - Attempt at ButtonBarRow implementation

0ff9d4a

Custom layout for ButtonBar column

957d7b6

Fix constraints bugs and store local constraints copy

45a4e23

Fix horizontal alignment bugs

70ee242

Account for ltr/rtl and implement MainAxisAlignment layouts in column…

06e2c51

… wrap around case

Add documentation to ButtonBarRow and RenderButtonBarRow

1f54be7

Fix default MainAxisAlignment value

eed8181

Add ButtonBar overflow tests

347e461

Merge branch 'master' of github.com:flutter/flutter into button-bar-r…

be5ee3e

…ender-flex

shihaohong added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) labels Oct 21, 2019

shihaohong requested review from darrenaustin and HansMuller October 21, 2019 19:10

googlebot added the cla: yes label Oct 21, 2019

shihaohong mentioned this pull request Oct 21, 2019

Implement ButtonBar.isWrapped and AlertDialog.areActionsWrapped #42870

Closed

9 tasks

Set button minimum width constraints to zero, fix tests

732c8a4

darrenaustin reviewed Oct 21, 2019

View reviewed changes

shihaohong added 2 commits October 21, 2019 16:42

ButtonBar feedback

9ec1a77

Remove unnecessary import

07d06b4

HansMuller approved these changes Oct 22, 2019

View reviewed changes

Incorporate code review feedback

8fda5bd

shihaohong merged commit 56f9c95 into flutter:master Oct 22, 2019

shihaohong deleted the button-bar-render-flex branch October 22, 2019 19:12

Inconnu08 pushed a commit to Inconnu08/flutter that referenced this pull request Nov 26, 2019

ButtonBar aligns in column when it overflows horizontally (flutter#43193

f55ae74

) * Custom layout for ButtonBar column when it overflows horizontally

github-actions bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ButtonBar aligns in column when it overflows horizontally #43193

ButtonBar aligns in column when it overflows horizontally #43193

shihaohong commented Oct 21, 2019 •

edited

darrenaustin left a comment

darrenaustin Oct 21, 2019

shihaohong Oct 21, 2019

darrenaustin Oct 21, 2019

shihaohong Oct 21, 2019

darrenaustin Oct 21, 2019

darrenaustin Oct 21, 2019

shihaohong Oct 21, 2019

shihaohong Oct 21, 2019

HansMuller left a comment

HansMuller Oct 21, 2019

HansMuller Oct 21, 2019

HansMuller Oct 22, 2019

shihaohong Oct 22, 2019

HansMuller Oct 22, 2019

HansMuller Oct 22, 2019

shihaohong Oct 22, 2019

HansMuller Oct 22, 2019

		@@ -95,7 +95,7 @@ void main() {
		);

		// ButtonBar uses a Row internally to implement this

ButtonBar aligns in column when it overflows horizontally #43193

ButtonBar aligns in column when it overflows horizontally #43193

Conversation

shihaohong commented Oct 21, 2019 • edited

Description

Original behavior:

Overflow behavior, when text scale factor is increased:

Related Issues

Tests

Checklist

Breaking Change

darrenaustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HansMuller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shihaohong commented Oct 21, 2019 •

edited