Fix SnackBar clipping when it is floating due to FloatingActionButton positioning #47616

filaps · 2019-12-22T14:57:31Z

Description

Screenshots

Before fix

After fix

Bug cause

Scaffold always wraps a FloatingActionButton(FAB) in a private widget _FloatingActionButtonTransition, even if the FAB is null (to animate the appearance of the FAB).
Thus, Scaffold always has a FAB and calculates an area for it. Before calculating the position of the SnackBar, Scaffold verifies the area of FAB on null. If the area is not null, then SnackBar will be positioned above it. Since FAB and its area is never null, SnackBar is always on top (This bug works only for SnackBar with floating behavior)

Solution

If the FAB is null, then FloatingActionButtonArea has a size equal to Size.zero. Consequently, if the size of FloatingActionButtonArea is zero, then FAB won't show and SnackBar will be laid out on top of the other content.

Tests

This PR also adds tests to ensure that the SnackBar is positioned in the right places given the many configurations of Scaffold

Related Issues

Fixes #47202
Fixes #43716

Tests

This PR include parameterized test (to avoid two identical tests differing in one detail). But if it is not ok, i'll split them.

I added the following tests:

Snackbar Position (group test)
- behavior is fixed and scaffold has no other elements
- behavior is floating and scaffold has no other elements
- behavior is fixed and scaffold has bottom navigation bar
- behavior is floating and scaffold has bottom navigation bar
- behavior is fixed and scaffold has floating action button
- behavior is floating and scaffold has floating action button
- behavior is fixed and scaffold has FAB and bottom navigation bar
- behavior is floating and scaffold has FAB and bottom navigation bar

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 read the [Tree Hygiene] wiki page, which explains my responsibilities.
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

Did any tests fail when you ran them? Please read [Handling breaking changes].

No, no existing tests failed, so this is not a breaking change.

shihaohong

Thank you for the submission! The implementation and the tests look great functionally. I think the most work that'll need to be done following up is that the code can be better formatted such that future readers of the code can easily understand what the intent behind the tests are. I made some higher level comments, but take a look at the Flutter style guide when you're unsure on formatting.

shihaohong · 2020-01-03T22:31:31Z

packages/flutter/test/material/scaffold_test.dart

I do not want to sound too particular, but we use American English in flutter/flutter. There are some mistakes/accidents that happen elsewhere in the codebase, but they weren't intentional :)

Suggested change

String getEnumStringValue(SnackBarBehavior behaviour) => behaviour.toString().split('.').last;

String getEnumStringValue(SnackBarBehavior behavior) => behavior.toString().split('.').last;

i removed this function :)

shihaohong · 2020-01-03T22:31:47Z

packages/flutter/test/material/scaffold_test.dart

I believe that we will be adding a lint soon that requires all usages of for (foo in bar) to have the final keyword in it.

Suggested change

for (SnackBarBehavior behavior in SnackBarBehavior.values) {

for (final SnackBarBehavior behavior in SnackBarBehavior.values) {

shihaohong · 2020-01-03T22:49:49Z

packages/flutter/test/material/scaffold_test.dart

I just tested printing ${SnackBarBehavior.fixed.toString()} and it seems to turn out just fine. Also, describe the expected result for the test in the title. Maybe something like the following:

Suggested change

testWidgets('behavior is ${getEnumStringValue(behavior)} and scaffold has no other elements',

testWidgets('$behavior should align with the bottom of the Scaffold when the Scaffold has no other elements',

There is one difference in the result string:
for $behavior the result is "SnackBarBehavior.fixed";
for getEnumStringValue(behavior) the result is "fixed".
But now i think this difference is not important. Therefore i will remove the function getEnumStringValue and will use $behavior instead.

I have added expected results in titles. Is it normal that new test titles have two lines?

I think that it's ok -- I find that it is difficult to come up with a title that fits in one line that is short enough to be within the 80-character guideline. Plus, I would not have to give up clarity in explaining my test this way

shihaohong · 2020-01-03T22:51:25Z

packages/flutter/test/material/scaffold_test.dart

Please fix the indentations on these lines. Every new line generally has two spaces. ie.

testWidgets( 'behavior is ${getEnumStringValue(behavior)} and scaffold has no other elements', (WidgetTester tester) async { //...

shihaohong · 2020-01-03T22:54:11Z

packages/flutter/test/material/scaffold_test.dart

Wouldn't hurt to test that the bottom left corners of the SnackBar and Scaffold align

I have added a conditions for the left and right corner of snackBar in all new tests except tests with FAB and SnackBarBehavior.floating.

shihaohong · 2020-01-03T23:00:26Z

packages/flutter/test/material/scaffold_test.dart

Suggested change

floatingActionButton: FloatingActionButton(onPressed: () {},),

floatingActionButton: FloatingActionButton(onPressed: () {}),

shihaohong · 2020-01-03T23:00:38Z

packages/flutter/test/material/scaffold_test.dart

Suggested change

scaffoldState.showSnackBar(const SnackBar(content: Text('Snackbar text'), behavior: SnackBarBehavior.floating,));

scaffoldState.showSnackBar(

const SnackBar(

content: Text('Snackbar text'),

behavior: SnackBarBehavior.floating,

),

);

shihaohong · 2020-01-03T23:01:19Z

packages/flutter/test/material/scaffold_test.dart

Same comments as above regarding the title of the test and code indentation/style.

shihaohong · 2020-01-03T23:01:22Z

packages/flutter/test/material/scaffold_test.dart

Same comments as above regarding the title of the test and code indentation/style.

shihaohong · 2020-01-03T23:02:05Z

packages/flutter/test/material/scaffold_test.dart

Suggested change

scaffoldState.showSnackBar(const SnackBar(content: Text('Snackbar text'), behavior: SnackBarBehavior.floating,));

scaffoldState.showSnackBar(

const SnackBar(

content: Text('Snackbar text'),

behavior: SnackBarBehavior.floating,

),

);

shihaohong · 2020-01-03T23:08:33Z

I hope you don't mind, but I also edited your first comment to use GitHub markdown and clarify the title of the pull request if that's okay with you!

shihaohong

I just noticed that there is a test/material/snack_bar_test.dart file, and that it does include some positioning tests. You might want to take a look at the existing tests to ensure that you're not duplicating some tests and maybe look to see why the existing tests are not catching the clipping issue this PR fixes.

Also, the tests you've written might actually belong in snack_bar_test.dart as well :)

Sorry for not noticing this sooner! Let me know if you need any suggestions or help with this PR

shihaohong · 2020-01-07T16:48:42Z

packages/flutter/test/material/scaffold_test.dart

nit

Suggested change

'when scaffold has no other elements',

'when Scaffold has no other elements',

shihaohong · 2020-01-07T16:49:03Z

packages/flutter/test/material/scaffold_test.dart

nit

Suggested change

});

},

);

Fixed in all new tests

shihaohong · 2020-01-07T16:50:12Z

packages/flutter/test/material/scaffold_test.dart

nit: trailing comma

Suggested change

body: Container()

body: Container(),

shihaohong · 2020-01-07T16:50:51Z

packages/flutter/test/material/scaffold_test.dart

Suggested change

});

},

);

shihaohong · 2020-01-07T16:52:14Z

packages/flutter/test/material/scaffold_test.dart

Suggested change

'$behavior should align snackBar with the bottom of Scaffold '

'$behavior should align SnackBar with the bottom of Scaffold '

shihaohong · 2020-01-07T16:52:29Z

packages/flutter/test/material/scaffold_test.dart

Suggested change

'$behavior should align snackBar with the top of bottom navigation bar '

'$behavior should align SnackBar with the top of bottom navigation bar '

shihaohong · 2020-01-07T16:52:56Z

packages/flutter/test/material/scaffold_test.dart

Suggested change

'${SnackBarBehavior.fixed} should align snackBar with the bottom of Scaffold '

'${SnackBarBehavior.fixed} should align SnackBar with the bottom of Scaffold '

shihaohong · 2020-01-07T16:53:05Z

packages/flutter/test/material/scaffold_test.dart

Suggested change

'${SnackBarBehavior.floating} should align snackBar with the top of FloatingActionButton'

'${SnackBarBehavior.floating} should align SnackBar with the top of FloatingActionButton'

shihaohong · 2020-01-07T16:53:13Z

packages/flutter/test/material/scaffold_test.dart

Suggested change

'${SnackBarBehavior.fixed} should align snackBar with the top of BottomNavigationBar '

'${SnackBarBehavior.fixed} should align SnackBar with the top of BottomNavigationBar '

shihaohong · 2020-01-07T16:53:21Z

packages/flutter/test/material/scaffold_test.dart

Suggested change

'${SnackBarBehavior.floating} should align snackBar with the top of FloatingActionButton '

'${SnackBarBehavior.floating} should align SnackBar with the top of FloatingActionButton '

filaps · 2020-01-08T18:44:21Z

I just noticed that there is a test/material/snack_bar_test.dart file, and that it does include some positioning tests. You might want to take a look at the existing tests to ensure that you're not duplicating some tests and maybe look to see why the existing tests are not catching the clipping issue this PR fixes.

Also, the tests you've written might actually belong in snack_bar_test.dart as well :)

Sorry for not noticing this sooner! Let me know if you need any suggestions or help with this PR

Thank you for this notice.
I did not look snak_bar_test.dart carefully.
I was sure that testing the position of the elements is the responsibility of the scaffold, and so I thought that the tests should be in scaffold_test.dart. :)

Now i analyzed snackbar_test.dart:

I found 4 tests with SnackBar position which look like my tests:

SnackBar is positioned above BottomNavigationBar
SnackBar should push FloatingActionButton above
Floating SnackBar is positioned above BottomNavigationBar
Floating SnackBar is positioned above FloatingActionButton

Test 1:
This test has a very complicated "expected" part:


    expect(textBottomLeft.dx - snackBarBottomLeft.dx, 24.0 + 10.0); // margin + left padding
    expect(snackBarBottomLeft.dy - textBottomLeft.dy, 17.0); // margin (with no bottom padding)
    expect(actionTextBottomLeft.dx - textBottomRight.dx, 24.0);
    expect(snackBarBottomRight.dx - actionTextBottomRight.dx, 24.0 + 30.0); // margin + right padding
    expect(snackBarBottomRight.dy - actionTextBottomRight.dy, 17.0); // margin (with no bottom padding)

In this case, the SnackBar position is calculated using the content of the SnackBar and the action of the SnackBar.
Moreover, there are many excess details in the test, such as the BottomNavigationBar widget (for example: in my case it is just a SizedBox), GestureDetecter and too detailed SnackBar.
The test has magic numbers. I spent a lot of time realizing what they mean
Test 2:
This test is good. It makes sure the button goes up when the SnackBar appears.
but it just check that y position of FAB more then y position of SnackBar.
i can improve the test by check the exact position of the FAB after the appearance of SnackBar.
Test 3:
This test should catch a bug, but it skips it.
expect (snackBarBottomRight.dy - actionTextBottomRight.dy, 27.0) This line checks that the distance between the action and the SnackBar border is 27, but does not check at what y position of the SnackBar is now. (same problem in test 1)
Test 4:
This is the correct test, but a lot of excess details in my opinion.
Its advantage is that it checks for padding, but I would move the checks for padding to a separate test.

In total:

I think test 1 should be replaced with my test ('$behavior should align snackBar with the top of bottom navigation bar when Scaffold has no FloatingActionButton')
Improve test 2
test 3 has all the same problems as test 1. I also suggest replacing it with my test.
Remove excess details from the test 4.
move all my tests (except ${SnackBarBehavior.floating} should align snackBar with the top of FloatingActionButton' 'when Scaffold has a FloatingActionButton') to snack_bar_test.dart
maybe separate tests that check SnackBar position when Scaffold has a padding

shihaohong · 2020-01-10T18:12:51Z

I was sure that testing the position of the elements is the responsibility of the scaffold, and so I thought that the tests should be in scaffold_test.dart. :)

That's okay, I had the same thoughts :)

Test 1:
This test has a very complicated "expected" part:
    expect(textBottomLeft.dx - snackBarBottomLeft.dx, 24.0 + 10.0); // margin + left padding
    expect(snackBarBottomLeft.dy - textBottomLeft.dy, 17.0); // margin (with no bottom padding)
    expect(actionTextBottomLeft.dx - textBottomRight.dx, 24.0);
    expect(snackBarBottomRight.dx - actionTextBottomRight.dx, 24.0 + 30.0); // margin + right padding
    expect(snackBarBottomRight.dy - actionTextBottomRight.dy, 17.0); // margin (with no bottom padding)
In this case, the SnackBar position is calculated using the content of the SnackBar and the action of the SnackBar.
Moreover, there are many excess details in the test, such as the BottomNavigationBar widget (for example: in my case it is just a SizedBox), GestureDetecter and too detailed SnackBar.
The test has magic numbers. I spent a lot of time realizing what they mean

Hmm, this seems to more so test the margins/padding between the edges of SnackBar and its contents. Perhaps keeping it and renaming the test would suffice?

Test 2:
This test is good. It makes sure the button goes up when the SnackBar appears.
but it just check that y position of FAB more then y position of SnackBar.
i can improve the test by check the exact position of the FAB after the appearance of SnackBar.
Sounds good! If it's an easy change to make, I'd welcome it :)

Test 3:
This test should catch a bug, but it skips it.
expect (snackBarBottomRight.dy - actionTextBottomRight.dy, 27.0) This line checks that the distance between the action and the SnackBar border is 27, but does not check at what y position of the SnackBar is now. (same problem in test 1)

Same comment as with test 1, let's keep the test for now and just rename it such that it's clear that it checks for margin/padding correctness.

Test 4:
This is the correct test, but a lot of excess details in my opinion.
Its advantage is that it checks for padding, but I would move the checks for padding to a separate test.

Seems like a great idea!

In total:

I think test 1 should be replaced with my test ('$behavior should align snackBar with the top of bottom navigation bar when Scaffold has no FloatingActionButton')

Improve test 2

test 3 has all the same problems as test 1. I also suggest replacing it with my test.

Remove excess details from the test 4.

move all my tests (except ${SnackBarBehavior.floating} should align snackBar with the top of FloatingActionButton' 'when Scaffold has a FloatingActionButton') to snack_bar_test.dart
maybe separate tests that check SnackBar position when Scaffold has a padding

I think for test 1, 3 and 4, what we could do is add your tests, and then take out the margin/padding sections of the preexisting tests and make them into new tests. That way, we don't remove tests for expected padding, while incorporating your positioning tests.

Any idea why the original tests never caught the error case? Is it simply because that specific case was never tested?

filaps · 2020-01-12T10:57:10Z

Any idea why the original tests never caught the error case? Is it simply because that specific case was never tested?

yes, that's right.

Perhaps keeping it and renaming the test would suffice?

Good idea, i agree!

take out the margin/padding sections of the preexisting tests and make them into new tests.

I found a test with paddings of SnackBar. In my commit i just improved it by adding universal behavior and checking the leftBottom and rightBottom points of SnackBar.

shihaohong

This looks much better than it used to before, thanks again for doing all the refactor!

The only bigger thing is tweaking some tests to fully animate the SnackBar instead of checking the positions of the SnackBar and the FAB only at the very beginning of the animation. Other than that, it's mostly code clarity.

shihaohong · 2020-01-13T17:56:34Z

packages/flutter/test/material/snack_bar_test.dart

nit: Better to use the same word to describe "the spacing in between widgets" since it might get confusing for future readers when padding/margin/indent is used interchangeably.

Suggested change

'Indent between border and content of SnackBar when behavior is fixed',

'Custom padding between SnackBar and its contents when set to SnackBarBehavior.fixed',

shihaohong · 2020-01-13T18:00:53Z

packages/flutter/test/material/snack_bar_test.dart

Suggested change

const int fabMargin = 16;

// {{Describe wherever this value was derived from}}

const int defaultFabPadding = 16;

shihaohong · 2020-01-13T18:08:01Z

packages/flutter/test/material/snack_bar_test.dart

nit: I confused origin and original here, since origin can also be interpreted as the reference point of some coordinate system :) Maybe we can make this clearer this way:

// Get the Rect of the FAB to compare after the SnackBar appears. final Offset originalFabRect = tester.getRect(find.byType(FloatingActionButton)); // Code that makes SnackBar appear. // FAB should shift upwards after SnackBar appears. expect(originalFabRect.getBottomRight.dy > fabBottomRight.dy, true);

shihaohong · 2020-01-13T18:09:56Z

packages/flutter/test/material/snack_bar_test.dart

Suggested change

expect(snackBarTopRight.dy - fabMargin, equals(fabBottomRight.dy));

// FAB should be positioned above the SnackBar by the default padding.

expect(snackBarTopRight.dy - fabMargin, equals(fabBottomRight.dy));

shihaohong · 2020-01-13T18:11:13Z

packages/flutter/test/material/snack_bar_test.dart

Suggested change

'Indent between border and content of SnackBar when behavior is floating',

'Custom padding between SnackBar and its contents when set to SnackBarBehavior.floating',

shihaohong · 2020-01-13T18:22:23Z

packages/flutter/test/material/snack_bar_test.dart

Suggested change

find.byType(FloatingActionButton));

find.byType(FloatingActionButton),

);

shihaohong · 2020-01-13T18:25:02Z

packages/flutter/test/material/snack_bar_test.dart

What is snackBarBox? If it's referring to the RenderBox of the SnackBar, maybe generalizing the term might be better:

Suggested change

// Since padding and margin is handled inside snackBarBox,

// Since padding between the SnackBar and the FAB is created by the SnackBar,

// the bottom offset of the SnackBar should be equal to the top offset of the FAB.

Edit: I realized later that this piece was copy-pasted, but this is a good opportunity to clean this up :)

shihaohong · 2020-01-13T18:26:22Z

packages/flutter/test/material/snack_bar_test.dart

We could just do:

Suggested change

expect(snackBarBottomCenter.dy == floatingActionButtonTopCenter.dy, true);

expect(snackBarBottomCenter.dy, floatingActionButtonTopCenter.dy);

shihaohong · 2020-01-13T18:28:00Z

packages/flutter/test/material/snack_bar_test.dart

Suggested change

await tester.pump();

await tester.pumpAndSettle(); // Have the SnackBar fully animate out.

shihaohong · 2020-01-13T18:28:36Z

packages/flutter/test/material/snack_bar_test.dart

Suggested change

await tester.pump();

await tester.pumpAndSettle(); // Have the SnackBar fully animate out.

shihaohong · 2020-01-13T18:34:27Z

Bonus points if you could get a screenshot with the fix and put it in the first comment of the issue. That helps 1) verify that the fix is as expected and 2) helps future maintainers of the code understand what the intent of this change was

shihaohong

Thank you for your patience, I was on vacation and am just getting back into the groove of things :) Pretty much good except for a few variable names in a test (they test the correct behavior though)

shihaohong · 2020-01-30T20:09:23Z

packages/flutter/test/material/snack_bar_test.dart

Use lessThan matcher here:

Suggested change

expect(originalFabRect.center.dy > fabRect.center.dy, true);

expect(fabRect.center.dy, lessThan(originalFabRect.center.dy));

shihaohong · 2020-01-30T20:12:22Z

packages/flutter/test/material/snack_bar_test.dart

Suggested change

// value was derived from floating_action_button_location.dart

// FAB's surrounding padding is set to [kFloatingActionButtonMargin] in floating_action_button_location.dart byb default.

shihaohong · 2020-01-30T20:13:51Z

packages/flutter/test/material/snack_bar_test.dart

the order is expect(actual, expected), so since fabRect is the actual Rect, it should appear first

Suggested change

expect(snackBarTopRight.dy - defaultFabPadding, fabRect.bottomRight.dy);

expect(fabRect.bottomRight.dy, snackBarTopRight.dy - defaultFabPadding);

shihaohong · 2020-01-30T20:25:32Z

packages/flutter/test/material/snack_bar_test.dart

nit

Suggested change

await tester.pumpAndSettle(); //Have the SnackBar fully animate out.

await tester.pumpAndSettle(); // Have the SnackBar fully animate out.

shihaohong · 2020-01-30T20:28:56Z

packages/flutter/test/material/snack_bar_test.dart

nit:

Suggested change

}

},

shihaohong · 2020-01-30T20:33:27Z

packages/flutter/test/material/snack_bar_test.dart

Suggested change

final Offset snackBarBottomCenter = tester.getBottomLeft(find.byType(SnackBar));

final Offset snackBarBottomLeft = tester.getBottomLeft(find.byType(SnackBar));

shihaohong · 2020-01-30T20:33:35Z

packages/flutter/test/material/snack_bar_test.dart

Suggested change

final Offset floatingActionButtonTopCenter = tester.getTopLeft(

final Offset floatingActionButtonTopLeft = tester.getTopLeft(

shihaohong · 2020-01-30T20:33:55Z

packages/flutter/test/material/snack_bar_test.dart

Suggested change

expect(snackBarBottomCenter.dy, floatingActionButtonTopCenter.dy);

expect(snackBarBottomLeft.dy, floatingActionButtonTopLeft.dy);

Accidentally approved despite wanting a few more changes

shihaohong

LGTM

shihaohong · 2020-02-03T03:40:36Z

packages/flutter/test/material/snack_bar_test.dart

Suggested change

// FAB's surrounding padding is set to [kFloatingActionButtonMargin] in floating_action_button_location.dart byb default.

// FAB's surrounding padding is set to [kFloatingActionButtonMargin] in floating_action_button_location.dart by default.

shihaohong · 2020-02-03T03:44:08Z

packages/flutter/test/material/snack_bar_test.dart

nit

Suggested change

}

},

shihaohong · 2020-02-05T21:53:39Z

All presubmit tests seem to be passing. Once the tree is green, this will be merged.

Thank you so much for this contribution and for taking the time to address all feedback!

filaps · 2020-02-06T14:43:04Z

All presubmit tests seem to be passing. Once the tree is green, this will be merged.

Thank you so much for this contribution and for taking the time to address all feedback!

Thank you for review, it was a pleasure to work with you!

Now the test checks certain position Y of FAB

added tests for testing height of SnackBar.

…n' to group 'SnackBar position' improved test by removing padding details, and renaming.

… group "SnackBar position" and improved test for testing with universal behavior

fluttergithubbot · 2020-02-06T15:53:03Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite build_tests-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

shihaohong · 2020-02-06T16:56:15Z

The presubmit tests seem to have rerun for some reason and build_tests-linux is failing on a 504 gateway timeout error (unrelated to your change)

…onButton positioning (#47616)" This reverts commit 801a6a7.

…onButton positioning (#47616)" (#50516) This reverts commit 801a6a7.

…tingActionButton positioning (#47616)" (#50516)" This reverts commit 67826bd.

fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. work in progress; do not review labels Dec 22, 2019

googlebot added the cla: yes label Dec 22, 2019

filaps changed the title ~~fixed issue where snack bar position was higher then should be~~ fixed issue where SnackBar position was higher then should be Dec 22, 2019

filaps marked this pull request as ready for review December 22, 2019 15:34

filaps changed the title ~~fixed issue where SnackBar position was higher then should be~~ [WIP] fixed issue where SnackBar position was higher then should be Dec 23, 2019

filaps changed the title ~~[WIP] fixed issue where SnackBar position was higher then should be~~ fixed issue where SnackBar position was higher then should be Dec 23, 2019

filaps closed this Dec 23, 2019

filaps reopened this Dec 23, 2019

filaps force-pushed the bugfix/snackbar branch from ffc2035 to 12a076a Compare December 25, 2019 03:38

jonahwilliams removed the work in progress; do not review label Dec 26, 2019

shihaohong self-assigned this Jan 3, 2020

shihaohong suggested changes Jan 3, 2020

View reviewed changes

shihaohong changed the title ~~fixed issue where SnackBar position was higher then should be~~ Fix SnackBar placement due to FloatingActionButton positioning Jan 3, 2020

shihaohong changed the title ~~Fix SnackBar placement due to FloatingActionButton positioning~~ Fix SnackBar clipping when it is floating due to FloatingActionButton positioning Jan 3, 2020

filaps force-pushed the bugfix/snackbar branch 2 times, most recently from 15e5326 to 64ac431 Compare January 7, 2020 16:55

shihaohong reviewed Jan 7, 2020

View reviewed changes

filaps force-pushed the bugfix/snackbar branch from 64ac431 to f20fb34 Compare January 12, 2020 10:38

shihaohong reviewed Jan 13, 2020

View reviewed changes

filaps force-pushed the bugfix/snackbar branch from f20fb34 to 32266db Compare January 19, 2020 10:05

filaps requested a review from shihaohong January 26, 2020 11:07

shihaohong previously approved these changes Jan 30, 2020

View reviewed changes

shihaohong self-requested a review January 30, 2020 20:39

filaps force-pushed the bugfix/snackbar branch 2 times, most recently from acc8ce7 to f96641d Compare February 1, 2020 12:27

shihaohong approved these changes Feb 3, 2020

View reviewed changes

shihaohong added the waiting for tree to go green label Feb 5, 2020

filaps force-pushed the bugfix/snackbar branch from f96641d to 28915f4 Compare February 6, 2020 14:36

filaps added 5 commits February 6, 2020 21:46

renamed some tests in snack_bar_test.dart

2e17635

improved one test from snack_bar_test.dart

15292f1

Now the test checks certain position Y of FAB

fixed issue where snack bar was higher then should be

1dbd586

added tests for testing height of SnackBar.

moved test 'Floating SnackBar is positioned above FloatingActionButto…

19ad3f2

…n' to group 'SnackBar position' improved test by removing padding details, and renaming.

moved test "SnackBar bottom padding is not consumed by viewInsets" to…

66c0d19

… group "SnackBar position" and improved test for testing with universal behavior

filaps force-pushed the bugfix/snackbar branch from 28915f4 to 66c0d19 Compare February 6, 2020 14:50

fluttergithubbot removed the waiting for tree to go green label Feb 6, 2020

shihaohong added the waiting for tree to go green label Feb 7, 2020

fluttergithubbot merged commit 801a6a7 into flutter:master Feb 8, 2020

shihaohong pushed a commit that referenced this pull request Feb 10, 2020

Revert "Fix SnackBar clipping when it is floating due to FloatingActi…

6cce24f

…onButton positioning (#47616)" This reverts commit 801a6a7.

shihaohong mentioned this pull request Feb 10, 2020

Revert "Fix SnackBar clipping when it is floating due to FloatingActionButton positioning" #50516

Merged

shihaohong pushed a commit that referenced this pull request Feb 10, 2020

Revert "Fix SnackBar clipping when it is floating due to FloatingActi…

67826bd

…onButton positioning (#47616)" (#50516) This reverts commit 801a6a7.

shihaohong pushed a commit that referenced this pull request Feb 11, 2020

Revert "Revert "Fix SnackBar clipping when it is floating due to Floa…

dc8ba19

…tingActionButton positioning (#47616)" (#50516)" This reverts commit 67826bd.

This was referenced Feb 11, 2020

Step 1 of ?: Re-land SnackBar clipping issue as opt-in change #50583

Closed

Step 1: SnackBarBehavior.floating offset fix - Soft breaking change #50597

Merged

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

	String getEnumStringValue(SnackBarBehavior behaviour) => behaviour.toString().split('.').last;
	String getEnumStringValue(SnackBarBehavior behavior) => behavior.toString().split('.').last;

	for (SnackBarBehavior behavior in SnackBarBehavior.values) {
	for (final SnackBarBehavior behavior in SnackBarBehavior.values) {

	testWidgets('behavior is ${getEnumStringValue(behavior)} and scaffold has no other elements',
	testWidgets('$behavior should align with the bottom of the Scaffold when the Scaffold has no other elements',

	floatingActionButton: FloatingActionButton(onPressed: () {},),
	floatingActionButton: FloatingActionButton(onPressed: () {}),

-      scaffoldState.showSnackBar(const SnackBar(content: Text('Snackbar text'), behavior: SnackBarBehavior.floating,));
+      scaffoldState.showSnackBar(
+        const SnackBar(
+          content: Text('Snackbar text'),
+          behavior: SnackBarBehavior.floating,
+        ),
+      );

	'when scaffold has no other elements',
	'when Scaffold has no other elements',

	'$behavior should align snackBar with the bottom of Scaffold '
	'$behavior should align SnackBar with the bottom of Scaffold '

	'${SnackBarBehavior.fixed} should align snackBar with the bottom of Scaffold '
	'${SnackBarBehavior.fixed} should align SnackBar with the bottom of Scaffold '

	'${SnackBarBehavior.floating} should align snackBar with the top of FloatingActionButton'
	'${SnackBarBehavior.floating} should align SnackBar with the top of FloatingActionButton'

	'Indent between border and content of SnackBar when behavior is fixed',
	'Custom padding between SnackBar and its contents when set to SnackBarBehavior.fixed',

	const int fabMargin = 16;
	// {{Describe wherever this value was derived from}}
	const int defaultFabPadding = 16;

	expect(snackBarTopRight.dy - fabMargin, equals(fabBottomRight.dy));
	// FAB should be positioned above the SnackBar by the default padding.
	expect(snackBarTopRight.dy - fabMargin, equals(fabBottomRight.dy));

	find.byType(FloatingActionButton));
	find.byType(FloatingActionButton),
	);

	// Since padding and margin is handled inside snackBarBox,
	// Since padding between the SnackBar and the FAB is created by the SnackBar,
	// the bottom offset of the SnackBar should be equal to the top offset of the FAB.

	expect(snackBarBottomCenter.dy == floatingActionButtonTopCenter.dy, true);
	expect(snackBarBottomCenter.dy, floatingActionButtonTopCenter.dy);

	await tester.pump();
	await tester.pumpAndSettle(); // Have the SnackBar fully animate out.

	expect(originalFabRect.center.dy > fabRect.center.dy, true);
	expect(fabRect.center.dy, lessThan(originalFabRect.center.dy));

	// value was derived from floating_action_button_location.dart
	// FAB's surrounding padding is set to [kFloatingActionButtonMargin] in floating_action_button_location.dart byb default.

	expect(snackBarTopRight.dy - defaultFabPadding, fabRect.bottomRight.dy);
	expect(fabRect.bottomRight.dy, snackBarTopRight.dy - defaultFabPadding);

	await tester.pumpAndSettle(); //Have the SnackBar fully animate out.
	await tester.pumpAndSettle(); // Have the SnackBar fully animate out.

Fix SnackBar clipping when it is floating due to FloatingActionButton positioning #47616

Fix SnackBar clipping when it is floating due to FloatingActionButton positioning #47616

Uh oh!

Conversation

filaps commented Dec 22, 2019 • edited by shihaohong Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshots

Before fix

After fix

Bug cause

Solution

Tests

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

shihaohong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shihaohong commented Jan 3, 2020

Uh oh!

shihaohong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filaps commented Dec 22, 2019 •

edited by shihaohong

Loading

	final Offset snackBarBottomCenter = tester.getBottomLeft(find.byType(SnackBar));
	final Offset snackBarBottomLeft = tester.getBottomLeft(find.byType(SnackBar));

	final Offset floatingActionButtonTopCenter = tester.getTopLeft(
	final Offset floatingActionButtonTopLeft = tester.getTopLeft(

	expect(snackBarBottomCenter.dy, floatingActionButtonTopCenter.dy);
	expect(snackBarBottomLeft.dy, floatingActionButtonTopLeft.dy);