Fixed slider value indicator not disappearing after a bit on desktop platform when slider is clicked not dragged #128137
Conversation
…en the slider is clicked and not dragged, the slider value indicator dissappears after a bit
…ter a bit when clicked test and added some comments
…sFocus && _state.valueIndicatorController.status == AnimationStatus.completed) conditional check, removed the && NOT hasFocus check.
…uplicate pumpAndSettle call before the timed call.
flutter-dashboard
bot
added
f: material design
github-actions
bot
removed
the
f: material design
flutter-dashboard
bot
added
the
f: material design
TahaTesser
left a comment
TahaTesser
left a comment
There was a problem hiding this comment.
Thanks for fixing this!
A few changes are needed then it should be good to go.
TahaTesser
left a comment
TahaTesser
left a comment
There was a problem hiding this comment.
Thanks for fixing this!
A few changes are needed then it should be good to go.
Committing changes suggested by TahaTesser. Moved if conditional to single line. Co-authored-by: Taha Tesser <[email protected]>
COmmitting changes suggested by TahaTesser. Significantly simplified the test, updated test title, and added a comment indicating that it is a regression test for buf flutter#123313. Co-authored-by: Taha Tesser <[email protected]>
|
@TahaTesser, I sincerely appreciate your review and suggestions. After going over the diff for my own learning I committed the suggested changes. I learned a lot from the simplified test. Thank you. |
|
This will need a second review from @Piinks to land. |
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Hey @cruiser-baxter, welcome! Thanks for contributing!
The description and test sound like this is specific to desktop, does this change alter the behavior on mobile? Will the label disappear now on mobile if there is focus? Might be good to add a test for this (if one does not already exist)
Looks like the !hasFocus was added in #113543 by @TahaTesser, which was also desktop specific.
This doesn't change the behavior on mobile.
Indeed I added it but based on the issue report, I found that interacting directly should dismiss the value indicator. This issue fixes that interaction behavior. While on the desktop the value indicator appears but doesn't dismiss when only using the keyboard.
Actually, we can remove the desktop target on this test. Since the interaction behavior is the same on desktop and mobile. |
Committing changes suggested by TahaTesser. Change removes desktop specific platform target. Co-authored-by: Taha Tesser <[email protected]>
Committing changes suggested by TahaTesser. Changes make the test future proof by using material 3 color. Co-authored-by: Taha Tesser <[email protected]>
Committing changes suggested by TahaTesser. Changes the color being checcked for from hex to material colorScheme.primary. Co-authored-by: Taha Tesser <[email protected]>
Committing changes suggested by TahaTesser. Changes color being checked for from hex to colorScheme.primary. Co-authored-by: Taha Tesser <[email protected]>
Committing changes suggested by TahaTesser. Changes color being checked for from hex to colorScheme.primary. Co-authored-by: Taha Tesser <[email protected]>
cruiser-baxter
left a comment
cruiser-baxter
left a comment
There was a problem hiding this comment.
I have reviewed the suggested changes and committed all suggestions. Thank you TahaTesser for all of your input, feedback, and code change suggestions.
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
Thank you for all the context @TahaTesser!
@cruiser-baxter it looks like there are some failing tests here now, can you take a look?
/b/s/w/ir/x/w/flutter/packages/flutter/test/material/slider_test.dart: Value indicator disappears after adjusting the slider
|
@TahaTesser Thank you for fixing that. I am in the middle of moving now and won't be able to look at this much for the next week. |
…desktop platform when slider is clicked not dragged (flutter/flutter#128137)
Manual roll requested by [email protected] flutter/flutter@09b7e56...95be76a 2023-06-14 [email protected] [web] Migrate framework away from dart:html and package:js (flutter/flutter#128580) 2023-06-14 [email protected] Fixed slider value indicator not disappearing after a bit on desktop platform when slider is clicked not dragged (flutter/flutter#128137) 2023-06-14 [email protected] Inline AbstractNode into SemanticsNode and Layer (flutter/flutter#128467) 2023-06-14 [email protected] Roll Flutter Engine from 727b4413fe6f to 2d8d5ecfe4a8 (5 revisions) (flutter/flutter#128842) 2023-06-14 [email protected] Roll Flutter Engine from 66a5761412f9 to 727b4413fe6f (10 revisions) (flutter/flutter#128841) 2023-06-14 [email protected] Roll Flutter Engine from b6bf3a6f1ccd to 66a5761412f9 (1 revision) (flutter/flutter#128813) 2023-06-13 [email protected] Fix syntax error in docstring (flutter/flutter#128692) 2023-06-13 [email protected] Update unit tests in material library for Material 3 (flutter/flutter#128725) 2023-06-13 [email protected] [flutter_tools] Suppress git output in flutter channel (flutter/flutter#128475) 2023-06-13 [email protected] Fix ensureVisible and default focus traversal for reversed scrollables (flutter/flutter#128756) 2023-06-13 [email protected] Roll Flutter Engine from f9a0a0dafeea to b6bf3a6f1ccd (2 revisions) (flutter/flutter#128797) 2023-06-13 [email protected] Update rest of the unit tests in material library for Material 3 (flutter/flutter#128747) 2023-06-13 [email protected] Update tests in material library for Material 3 by default (flutter/flutter#128300) 2023-06-13 [email protected] Update misc tests for Material3 (flutter/flutter#128712) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…desktop platform when slider is clicked not dragged (flutter/flutter#128137)
…desktop platform when slider is clicked not dragged (flutter/flutter#128137)
…desktop platform when slider is clicked not dragged (flutter/flutter#128137)
…desktop platform when slider is clicked not dragged (flutter/flutter#128137)
3 participants
In slider.dart within the _startInteraction method and within the below conditional.
"if (!_active && !hasFocus &&
_state.valueIndicatorController.status == AnimationStatus.completed)"
Changed to:
"f (!_active &&
_state.valueIndicatorController.status == AnimationStatus.completed)"
This allows the value indicator to disappear after a bit when clicked instead of dragged on Desktop platform.
I also added a test in slider_test.dart to detect the bug if it ever returns.
Fixes #123313
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.