New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SingleChildScrollView
for NavigationRail
#137415
Add SingleChildScrollView
for NavigationRail
#137415
Conversation
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Hey @mbfakourii, can you please add a test to this to make sure we never regress this? See the comment posted by the flutter-dashboard. Also, it looks like this change is failing some of the checks. Can you please take a look at that and address it? Thanks! |
Yes, Sure. I added the test. ../../bin/flutter test --test-randomize-ordering-seed=20231104 --fatal-warnings --no-track-widget-creation test/dart/ test/scheduler/ test/animation/ test/foundation/ test/semantics/ test/harness/ test/painting/ test/gestures/ test/examples/ test/rendering/ test/physics/ test/cupertino/ test/material/ test/services/ These fails are related to the file flutter/packages/flutter/test/material/value_indicating_slider_test.dart Please may check again |
This is fantastic! I hope the PR is approved quickly. |
This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Looks like some existing tests are failing, can you please take a look and fix those up? |
I checked the fail tests, please check here. |
There's at least one failing test that's pretty clearly linked to the change here: 07:58 +8848 ~22: /b/s/w/ir/x/w/flutter/packages/flutter/test/material/navigation_rail_test.dart: Semantics - labelType=[none]
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: semantics node matching:
TestSemantics(
id: 0,
rect: Rect.fromLTRB(0.0, 0.0, 2400.0, 1800.0),
elevation: 0.0,
thickness: 0.0,
children: <TestSemantics>[
TestSemantics(
textDirection: TextDirection.ltr,
children: <TestSemantics>[
TestSemantics(
children: <TestSemantics>[
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.scopesRoute],
children: <TestSemantics>[
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.isSelected,
SemanticsFlag.isFocusable],
actions: <SemanticsAction>[SemanticsAction.tap],
label: 'Abc
Tab 1 of 4',
textDirection: TextDirection.ltr,
children: <TestSemantics>[
],
),
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.isFocusable],
actions: <SemanticsAction>[SemanticsAction.tap],
label: 'Def
Tab 2 of 4',
textDirection: TextDirection.ltr,
children: <TestSemantics>[
],
),
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.isFocusable],
actions: <SemanticsAction>[SemanticsAction.tap],
label: 'Ghi
Tab 3 of 4',
textDirection: TextDirection.ltr,
children: <TestSemantics>[
],
),
TestSemantics(
flags: <SemanticsFlag>[SemanticsFlag.isFocusable],
actions: <SemanticsAction>[SemanticsAction.tap],
label: 'Jkl
Tab 4 of 4',
textDirection: TextDirection.ltr,
children: <TestSemantics>[
],
),
TestSemantics(
label: 'body',
textDirection: TextDirection.ltr,
children: <TestSemantics>[
],
),
],
),
],
),
],
),
],
)
Actual: SemanticsTester:<SemanticsTester for SemanticsNode#0(Rect.fromLTRB(0.0, 0.0, 2400.0,
1800.0))>
Which: expected node id null to have 5 children but found 2.
Current SemanticsNode tree:
SemanticsNode#0
│ Rect.fromLTRB(0.0, 0.0, 2400.0, 1800.0)
│
└─SemanticsNode#1
│ Rect.fromLTRB(0.0, 0.0, 800.0, 600.0) scaled by 3.0x
│ textDirection: ltr
│
└─SemanticsNode#2
│ Rect.fromLTRB(0.0, 0.0, 800.0, 600.0)
│ sortKey: OrdinalSortKey#1e145(order: 0.0)
│
└─SemanticsNode#3
│ Rect.fromLTRB(0.0, 0.0, 800.0, 600.0)
│ flags: scopesRoute
│
├─SemanticsNode#4
│ │ Rect.fromLTRB(0.0, 8.0, 80.0, 184.0)
│ │ flags: hasImplicitScrolling
│ │ scrollExtentMin: 0.0
│ │ scrollPosition: 0.0
│ │ scrollExtentMax: 0.0
│ │
│ ├─SemanticsNode#5
│ │ Rect.fromLTRB(0.0, 0.0, 80.0, 44.0)
│ │ actions: tap
│ │ flags: isSelected, isFocusable
│ │ label:
│ │ "Abc
│ │ Tab 1 of 4"
│ │ textDirection: ltr
│ │
│ ├─SemanticsNode#6
│ │ Rect.fromLTRB(0.0, 44.0, 80.0, 88.0)
│ │ actions: tap
│ │ flags: isFocusable
│ │ label:
│ │ "Def
│ │ Tab 2 of 4"
│ │ textDirection: ltr
│ │
│ ├─SemanticsNode#7
│ │ Rect.fromLTRB(0.0, 88.0, 80.0, 132.0)
│ │ actions: tap
│ │ flags: isFocusable
│ │ label:
│ │ "Ghi
│ │ Tab 3 of 4"
│ │ textDirection: ltr
│ │
│ └─SemanticsNode#8
│ Rect.fromLTRB(0.0, 132.0, 80.0, 176.0)
│ actions: tap
│ flags: isFocusable
│ label:
│ "Jkl
│ Tab 4 of 4"
│ textDirection: ltr
│
└─SemanticsNode#9
Rect.fromLTRB(80.0, 290.0, 800.0, 310.0)
label: "body"
textDirection: ltr
The semantics tree would have matched the following configuration:
TestSemantics.root(
children: <TestSemantics>[
TestSemantics(
id: 1,
textDirection: TextDirection.ltr,
children: <TestSemantics>[
TestSemantics(
id: 2,
children: <TestSemantics>[
TestSemantics(
id: 3,
flags: <SemanticsFlag>[SemanticsFlag.scopesRoute],
children: <TestSemantics>[
TestSemantics(
id: 4,
flags: <SemanticsFlag>[SemanticsFlag.hasImplicitScrolling],
children: <TestSemantics>[
TestSemantics(
id: 5,
flags: <SemanticsFlag>[SemanticsFlag.isSelected,
SemanticsFlag.isFocusable],
actions: <SemanticsAction>[SemanticsAction.tap],
label: 'Abc\nTab 1 of 4',
textDirection: TextDirection.ltr,
),
TestSemantics(
id: 6,
flags: <SemanticsFlag>[SemanticsFlag.isFocusable],
actions: <SemanticsAction>[SemanticsAction.tap],
label: 'Def\nTab 2 of 4',
textDirection: TextDirection.ltr,
),
TestSemantics(
id: 7,
flags: <SemanticsFlag>[SemanticsFlag.isFocusable],
actions: <SemanticsAction>[SemanticsAction.tap],
label: 'Ghi\nTab 3 of 4',
textDirection: TextDirection.ltr,
),
TestSemantics(
id: 8,
flags: <SemanticsFlag>[SemanticsFlag.isFocusable],
actions: <SemanticsAction>[SemanticsAction.tap],
label: 'Jkl\nTab 4 of 4',
textDirection: TextDirection.ltr,
),
],
),
TestSemantics(
id: 9,
label: 'body',
textDirection: TextDirection.ltr,
),
],
),
],
),
],
),
],
)
When the exception was thrown, this was the stack:
#4 main.<anonymous closure> (file:///b/s/w/ir/x/w/flutter/packages/flutter/test/material/navigation_rail_test.dart:2276:5)
<asynchronous suspension>
#5 testWidgetsWithLeakTracking.wrappedCallBack (package:leak_tracker_flutter_testing/src/test_widgets.dart:108:5)
<asynchronous suspension>
#6 testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:168:15)
<asynchronous suspension>
#7 TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1017:5)
<asynchronous suspension>
<asynchronous suspension>
(elided one frame from package:stack_trace)
This was caught by the test expectation on the following line:
file:///b/s/w/ir/x/w/flutter/packages/flutter/test/material/navigation_rail_test.dart line 2276
The test description was:
Semantics - labelType=[none]
════════════════════════════════════════════════════════════════════════════════════════════════════
07:58 +8848 ~22 -1: /b/s/w/ir/x/w/flutter/packages/flutter/test/material/navigation_rail_test.dart: Semantics - labelType=[none] [E]
Test failed. See exception logs above.
The test description was: Semantics - labelType=[none] That one will likely need to get fixed. If you see other unrelated test failures, try rebasing this PR with the latest master and see if they go away. The tree is currently green, so there are no unrelated existing failures. |
I checked and tested this issue several times, I even returned the codes before I changed them, but I get the same error again! I don't think it is related to this PR! |
In that output of the test there is a new scrollable semantics node that wasn't there before if you compare expected and actual:
This is node is almost guaranteed produced by the SingleChildScrollView added in this PR. So, in order to get the tests passing, this test will have to be adjusted to account for this extra node. |
Fixed, can you please check again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Nice improvement:) Thanks for the contribution!
…6013) Manual roll Flutter from 2f6fdf2650d4 to ace9181172b6 (57 revisions) Manual roll requested by stuartmorgan@google.com flutter/flutter@2f6fdf2...ace9181 2024-01-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from 9c3ebf67b5da to bedafa8794b6 (4 revisions) (flutter/flutter#142478) 2024-01-29 jmccandless@google.com onNavigationNotification for *App.router (flutter/flutter#142190) 2024-01-29 fluttergithubbot@gmail.com Marks Mac_x64 framework_tests_misc to be unflaky (flutter/flutter#142118) 2024-01-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3e2b8975bd5b to 9c3ebf67b5da (1 revision) (flutter/flutter#142472) 2024-01-29 66151079+bryanoli@users.noreply.github.com Feat: TextField can scroll when disabled (flutter/flutter#140922) 2024-01-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from 436f91f3b06b to 3e2b8975bd5b (2 revisions) (flutter/flutter#142466) 2024-01-29 fluttergithubbot@gmail.com Marks Mac_arm64 framework_tests_misc to be unflaky (flutter/flutter#142119) 2024-01-29 goderbauer@google.com Fix InputDecorationTheme copyWith fallback for iconColor (flutter/flutter#142462) 2024-01-29 mbfakourii@gmail.com Add `SingleChildScrollView` for `NavigationRail` (flutter/flutter#137415) 2024-01-29 737941+loic-sharma@users.noreply.github.com [Windows Arm64] Run plugin test post-submit (flutter/flutter#141987) 2024-01-29 andrewrkolos@gmail.com Catch file system exceptions when trying to parse user-provided asset file paths (flutter/flutter#142214) 2024-01-29 nathan.wilson1232@gmail.com Implementing `switch` expressions in `foundation/` and `material/` (flutter/flutter#142279) 2024-01-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from bff1e46c0d65 to 436f91f3b06b (1 revision) (flutter/flutter#142455) 2024-01-29 polinach@google.com Opt out test from leak tracking. (flutter/flutter#142417) 2024-01-29 zanderso@users.noreply.github.com Update Android minSdkVersion to 21 (flutter/flutter#142267) 2024-01-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3d87470655b1 to bff1e46c0d65 (1 revision) (flutter/flutter#142446) 2024-01-29 engine-flutter-autoroll@skia.org Roll Packages from cbe8100 to 516648a (3 revisions) (flutter/flutter#142445) 2024-01-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from 1405cb7b6e74 to 3d87470655b1 (1 revision) (flutter/flutter#142444) 2024-01-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3e65f1720a6f to 1405cb7b6e74 (1 revision) (flutter/flutter#142432) 2024-01-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from 210ed1dfb8cf to 3e65f1720a6f (1 revision) (flutter/flutter#142429) 2024-01-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from f15cb86d31c3 to 210ed1dfb8cf (1 revision) (flutter/flutter#142426) 2024-01-29 engine-flutter-autoroll@skia.org Roll Flutter Engine from f3d48be76630 to f15cb86d31c3 (11 revisions) (flutter/flutter#142425) 2024-01-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 95e9a15fd909 to f3d48be76630 (1 revision) (flutter/flutter#142370) 2024-01-27 zanderso@users.noreply.github.com Add no-shuffle to language_version_test.dart (flutter/flutter#142378) 2024-01-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2687ddb2655c to 95e9a15fd909 (8 revisions) (flutter/flutter#142369) 2024-01-27 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 2687ddb2655c to 2adad88a39f4 (4 revisions)" (flutter/flutter#142366) 2024-01-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2687ddb2655c to 2adad88a39f4 (4 revisions) (flutter/flutter#142362) 2024-01-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 45c06c22d5c7 to 2687ddb2655c (1 revision) (flutter/flutter#142359) 2024-01-27 31859944+LongCatIsLooong@users.noreply.github.com Remove suspicious constant from input decorator layout (flutter/flutter#142342) 2024-01-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from 2e32acf4c31a to 45c06c22d5c7 (2 revisions) (flutter/flutter#142353) 2024-01-27 engine-flutter-autoroll@skia.org Roll Flutter Engine from a65a1b55e06a to 2e32acf4c31a (1 revision) (flutter/flutter#142351) 2024-01-26 engine-flutter-autoroll@skia.org Roll Flutter Engine from 525bd7dcf7f3 to a65a1b55e06a (11 revisions) (flutter/flutter#142347) 2024-01-26 goderbauer@google.com Relands "Add runWidget to bootstrap a widget tree without a default View" (flutter/flutter#142344) 2024-01-26 amirpanahandeh@yahoo.com Fix assertion failure when reordering two dimensional children (flutter/flutter#141504) 2024-01-26 magder@google.com Limit `fuchsia_precache` in presubmit to engine rolls (flutter/flutter#142333) 2024-01-26 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 3.23.1 to 3.23.2 (flutter/flutter#142345) 2024-01-26 andrewrkolos@gmail.com refactor asset bundle code to not depend on the global Cache.flutterRoot (flutter/flutter#142277) 2024-01-26 christopherfujino@gmail.com [flutter_tools] remove await runZonedGuarded() in tests (flutter/flutter#142336) 2024-01-26 15619084+vashworth@users.noreply.github.com Only use iOS 17 physical devices in staging tests (flutter/flutter#142323) 2024-01-26 dacoharkes@google.com Roll deps from dart-lang/native in templates (flutter/flutter#142322) 2024-01-26 andrewrkolos@gmail.com Remove duplicate global declaration of `UserMessages` (flutter/flutter#142281) 2024-01-26 magder@google.com Redistribute iOS TESTOWNERS (flutter/flutter#142315) 2024-01-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Add runWidget to bootstrap a widget tree without a default View" (flutter/flutter#142339) 2024-01-26 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Roll Flutter Engine from 525bd7dcf7f3 to 65d1291c3add (1 revision)" (flutter/flutter#142332) ...
This seems to cause a regression, see #143061 |
NavigationRail now uses a SingleChildScrollView (flutter/flutter#137415) fixes #143061
NavigationRail now uses a SingleChildScrollView (flutter/flutter#137415) fixes flutter/flutter#143061
Reason for revert: #143061, this is breaking some existing use cases, see also flutter/samples#2157. If we try this again, we need to add this in less breaking way. |
Time to revert pull request flutter/flutter/137415 has elapsed. |
This reverts commit fd7f45a.
Reverts #137415 Reverting due to #143061. this is breaking some existing use cases, see also flutter/samples#2157. If we try this again, we need to add this in less breaking way. Fixes #143061
Description
Add
SingleChildScrollView
toNavigationRail
for scrolling.Closes: #89167
Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.