[cupertino.dart] Implement CupertinoMenuAnchor and CupertinoMenuItem using RawMenuAnchor #174695
Conversation
github-actions
bot
added
framework
|
@dkwingsmt Heads up, a somewhat large change to menu item layout will land soon, so you may want to avoid reviewing that section for a day or so. |
|
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. |
|
Oh @dkwingsmt this should be ready for review |
|
Awesome! I'll review it after the Thanksgiving. |
dkwingsmt
left a comment
dkwingsmt
left a comment
There was a problem hiding this comment.
Generally LGTM. Mostly minor suggestions and doc improvements. Thank you!
| /// The following constant represents a division in text scale factor beyond which | ||
| /// we want to change how the menu is laid out. | ||
| /// | ||
| /// This explanation was ported from CupertinoDialog. |
There was a problem hiding this comment.
Do you mean CupertinoAlertDialog? (Maybe use cupertino/dialog.dart instead since the said explanation does not technically belong to the class.)
There was a problem hiding this comment.
Yup, whoops. Done! Good catch.
| } | ||
| } | ||
|
|
||
| // The font size at which text scales linearly on the iOS 18.5 simulator. |
There was a problem hiding this comment.
I reworded the doc here and mentioned "unit" here so that followup explanations are easier to understand. LMK what you think.
| // The font size at which text scales linearly on the iOS 18.5 simulator. | |
| // Base font size used for text-scaling calculations. | |
| // | |
| // On iOS the text scale changes in increments of 1/17 (≈5.88%), as | |
| // observed on the iOS 18.5 simulator. Each step (1/17 of the base font size) | |
| // is referred to as one "unit" in the following calculations. | |
| // The font size of 17.0 pt is referred to as the "base font size". |
If you agree, remove the relevant parts in the doc of _normalizeTextScale.
There was a problem hiding this comment.
That makes sense. Done, except I removed the "The font size of 17.0 pt is referred to as the 'base font size'" since we already say "Base font size used for text-scaling calculations." Lmk if I should add it back in.
| final double units = | ||
| textScaler.scale(_kCupertinoMobileBaseFontSize) - _kCupertinoMobileBaseFontSize; |
There was a problem hiding this comment.
Maybe
| final double units = | |
| textScaler.scale(_kCupertinoMobileBaseFontSize) - _kCupertinoMobileBaseFontSize; | |
| final double units = _normalizeTextScale(textScaler); |
| /// * Max "regular" scale factor ≈ 23/17 ≈ 1.352... (6 units) | ||
| /// * Min "accessible" scale factor ≈ 28/17 ≈ 1.647... (11 units) |
There was a problem hiding this comment.
| /// * Max "regular" scale factor ≈ 23/17 ≈ 1.352... (6 units) | |
| /// * Min "accessible" scale factor ≈ 28/17 ≈ 1.647... (11 units) | |
| /// * Max "regular" scale factor ≈ 23/17 ≈ 1.352... (normalized text scale: 6) | |
| /// * Min "accessible" scale factor ≈ 28/17 ≈ 1.647... (normalized text scale: 11) |
| /// | ||
| /// The returned value is positive when the text scale factor is larger than the | ||
| /// base font size, negative when smaller, and zero when equal. | ||
| double _normalizeTextScale(TextScaler textScaler) { |
There was a problem hiding this comment.
nit: move this to just below _kCupertinoMobileBaseFontSize so that it's clearer what "normalized text scale" is.
| /// | ||
| /// On all platforms except web, this color is applied to the divider before | ||
| /// the [color] is applied, and is used to give the appearance of the divider | ||
| /// cutting into the background. |
There was a problem hiding this comment.
Can you further explain what "cutting into the background" means?
| size: Size(double.infinity, displacement), | ||
| painter: _AliasedLinePainter( | ||
| overlayColor: CupertinoDynamicColor.resolve(overlayColor, context), | ||
| border: BorderSide(width: 0.0, color: CupertinoDynamicColor.resolve(color, context)), |
There was a problem hiding this comment.
Why do we need to define it with border instead of simply another color?
There was a problem hiding this comment.
Color works. Should be done!
| /// The horizontal space in which the [trailing] widget can be placed. | ||
| final double? trailingWidth; | ||
|
|
||
| /// The alignment of the center point of the leading widget within the |
There was a problem hiding this comment.
So why do we want to align the leading/trailing widgets this special way?
There was a problem hiding this comment.
I added a comment on the _AlignMidpoint widget. This matches how iOS lays out it's leading and trailing widgets: https://developer.apple.com/documentation/uikit/uiview/centerxanchor. AFAIK there's currently no way to simply achieve this behavior with regular Flutter widgets.
| /// When the inner menu button is disabled, [longPressToOpenDuration] should | ||
| /// be set to [Duration.zero] to prevent the menu from opening on long-press. | ||
| /// | ||
| /// Defaults to [Duration.zero], which disables the behavior. |
There was a problem hiding this comment.
Shall we remove disabling in this PR, so that we don't have to break this behavior later?
There was a problem hiding this comment.
I would provide some method of disabling. If a user wants to long-press-and-drag the menu anchor around the screen, they may want to disable longPressToOpen.
There was a problem hiding this comment.
I ended up removing longPressToOpenDuration, adding enableLongPressToOpen, and adding a _CupertinoMenuAnchorState.longPressToOpenDuration constant. I figure we can always add a way to customize the duration if needed.
| } | ||
|
|
||
| /// Handles swiping events for a [_SwipeRegion]. | ||
| // This class was adapted from _DragAvatar. |
There was a problem hiding this comment.
I recommend not mentioning connections like this, because such connections can be outdated in the future as one of them is updated.
2 participants
This PR implements
CupertinoMenuAnchor,CupertinoMenuItem,CupertinoLargeMenuDividerusingRawMenuAnchor.Resolves #60298, notDmDrl/pull_down_button#26, #137936.
This repo is a rough draft. Some of the tests are failing due to code changes. Comments may not accurately reflect the latest version of
CupertinoMenuAnchor. Subjective decisions had to be made due to limits of the Flutter engine. This PR was extracted from a multi-layer menu, so extraneous code may still be present.One large decision that I'd like feedback on is whether CupertinoMenuAnchor should receive a single panel widget rather than individual children.
I'll be busy this weekend, so excuse my lack of response to questions/comments.
Screen.Recording.2025-08-29.at.7.17.12.AM.mov
Dartpad: https://dartpad.dev/8c6bba779b6a00e95582b61b132292bc
@dkwingsmt
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.