Skip to content

Fixed slider value indicator not disappearing after a bit on desktop platform when slider is clicked not dragged #128137

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 19 commits into from
Jun 14, 2023
Merged

Fixed slider value indicator not disappearing after a bit on desktop platform when slider is clicked not dragged #128137

merged 19 commits into from
Jun 14, 2023

Conversation

cruiser-baxter
Copy link
Contributor

@cruiser-baxter cruiser-baxter commented Jun 2, 2023 •
edited by TahaTesser
Loading

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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

cruiser-baxter and others added 6 commits May 23, 2023 09:45
@cruiser-baxter
slider_test.dart added a test for the desktop slider to check that wh…
14e0157 
…en the slider is clicked and not dragged, the slider value indicator dissappears after a bit
@cruiser-baxter
Reformatted the desktop platform slider value indicator disappears af…
e28bd6a 
…ter a bit when clicked test and added some comments
@cruiser-baxter
In the _startInteraction method, changed the if(NOT _active && NOT ha…
1c22d71 
…sFocus && _state.valueIndicatorController.status == AnimationStatus.completed) conditional check, removed the && NOT hasFocus check.
@cruiser-baxter
Merge branch 'flutter:master' into b123313
13cb760
@cruiser-baxter
Merge branch 'flutter:master' into b123313
19dfd0b
@cruiser-baxter
renamed test title to conform with a similar existing test. Removed d…
5ba85ca 
…uplicate pumpAndSettle call before the timed call.
@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jun 2, 2023
@github-actions github-actions bot removed the f: material design flutter/packages/flutter/material repository. label Jun 2, 2023
@Piinks Piinks requested a review from TahaTesser June 5, 2023 23:09
@flutter-dashboard flutter-dashboard bot added the f: material design flutter/packages/flutter/material repository. label Jun 6, 2023
TahaTesser
TahaTesser requested changes Jun 8, 2023
View reviewed changes
Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this!
A few changes are needed then it should be good to go.

TahaTesser
TahaTesser requested changes Jun 8, 2023
View reviewed changes
Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this!
A few changes are needed then it should be good to go.

cruiser-baxter and others added 3 commits June 8, 2023 11:58
@cruiser-baxter
Merge branch 'master' into b123313
cc8a03c
@cruiser-baxter @TahaTesser
Update packages/flutter/lib/src/material/slider.dart
1f5049e 
Committing changes suggested by TahaTesser. Moved 
 if conditional to single line.

Co-authored-by: Taha Tesser <tessertaha@gmail.com>
@cruiser-baxter @TahaTesser
Update packages/flutter/test/material/slider_test.dart
73e9c2c 
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 <tessertaha@gmail.com>
@cruiser-baxter
Copy link
Contributor Author

@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.

TahaTesser
TahaTesser approved these changes Jun 8, 2023
View reviewed changes
Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@TahaTesser TahaTesser requested a review from Piinks June 8, 2023 19:19
@TahaTesser
Copy link
Member

This will need a second review from @Piinks to land.

Piinks
Piinks reviewed Jun 9, 2023
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@TahaTesser
Copy link
Member

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?

This doesn't change the behavior on mobile. _endInteraction always dismisses the value indicator.

Looks like the !hasFocus was added in #113543 by @TahaTesser, which was also desktop specific.

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.
However current Slider implementation doesn't support invoking value indicators this way. #113538.

Might be good to add a test for this (if one does not already exist)

Actually, we can remove the desktop target on this test. Since the interaction behavior is the same on desktop and mobile.

TahaTesser
TahaTesser requested changes Jun 12, 2023
View reviewed changes
cruiser-baxter and others added 6 commits June 12, 2023 13:07
@cruiser-baxter @TahaTesser
Update packages/flutter/test/material/slider_test.dart
d5b9304 
Committing changes suggested by TahaTesser. Change removes desktop specific platform target.

Co-authored-by: Taha Tesser <tessertaha@gmail.com>
@cruiser-baxter @TahaTesser
Update packages/flutter/test/material/slider_test.dart
f18903a 
Committing changes suggested by TahaTesser. Changes make the test future proof by using material 3 color.

Co-authored-by: Taha Tesser <tessertaha@gmail.com>
@cruiser-baxter @TahaTesser
Update packages/flutter/test/material/slider_test.dart
45f89d5 
Committing changes suggested by TahaTesser. Changes the color being checcked for  from hex to material colorScheme.primary.

Co-authored-by: Taha Tesser <tessertaha@gmail.com>
@cruiser-baxter @TahaTesser
Update packages/flutter/test/material/slider_test.dart
3c7b637 
Committing changes suggested by TahaTesser. Changes color being checked for from hex to colorScheme.primary.

Co-authored-by: Taha Tesser <tessertaha@gmail.com>
@cruiser-baxter @TahaTesser
Update packages/flutter/test/material/slider_test.dart
0586a84 
Committing changes suggested by TahaTesser. Changes color being checked for from hex to colorScheme.primary.

Co-authored-by: Taha Tesser <tessertaha@gmail.com>
@cruiser-baxter
Merge branch 'master' into b123313
a18115a
cruiser-baxter
cruiser-baxter commented Jun 12, 2023
View reviewed changes
Copy link
Contributor Author

@cruiser-baxter cruiser-baxter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed the suggested changes and committed all suggestions. Thank you TahaTesser for all of your input, feedback, and code change suggestions.

Piinks
Piinks reviewed Jun 12, 2023
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
TahaTesser requested changes Jun 13, 2023
View reviewed changes
Copy link
Member

@TahaTesser TahaTesser left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tried the test locally. After updating the test for M3 the expected order for path has changed.

@TahaTesser TahaTesser requested a review from Piinks June 13, 2023 12:42
TahaTesser
TahaTesser approved these changes Jun 13, 2023
View reviewed changes
Copy link
Member

@TahaTesser TahaTesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cruiser-baxter
Copy link
Contributor Author

@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.

Piinks
Piinks approved these changes Jun 14, 2023
View reviewed changes
Copy link
Contributor

@Piinks Piinks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flutter_LGTM
Thank you!

@Piinks Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 14, 2023
@auto-submit auto-submit bot merged commit 52c4db8 into flutter:master Jun 14, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jun 14, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 14, 2023
@engine-flutter-autoroll
52c4db8 Fixed slider value indicator not disappearing after a bit on …
ffe787b 
…desktop platform when slider is clicked not dragged (flutter/flutter#128137)
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Jun 14, 2023
@engine-flutter-autoroll
Manual roll Flutter from 09b7e56 to 95be76a (14 revisions) (#4214)
c34ca5d 
Manual roll requested by tarrinneal@google.com

flutter/flutter@09b7e56...95be76a

2023-06-14 mdebbar@google.com [web] Migrate framework away from dart:html and package:js (flutter/flutter#128580)
2023-06-14 77465135+cruiser-baxter@users.noreply.github.com Fixed slider value indicator not disappearing after a bit on desktop platform when slider is clicked not dragged (flutter/flutter#128137)
2023-06-14 goderbauer@google.com Inline AbstractNode into SemanticsNode and Layer (flutter/flutter#128467)
2023-06-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 727b4413fe6f to 2d8d5ecfe4a8 (5 revisions) (flutter/flutter#128842)
2023-06-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from 66a5761412f9 to 727b4413fe6f (10 revisions) (flutter/flutter#128841)
2023-06-14 engine-flutter-autoroll@skia.org Roll Flutter Engine from b6bf3a6f1ccd to 66a5761412f9 (1 revision) (flutter/flutter#128813)
2023-06-13 william.oprandi+github@gmail.com Fix syntax error in docstring (flutter/flutter#128692)
2023-06-13 36861262+QuncCccccc@users.noreply.github.com Update unit tests in material library for Material 3 (flutter/flutter#128725)
2023-06-13 christopherfujino@gmail.com [flutter_tools] Suppress git output in flutter channel (flutter/flutter#128475)
2023-06-13 katelovett@google.com Fix ensureVisible and default focus traversal for reversed scrollables (flutter/flutter#128756)
2023-06-13 engine-flutter-autoroll@skia.org Roll Flutter Engine from f9a0a0dafeea to b6bf3a6f1ccd (2 revisions) (flutter/flutter#128797)
2023-06-13 36861262+QuncCccccc@users.noreply.github.com Update rest of the unit tests in material library for Material 3 (flutter/flutter#128747)
2023-06-13 36861262+QuncCccccc@users.noreply.github.com Update tests in material library for Material 3 by default (flutter/flutter#128300)
2023-06-13 hans.muller@gmail.com 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 rmistry@google.com,stuartmorgan@google.com,tarrinneal@google.com 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
@HansMuller HansMuller mentioned this pull request Jun 16, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023
@engine-flutter-autoroll
52c4db8 Fixed slider value indicator not disappearing after a bit on …
5af8b6b 
…desktop platform when slider is clicked not dragged (flutter/flutter#128137)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
52c4db8 Fixed slider value indicator not disappearing after a bit on …
aad10e8 
…desktop platform when slider is clicked not dragged (flutter/flutter#128137)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
52c4db8 Fixed slider value indicator not disappearing after a bit on …
951db4b 
…desktop platform when slider is clicked not dragged (flutter/flutter#128137)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023
@engine-flutter-autoroll
52c4db8 Fixed slider value indicator not disappearing after a bit on …
3f1e2d0 
…desktop platform when slider is clicked not dragged (flutter/flutter#128137)
@hgraceb hgraceb mentioned this pull request Nov 29, 2023
Merged
8 tasks
auto-submit bot pushed a commit that referenced this pull request Dec 11, 2023
@hgraceb
Improve slider's value indicator display test (#139198)
4252aa0 
The previous test for #128137 does not effectively regress and verify #123313 because the issue is specific to desktop platforms.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Piinks Piinks Piinks approved these changes

@TahaTesser TahaTesser TahaTesser approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Slider label never goes away when clicking slider to new value
3 participants
@cruiser-baxter @TahaTesser @Piinks