InkWell can show focusColor optional on touch device #70442
Conversation
flutter-dashboard
bot
added
f: material design
|
Hi @xu-baolin, Thanks for the PR, but I'd rather not add this parameter to |
|
But this doesn't seem to work for this issue scenario. I just want to let some widget response instead of making all of them respond focus change. |
HansMuller
changed the title
|
I'd prefer to avoid adding one-off flags like alwaysShowFocusColor. It's not possible to use the FocusManager, since it's a singleton and can't be used to override highlightStrategy within a subtree (i.e. for just one DropdownButton's menu). I think what's really needed is a way to specify how menu item highlights are computed for the selected/focused/etc states. I think we could do that without hijacking the focus manager's highlightMode property, by exposing the per-item InkWell overlayColor property in DropdownButton and by folding in the MaterialState.selected state, so overlayColor ( |
| switch (FocusManager.instance.highlightMode) { | ||
| case FocusHighlightMode.touch: | ||
| showFocus = false; | ||
| showSelect = true; |
There was a problem hiding this comment.
For touch devices, do not display the focus color, try to display the selected color(if set by overlayColor) instead.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
Why not "expose the per-item InkWell overlayColor property in DropdownButton" so that the color of menu items for all states could be specified with |
Done. |
|
How would a developer override the default overlayColor you've added? |
Override the ThemeData for the subtree, the same as before this change. |
|
Hi guys, Any thoughts about this patch? |
|
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. |
|
I appreciate the work you've put into this PR, and I apologize for only being a "drive by" reviewer. I think we're going to need to sort out a general solution per #70442 (comment), for all components that can take the focus when they're used in desktop applications. I'm going to close this PR to give everyone a chance to think about how best to solve (and explain) a general solution. Sorry about the long delay and for the effort you've put into this. |
Fix #70294 ### Description The bug is about the previously selected DropdownMenuItem isn't highlighted on mobile while it works on Web and desktop. The reason is that `FocusManager.instance.highlightMode` has fallen to `FocusHighlightMode.touch` on mobile by default (or touch device without mouse in general): https://github.com/flutter/flutter/blob/6f220d3ec0948a4def1622de805fabda9855338d/packages/flutter/lib/src/material/dropdown.dart#L172-L175 https://github.com/flutter/flutter/blob/7d56be4c8de814022357633179a816b51ffca4b8/packages/flutter/lib/src/widgets/focus_manager.dart#L2335-L2353 https://github.com/flutter/flutter/blob/7d56be4c8de814022357633179a816b51ffca4b8/packages/flutter/lib/src/widgets/focus_manager.dart#L1741-L1748 We actually can work around this by setting `FocusManager.instance.highlightStrategy` to `FocusHighlightStrategy.alwaysTraditional`, see #70442 (comment). Nevertheless, this approach will may affect the entire scope of the app, potentially resulting in unforeseen behavior for other widgets. In this PR, I propose a straightforward solution that utilizes an ancestor widget (Ink) to manage the color state corresponding to `InkWell.overlayColor`. ### Demo the fix https://github.com/user-attachments/assets/b6476732-716c-4f39-ab82-7ab5accb7182 ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md Signed-off-by: huycozy <[email protected]>
5 participants
Description
Currently, the focus highlight can only be shown when
[FocusManager.highlightMode]is[FocusHighlightMode.traditional],Sometimes, showing the focus highlight is useful for the touch device widget, such as highlight the selected item of
DropdownButton, so I add a propertyalwaysShowFocusColorofInkWellto make it optinal.I also improve some documentations about
focusColorand[FocusManager.highlightMode]behavior.Related Issues
Fixes #70294
Tests
I added the following tests:
See files.