Android context menu theming and visual update #131816

justinmc · 2023-08-02T21:49:37Z

Fixes #89939 and updates the look of the Android context menu to match API 34.

The problem

Before this PR, setting surface in the color scheme caused the background color of the Android context menu to change, but it wasn't possible to change the text color.

MaterialApp(
  theme: ThemeData(
    // Using a dark theme made the context menu text color be white.
    colorScheme: ThemeData.dark().colorScheme.copyWith(
      // Setting the surface here worked.
      surface: Colors.white,
      // But there was no way to set the text color. This didn't work.
      onSurface: Colors.black,
    ),
  ),
),

Expected (after PR)	Actual (before PR)

Other examples

Scenario	Result
MaterialApp( theme: ThemeData( colorScheme: ThemeData.light(), ), ... ),
MaterialApp( theme: ThemeData( colorScheme: ThemeData.dark(), ), ... ),
MaterialApp( theme: ThemeData( colorScheme: ThemeData.light().colorScheme.copyWith( surface: Colors.blue, onSurface: Colors.red, ), ), ... ),
MaterialApp( theme: ThemeData( colorScheme: ThemeData.light().colorScheme.copyWith( surface: Colors.blue, onSurface: Colors.red, ), ), ... ),

justinmc · 2023-08-02T22:37:24Z

@QuncCccccc This is based on your suggestion in #89939 (comment) (thank you!).

One question though: What if we want to deviate from the default surface colors by default?

I noticed that we're getting the default background color wrong in dark mode right now. What if I want to fix that while still allowing users to customize the colors using ColorScheme's surface and onSurface?

Native Android	Flutter Android

QuncCccccc · 2023-08-02T23:39:16Z

One question though: What if we want to deviate from the default surface colors by default?

I noticed that we're getting the default background color wrong in dark mode right now. What if I want to fix that while still allowing users to customize the colors using ColorScheme's surface and onSurface?

Ah this would be a little bit tricky. Ideally, we can use a propertyColor ?? themeColor ?? defaultColor pattern to assign a color for the backgroundColor. If we do a similar thing here, it would be backgroundColor: theme.colorScheme.surface ?? defaultAndroidColor, but the problem is theme.colorScheme.surface will never return null, so the default color for native android can never be reached.🥲

I once added a similar fix for IconButton. We can check if the color is a default color scheme color, if it is, then we should use our own default color(native android). If it's not, we should use the customized surface color. So for this problem, I think we can firstly determine if there is a customized surface color in ColorScheme. We can compare the color Theme.of(context).colorScheme.surface with ThemeData().colorScheme.surface/ThemeData.dark().colorScheme.surface. If they are the same object, it means no custom surface exists, then we can use the native android background color; otherwise, we should use the custom surface.

The code change should look like this(I assumed a pink color for the native color😛):

Please let me know if there's any confusion or problems!

QuncCccccc

LGTM:)

…rial-colors

flutter-dashboard · 2023-08-04T23:26:26Z

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #131816 at sha 6d2208a

justinmc · 2023-08-07T18:53:05Z

@QuncCccccc Would you mind reviewing this again? I changed the PR to fully match the colors as well as font weight and border radius. I noticed that there were goldens breakages, so I thought it would be better to update all of this at once rather than break the goldens multiple times in multiple PRs. I update the description and title of the PR to match the increased scope.

Also, I took your advice from above about allowing the override of default values, so it will now be possible to change the background and foreground colors with surface and onSurface in the color scheme.

In these screenshots, the top is native and the bottom is Flutter.

	Light	Dark
With this PR
Before this PR

flutter-dashboard · 2023-08-07T19:27:16Z

Golden file changes are available for triage from new commit, Click here to view.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Changes reported for pull request #131816 at sha 6bd2f3d

justinmc · 2023-08-07T19:57:08Z

Golden changes all looked expected so I accepted them 👍

QuncCccccc · 2023-08-07T19:58:51Z

@QuncCccccc Would you mind reviewing this again? I changed the PR to fully match the colors as well as font weight and border radius. I noticed that there were goldens breakages, so I thought it would be better to update all of this at once rather than break the goldens multiple times in multiple PRs. I update the description and title of the PR to match the increased scope.

I see! Reviewing now!

QuncCccccc

LGTM!

justinmc · 2023-08-07T20:16:32Z

Thanks! And thanks for all of your help figuring out how to approach this.

…er#131816)

flutter/flutter@ad0aa8d...436df69 2023-08-08 [email protected] Roll Flutter Engine from 9c83d90b01bd to 146c4c9487fc (6 revisions) (flutter/flutter#132112) 2023-08-08 [email protected] Roll Flutter Engine from c27109291e22 to 9c83d90b01bd (5 revisions) (flutter/flutter#132108) 2023-08-08 [email protected] Roll Flutter Engine from be085f6699b6 to c27109291e22 (3 revisions) (flutter/flutter#132086) 2023-08-08 [email protected] Revert "Replace TextField.canRequestFocus with TextField.focusNode.canRequestFocus" (flutter/flutter#132104) 2023-08-08 [email protected] [Impeller] add drawVertices and drawAtlas benchmarks. (flutter/flutter#132080) 2023-08-07 [email protected] Adds more documentations around ignoreSemantics deprecations. (flutter/flutter#131287) 2023-08-07 [email protected] [web] New HtmlElementView.fromTagName constructor (flutter/flutter#130513) 2023-08-07 [email protected] Move mock canvas to flutter_test (flutter/flutter#131631) 2023-08-07 [email protected] Add static_path_tessellation macrobenchmark (flutter/flutter#131837) 2023-08-07 [email protected] [web] Remove usage of `ui.webOnlyInitializePlatform()` (flutter/flutter#131344) 2023-08-07 [email protected] Roll Flutter Engine from 39a575f65d50 to be085f6699b6 (1 revision) (flutter/flutter#132069) 2023-08-07 [email protected] Android context menu theming and visual update (flutter/flutter#131816) 2023-08-07 [email protected] Roll Flutter Engine from 39ce1c097bce to 39a575f65d50 (2 revisions) (flutter/flutter#132064) 2023-08-07 [email protected] CupertinoContextMenu improvement (flutter/flutter#131030) 2023-08-07 [email protected] Roll Flutter Engine from 5b47c0577060 to 39ce1c097bce (3 revisions) (flutter/flutter#132057) 2023-08-07 [email protected] Slider should check `mounted` before start interaction (flutter/flutter#132010) 2023-08-07 [email protected] Roll Packages from ce53da1 to d7ee75a (7 revisions) (flutter/flutter#132058) 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 [email protected],[email protected],[email protected] 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

…er#131816)

justinmc self-assigned this Aug 2, 2023

github-actions bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Aug 2, 2023

Use onSurface color for context menu buttons

1019566

justinmc force-pushed the context-menu-material-colors branch from 46899bb to 1019566 Compare August 2, 2023 22:15

justinmc changed the title ~~Use onSurface color for context menu buttons~~ Fix Material context menu colors as they relate to the Theme Aug 2, 2023

Tests

d7ad8af

justinmc requested a review from QuncCccccc August 2, 2023 22:34

justinmc marked this pull request as ready for review August 2, 2023 22:34

QuncCccccc approved these changes Aug 2, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/master' into context-menu-mate…

6d2208a

…rial-colors

flutter-dashboard bot added the will affect goldens Changes to golden files label Aug 4, 2023

justinmc added 6 commits August 7, 2023 10:40

Correct default and overridable background and foreground colors

7e64368

Round the toolbar edges

53570fe

Font weight matched

7e90fdb

Analyzer

c9f938a

Fix toolbar text button test

e222d99

Test the background color

db70df7

justinmc changed the title ~~Fix Material context menu colors as they relate to the Theme~~ Android context menu theming and visual update Aug 7, 2023

justinmc requested a review from QuncCccccc August 7, 2023 18:53

Analyzer

6bd2f3d

QuncCccccc approved these changes Aug 7, 2023

View reviewed changes

justinmc added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 7, 2023

auto-submit bot merged commit ebbb4b3 into flutter:master Aug 7, 2023

justinmc deleted the context-menu-material-colors branch August 7, 2023 21:01

engine-flutter-autoroll mentioned this pull request Aug 8, 2023

Roll Flutter from ad0aa8de7512 to 436df69a4684 (17 revisions) flutter/packages#4663

Merged

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

ebbb4b3 Android context menu theming and visual update (flutter/flutt…

0edbaa1

…er#131816)

HansMuller mentioned this pull request Aug 11, 2023

Flutter Framework Team: Notable Commits #121415

Closed

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

ebbb4b3 Android context menu theming and visual update (flutter/flutt…

74070d3

…er#131816)

Abestanis mentioned this pull request May 27, 2024

Update to new flutter version nt4f04uNd/nt4f04unds_widgets#12

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Android context menu theming and visual update #131816

Android context menu theming and visual update #131816

Uh oh!

justinmc commented Aug 2, 2023 •

edited

Loading

Uh oh!

justinmc commented Aug 2, 2023

Uh oh!

QuncCccccc commented Aug 2, 2023

Uh oh!

QuncCccccc left a comment

Uh oh!

flutter-dashboard bot commented Aug 4, 2023

Uh oh!

justinmc commented Aug 7, 2023

Uh oh!

flutter-dashboard bot commented Aug 7, 2023

Uh oh!

justinmc commented Aug 7, 2023

Uh oh!

QuncCccccc commented Aug 7, 2023

Uh oh!

QuncCccccc left a comment

Uh oh!

justinmc commented Aug 7, 2023 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Android context menu theming and visual update #131816

Android context menu theming and visual update #131816

Uh oh!

Conversation

justinmc commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The problem

Other examples

Uh oh!

justinmc commented Aug 2, 2023

Uh oh!

QuncCccccc commented Aug 2, 2023

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

flutter-dashboard bot commented Aug 4, 2023

Uh oh!

justinmc commented Aug 7, 2023

Uh oh!

flutter-dashboard bot commented Aug 7, 2023

Uh oh!

justinmc commented Aug 7, 2023

Uh oh!

QuncCccccc commented Aug 7, 2023

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

justinmc commented Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

justinmc commented Aug 2, 2023 •

edited

Loading

justinmc commented Aug 7, 2023 •

edited

Loading