Add focus support for CupertinoActionSheetAction #166398 #167119
Conversation
|
It'll take me some time to review but first I want to say that this is a fascinating change! |
|
Gave another round of review. My apology that the review was a bit delayed due to the chaos around the US holiday lately. |
|
@/chunhtai is OOO until the end of the month, so we'll probably have to wait for him for further progress. Meanwhile I'll see if there's anything I can do. |
| : _borderRadius = BorderRadius.zero; | ||
|
|
||
| /// {@macro flutter.cupertino.CupertinoFocusHalo} | ||
| const CupertinoFocusHalo.onRRect({ |
There was a problem hiding this comment.
These names onRect and onRRect sound like they are meant to work with existing rectangles or rounded rectangles, but do they just decide to apply a border radius? It looks to me like a single constructor that takes an optional BorderRadius that defaults to BorderRadius.zero would be best here.
There was a problem hiding this comment.
Related to this, @dkwingsmt any thoughts?
There was a problem hiding this comment.
Oh I didn't see this comment! Sorry for the late reply.
I've talked with @justinmc offline and explained some design decisions I've proposed before for this PR. In short, I didn't want cornerRadius to appear on a unnamed constructor because it prioritizes RRects over other shapes (especially RSuperellipse, which will actually be the prominent one for Cupertino).
I also agree that onRRect isn't the most intuitive name and we can think of other options. I thought about rrect but the small letter may not be the correct grammar, as well as withRRect which might not fit the concept of halo very nicely. @justinmc agreed to think about naming with me.
|
Hi! @O-Hannonen Are you able to address the comments above so we can work together to get this landed? This is a great addition and I'm looking forward to it! |
…upertinoTraversalGroup
ded01ef to
350cf22
Compare
|
Sorry for the delay due to holidays. @dkwingsmt fixed the other issues but the one regarding the constructors, as there were conflicting requests from reviewers. |
| @@ -1420,6 +1442,8 @@ class CupertinoActionSheetAction extends StatefulWidget { | |||
|
|
|||
| class _CupertinoActionSheetActionState extends State<CupertinoActionSheetAction> | |||
| implements _SlideTarget { | |||
| bool _isFocused = false; | |||
There was a problem hiding this comment.
| bool _isFocused = false; | |
| bool _showHighLight = false; |
There was a problem hiding this comment.
IMO the original one that talks about focus is more descriptive in this context and also same wording is used in many other places, so keeping as is for now. Feel free to open discussion again if you strongly disagree
There was a problem hiding this comment.
This property is set in _onShowFocusHighlight which means the code tries to infer has focus highlight -> is focused.
The property is then used to decide whether to draw the highlight.
I found it unnecessary to infer hashighlight to isfocused, and then infer it back to highlight in the build.
…rd-focus-support-for-cupertino-action-sheet-action
| @@ -1420,6 +1442,8 @@ class CupertinoActionSheetAction extends StatefulWidget { | |||
|
|
|||
| class _CupertinoActionSheetActionState extends State<CupertinoActionSheetAction> | |||
| implements _SlideTarget { | |||
| bool _isFocused = false; | |||
There was a problem hiding this comment.
This property is set in _onShowFocusHighlight which means the code tries to infer has focus highlight -> is focused.
The property is then used to decide whether to draw the highlight.
I found it unnecessary to infer hashighlight to isfocused, and then infer it back to highlight in the build.
This PR fixes #166398
CupertinoActionSheetAction. This makes it work with keyboard shortcutsCupertinoTraversalGroupthat applies a Cupertino style focus border around its child when any of its descendant has focusCupertinoTraversalGroupinCupertinoActionSheetHow the new implementation looks and behaves:
https://github.com/user-attachments/assets/ea6789f1-921d-4598-bcca-489dc063ff73
How the native counterpart looks and behaves:
https://github.com/user-attachments/assets/4c6ae2a0-7205-4de2-b981-ec7f4839da6e
Pre-launch Checklist
///).