`Slider.onChangeStart` & `Slider.onChangeEnd` are not called on keyboard shortcuts #126896

dleyba042 · 2023-05-16T01:37:20Z

fixes #123315

This PR makes changes to the _actionHandler function used on the Slider.Dart Widget for Key Events. It ensures onChangeStart is called at the beginning of a Key Event and onChangeEnd at the end of one. This PR includes a test for the changes made.
I ran all existing tests after my changes were made and they passed.

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.

google-cla · 2023-05-16T01:37:23Z

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.

TahaTesser · 2023-05-16T11:26:06Z

Hi @dleyba042
Please note: #123315 (comment)

Thanks for attempting to fix this, however several issues in your implementation.

Check out the Style guide for Flutter repo and see how increaseAction and decreaseAction is currently calling onChanged.

in order to fix this, first we need to move the clampDoubles from onChanged to separate functions then reuse it for the callback including in the action handler.

For tests, please check out other similar tests in the framework on how to title, format and test the functionality.

TahaTesser · 2023-05-22T12:50:43Z

Hi @dleyba042
Do you've plans to update this PR? :)

dleyba042 · 2023-05-22T19:53:30Z

Hi @TahaTesser, yes, I made some changes based on your comments. I'm not sure at this time how to fix the Linux errors that are happening. If you wanted to use any part of what I have done in your fix, be my guest.

TahaTesser · 2023-05-23T14:17:12Z

Hi @dleyba042 Please note: #123315 (comment)

Thanks for attempting to fix this, however several issues in your implementation.

Check out the Style guide for Flutter repo and see how increaseAction and decreaseAction is currently calling onChanged.

in order to fix this, first we need to move the clampDoubles from onChanged to separate functions then reuse it for the callback including in the action handler.

For tests, please check out other similar tests in the framework on how to title, format and test the functionality.

As mentioned, these tests had several format issues including some redundancy. Nevertheless given it is your first PR. I've fixed the tests for you.

TahaTesser

I just pushed rewritten tests. For this change, we don't need a new set of tests, the existing keyboard shortcuts are already testing onChanged callback. I just update these tests.

Finally, just this extra space needs to be removed.

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

TahaTesser

LGTM

…adability

TahaTesser · 2023-05-23T17:54:14Z

@Piinks
This needs a second review

Piinks

Hey @dleyba042, welcome! Thank you for contributing!
Appreciate the review @TahaTesser. This also LGTM! 🎉

dleyba042 · 2023-05-25T08:46:49Z

@TahaTesser sorry for the slow response currently on vacation. Thank you for the input and the learning opportunity! Thanks @Piinks it was a great learning experience.

…on keyboard shortcuts (flutter/flutter#126896)

Roll Flutter from 216605b to ee162e4 (53 revisions) flutter/flutter@216605b...ee162e4 2023-05-26 [email protected] Roll Packages from 995bfc5 to 9f8dcc5 (4 revisions) (flutter/flutter#127671) 2023-05-26 [email protected] Roll Flutter Engine from 474fcbbe445e to 8573f3b63a1f (1 revision) (flutter/flutter#127647) 2023-05-26 [email protected] Roll Flutter Engine from f63fcf5a274e to 474fcbbe445e (1 revision) (flutter/flutter#127644) 2023-05-26 [email protected] Roll Flutter Engine from b6fcbe31ba1a to f63fcf5a274e (1 revision) (flutter/flutter#127642) 2023-05-26 [email protected] Roll Flutter Engine from bec5da5bff7c to b6fcbe31ba1a (1 revision) (flutter/flutter#127641) 2023-05-26 [email protected] Roll Flutter Engine from 593d4d6b1f9b to bec5da5bff7c (2 revisions) (flutter/flutter#127638) 2023-05-26 [email protected] Roll Flutter Engine from f4dc96aef71b to 593d4d6b1f9b (1 revision) (flutter/flutter#127635) 2023-05-26 [email protected] Roll Flutter Engine from ff04d2fdd12b to f4dc96aef71b (2 revisions) (flutter/flutter#127633) 2023-05-26 [email protected] Remove a bad assert from tooltip implementation (flutter/flutter#127629) 2023-05-26 [email protected] Remove rounding from TextPainter (flutter/flutter#127099) 2023-05-26 [email protected] Adds `TestDisplay` API for testing `Display` features (flutter/flutter#127525) 2023-05-25 [email protected] Roll Flutter Engine from 6a4e675ab0b4 to ff04d2fdd12b (3 revisions) (flutter/flutter#127630) 2023-05-25 [email protected] Roll Flutter Engine from 99f2653cbb79 to 6a4e675ab0b4 (2 revisions) (flutter/flutter#127625) 2023-05-25 [email protected] Append $flutter/osx sdk property to existing platforms/targets relying on xcode (flutter/flutter#127537) 2023-05-25 [email protected] Indent markdown value on CP issue template (flutter/flutter#127623) 2023-05-25 [email protected] Roll Flutter Engine from 6ff02c17268f to 99f2653cbb79 (6 revisions) (flutter/flutter#127619) 2023-05-25 [email protected] `ProcessResultMatcher` created and used in test (flutter/flutter#127414) 2023-05-25 [email protected] Fix yaml lint issue (flutter/flutter#127600) 2023-05-25 [email protected] Roll Flutter Engine from 515a4bfc4b6e to 6ff02c17268f (1 revision) (flutter/flutter#127609) 2023-05-25 [email protected] `SearchBar` should not be impacted by overall `InputDecorationTheme` (flutter/flutter#127465) 2023-05-25 [email protected] Roll Flutter Engine from 195009b91abd to 515a4bfc4b6e (4 revisions) (flutter/flutter#127601) 2023-05-25 [email protected] Roll Flutter Engine from fb463217d1f3 to 195009b91abd (1 revision) (flutter/flutter#127591) 2023-05-25 [email protected] avoid incorrect usage of TimelineTask (flutter/flutter#127527) 2023-05-25 [email protected] Add `ScrollNotificationObserver` sample (flutter/flutter#127023) 2023-05-25 [email protected] Roll Packages from fba97fa to 995bfc5 (3 revisions) (flutter/flutter#127590) 2023-05-25 [email protected] Roll Flutter Engine from a74d9d1f4776 to fb463217d1f3 (1 revision) (flutter/flutter#127577) 2023-05-25 [email protected] Roll Flutter Engine from a7f026b03e35 to a74d9d1f4776 (1 revision) (flutter/flutter#127565) 2023-05-25 [email protected] Roll Flutter Engine from fb38bb6b1bc1 to a7f026b03e35 (1 revision) (flutter/flutter#127563) 2023-05-25 [email protected] Roll Flutter Engine from 2be736c390ee to fb38bb6b1bc1 (1 revision) (flutter/flutter#127560) 2023-05-25 [email protected] Remove Tooltip mouse tracker listener & update hovering/MouseRegion logic & animation (flutter/flutter#119199) 2023-05-25 [email protected] Roll Flutter Engine from 37ebad2d5c27 to 2be736c390ee (2 revisions) (flutter/flutter#127554) 2023-05-25 [email protected] Roll Flutter Engine from 62a83490ee60 to 37ebad2d5c27 (1 revision) (flutter/flutter#127551) 2023-05-25 [email protected] Roll Flutter Engine from 1bed04a4375a to 62a83490ee60 (3 revisions) (flutter/flutter#127546) 2023-05-25 [email protected] Roll Flutter Engine from 6a361090a367 to 1bed04a4375a (2 revisions) (flutter/flutter#127544) 2023-05-25 [email protected] Roll Flutter Engine from 9ba461efd3fe to 6a361090a367 (3 revisions) (flutter/flutter#127542) 2023-05-24 [email protected] Revert "Update labeler action wildcards" (flutter/flutter#127541) 2023-05-24 [email protected] [web] ui.platformViewRegistry => ui_web.platformViewRegistry (flutter/flutter#127493) 2023-05-24 [email protected] Roll Flutter Engine from c641f6307f08 to 9ba461efd3fe (6 revisions) (flutter/flutter#127536) 2023-05-24 [email protected] Add missing parameters in `TextFormField` (flutter/flutter#127020) 2023-05-24 [email protected] Update labeler action wildcards (flutter/flutter#127524) 2023-05-24 [email protected] Improve `TextPainter.layout` caching (flutter/flutter#118128) 2023-05-24 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 2713f7303c96cb1e69627957ec16eea0fd7f94a4 to 0776a679364a9a16110aac8d0f40f5e11009e327 (flutter/flutter#127533) 2023-05-24 [email protected] Fix ScrollPosition overscroll precision error (flutter/flutter#127321) 2023-05-24 [email protected] `Slider.onChangeStart` & `Slider.onChangeEnd` are not called on keyboard shortcuts (flutter/flutter#126896) 2023-05-24 [email protected] Manually roll Flutter Engine from 0c41b02cd5a6 to c641f6307f08 (flutter/flutter#127514) 2023-05-24 [email protected] Update CP template to include timeline and changelog (flutter/flutter#127507) ...

…on keyboard shortcuts (flutter/flutter#126896)

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 16, 2023

TahaTesser self-requested a review May 16, 2023 11:26

dleyba042 closed this May 19, 2023

dleyba042 reopened this May 19, 2023

This comment was marked as duplicate.

Sign in to view

TahaTesser requested changes May 23, 2023

View reviewed changes

packages/flutter/lib/src/material/slider.dart Outdated Show resolved Hide resolved

TahaTesser changed the title ~~Slider onChangeEnd is never called when arrow keys are used~~ Slider.onChangeStart & Slider.onChangeEnd are not called on keyboard shortcuts May 23, 2023

TahaTesser changed the title ~~Slider.onChangeStart & Slider.onChangeEnd are not called on keyboard shortcuts~~ Slider.onChangeStart & Slider.onChangeEnd are not called on keyboard shortcuts May 23, 2023

This comment was marked as duplicate.

Sign in to view

TahaTesser approved these changes May 23, 2023

View reviewed changes

dleyba042 and others added 8 commits May 23, 2023 18:09

fix for slider issue 123315

505fa95

splits up tests for more granularity

271d6ae

fixes spacing

7df0c49

refactors code based on feedback

fcdfde7

responds to feedback, creates functions for code reuse and ease of re…

c864b85

…adability

renames tests

4247b0d

Update slider_test.dart

3731cf5

Update packages/flutter/lib/src/material/slider.dart

5672ff8

TahaTesser force-pushed the b123315 branch from 205b92c to 5672ff8 Compare May 23, 2023 15:09

TahaTesser requested a review from Piinks May 23, 2023 17:54

Piinks approved these changes May 24, 2023

View reviewed changes

Piinks added the autosubmit Merge PR when tree becomes green via auto submit App label May 24, 2023

engine-flutter-autoroll mentioned this pull request May 25, 2023

Roll Flutter from 216605b60f67 to 20e164e60cb2 (27 revisions) flutter/packages#4077

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 25, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

eb4bdd5

…on keyboard shortcuts (flutter/flutter#126896)

engine-flutter-autoroll mentioned this pull request May 25, 2023

Roll Flutter from 216605b60f67 to 60a64a5a5c7f (28 revisions) flutter/packages#4078

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 25, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

20b6929

…on keyboard shortcuts (flutter/flutter#126896)

engine-flutter-autoroll mentioned this pull request May 25, 2023

Roll Flutter from 216605b60f67 to 3e610f7333f6 (29 revisions) flutter/packages#4079

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 25, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

28f095d

…on keyboard shortcuts (flutter/flutter#126896)

engine-flutter-autoroll mentioned this pull request May 25, 2023

Roll Flutter from 216605b60f67 to c05a05e6fcc0 (30 revisions) flutter/packages#4080

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 25, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

c519ae4

…on keyboard shortcuts (flutter/flutter#126896)

engine-flutter-autoroll mentioned this pull request May 25, 2023

Roll Flutter from 216605b60f67 to a21f33e7ad2f (31 revisions) flutter/packages#4081

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 25, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

768ee4e

…on keyboard shortcuts (flutter/flutter#126896)

engine-flutter-autoroll mentioned this pull request May 25, 2023

Roll Flutter from 216605b60f67 to e583ba4cf0b1 (32 revisions) flutter/packages#4082

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 25, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

9cf7b22

…on keyboard shortcuts (flutter/flutter#126896)

engine-flutter-autoroll mentioned this pull request May 25, 2023

Roll Flutter from 216605b60f67 to f71075db2eba (33 revisions) flutter/packages#4083

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 25, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

261ffec

…on keyboard shortcuts (flutter/flutter#126896)

engine-flutter-autoroll mentioned this pull request May 25, 2023

Manual roll Flutter from 216605b60f67 to 08266bdfcf11 (38 revisions) flutter/packages#4084

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 25, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

2e4dbf2

…on keyboard shortcuts (flutter/flutter#126896)

reidbaker mentioned this pull request May 25, 2023

Support minifcation for apps that depend on AGP 8 and integration_test #127628

Merged

8 tasks

engine-flutter-autoroll mentioned this pull request May 26, 2023

Roll Flutter from 216605b60f67 to 369192ae5049 (52 revisions) flutter/packages#4088

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 26, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

4843b68

…on keyboard shortcuts (flutter/flutter#126896)

engine-flutter-autoroll mentioned this pull request May 26, 2023

Roll Flutter from 216605b60f67 to ee162e4b7f4f (53 revisions) flutter/packages#4090

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 26, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

04ee3f7

…on keyboard shortcuts (flutter/flutter#126896)

reidbaker mentioned this pull request May 26, 2023

i127682 android api 33 test support #127689

Merged

8 tasks

HansMuller mentioned this pull request May 26, 2023

Flutter Framework Team: Notable Commits #121415

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 16, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

f89d2e7

…on keyboard shortcuts (flutter/flutter#126896)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

03cbffa

…on keyboard shortcuts (flutter/flutter#126896)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

3983ccb

…on keyboard shortcuts (flutter/flutter#126896)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 17, 2023

5451ea6 Slider.onChangeStart & Slider.onChangeEnd are not called …

cdf801d

…on keyboard shortcuts (flutter/flutter#126896)

Slider.onChangeStart & Slider.onChangeEnd are not called on keyboard shortcuts #126896

Slider.onChangeStart & Slider.onChangeEnd are not called on keyboard shortcuts #126896

Uh oh!

Conversation

dleyba042 commented May 16, 2023 • edited by TahaTesser Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

fixes #123315

Pre-launch Checklist

Uh oh!

google-cla bot commented May 16, 2023

Uh oh!

TahaTesser commented May 16, 2023

Uh oh!

TahaTesser commented May 22, 2023

Uh oh!

dleyba042 commented May 22, 2023

Uh oh!

TahaTesser commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as duplicate.

Uh oh!

TahaTesser left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as duplicate.

Uh oh!

TahaTesser left a comment

Choose a reason for hiding this comment

Uh oh!

TahaTesser commented May 23, 2023

Uh oh!

Piinks left a comment

Choose a reason for hiding this comment

Uh oh!

dleyba042 commented May 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

`Slider.onChangeStart` & `Slider.onChangeEnd` are not called on keyboard shortcuts #126896

`Slider.onChangeStart` & `Slider.onChangeEnd` are not called on keyboard shortcuts #126896

dleyba042 commented May 16, 2023 •

edited by TahaTesser

Loading

TahaTesser commented May 23, 2023 •

edited

Loading

TahaTesser left a comment •

edited

Loading