2043465 - Simplify and consolidate colors usage in the Menu using Default objects

Status: ASSIGNED

(Firefox for Android :: Settings, task, P3)

Description

[Gabriel Luong [:gl] (ΦωΦ)]

After the changes in Bug 2035380, we should evaluate adding objects such as MenuDefaults, MenuItemDefaults, etc that can allow us to better handle consolidating the colors use in the Menu such that we only ever have to change the color in a single place and can expect that the Menu UI components will respond accordingly. This can apply to MenuItem, Badges, and other commonly reused component found in the Menu.

This might be more of an exploration at times to gather feedback on what would be the best developer ergonomics and how we can improve the Menu components.

Comment 1

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 2043465 - Part 1: Refactor menu item background colors to use MenuDefaults.backgroundColor

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 2043465 - Part 2: Refactor surfaceContainerHigh usages in menu to use MenuDefaults.containerColor

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 2043465 - Part 3: Add Compose previews for ExtensionsMenuItem, WebExtensionMenuItems and ProtectionPanelBanner

Comment 4

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 2043465 - Part 3: Refactor menu badge background to use MenuDefaults.badgeColor

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Bug 2043465 - Part 4: Simplify MenuNavigation content color

Comment 6

[Petru-Mugurel Lingurar [:petru]]

Looking over the patches I see an improvement but a minimal one.

MenuDefaults consolidates the 3 tokens so it's a bit easier to for new menu items to know which such token to use
But configuring menu items with those tokens, the clip + background pair, content colour, shape is still copy-pasted at every call site
So it is still easy for these to drift or for new items to just use something different.

An improvement on this might be to consolidate the component, not the tokens, so that a different styling will require more work and so it will be a conscious choice to have a different UI.
One way of achieving this would be something like

  @Composable                                                                                                                                                                                                                            
  internal fun MenuSurface(                                                                                                                                                                                                              
      modifier: Modifier = Modifier,                                                                                                                                                                                                     
      shape: Shape = MaterialTheme.shapes.extraSmall,                                                                                                                                                                                    
      color: Color = MenuDefaults.backgroundColor,                                                                                                                                                                                       
      contentColor: Color = contentColorFor(color),                                                                                                                                                                                      
      content: @Composable () -> Unit,                                                                                                                                                                                                   
  ) = Surface(modifier, shape, color, contentColor, content = content

One caveat for this would be that MenuItem delegates to AC's IconListItem/TextListItem and applies the background via a modifier, so a few items can't be trivially wrapped — they'd keep Modifier.menuItemBackground().

Alternatives to this like a new modifier

  fun Modifier.menuItemBackground(                                                                                                                                                                                                       
      shape: Shape = ...,                                                                                                                                                         
      color: Color = ...,                                                                                                                                                                                                                
  ) = clip(shape).background(color)

or using something like this at the menu root

CompositionLocalProvider(LocalMenuColors provides menuColors()) { MenuContent() }

would work but these would still allow items to avoid reading/using the new APIs so we'd still have no guarantee that the intended style is always used.

Curious of your view of this and also of Noah's as this would introduce a new pattern for standardizing theming in the app.

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

My 2 cents here. I originally started with MenuItemDefaults, which only consolidated the colors for the MenuItem. At some point, I got a bit lost in consolidating things and turned MenuItemDefaults to MenuDefaults. I got to point where I edited the original bug description to say this is now more of a exploration and throwing ideas out there to get feedback and see what stick.

I like the idea of consolidating a per-component level here instead of what I tried to do which was consolidated the whole Menu component in one. This would align more along with M3 pattern of having defaults per component like ButtonDefaults and SwitchDefaults. This will still be an incremental improvement. One caveat here is that I don't want us over-inflate the scope, but be able to provide incremental steps toward that end goal that we might happy with because we are not using this bug to overhaul everything. Even though I found that there are opportunities to overhaul things, but it's likely need to happen incrementally.

Comment 8

[Noah Bond [:007]]

If we treat the Menu as a "feature", I think it makes sense to have feature-scoped reusable components and tokens/defaults. If there were a sweeping amount of defaults and/or we wanted the extra granular control of component defaults, a MenuDefaults object could then contain MenuXComponentDefaults objects (or they could be hierarchically separate).

For an invocation syntax, I think keeping with the pattern of using functional getters makes the most sense. We don't need something as heavy handed, or universally accessible, as a CompositionLocal in this case, since it's only the Menu's components that need this and not subcomponents within.

object MenuDefaults {

    ...
    val backgroundColor: Color
        @Composable
        @ReadOnlyComposable
        get() = MaterialTheme.colorScheme.surfaceBright
}

Comment 9

[Phabricator Automation]

Comment on attachment 9591891 [details]
Bug 2043465 - Part 3: Add Compose previews for ExtensionsMenuItem, WebExtensionMenuItems and ProtectionPanelBanner

Revision D303637 was moved to bug 2047477. Setting attachment 9591891 [details] to obsolete.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 1 week.
:gl, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.