adobe / spectrum-css

The standard CSS implementation of the Spectrum design language.
http://opensource.adobe.com/spectrum-css/
Apache License 2.0
1.2k stars 195 forks source link

`spectrum-Menu-item` label alignment subject to cascade of text-align CSS property #3058

Open Rocss opened 2 months ago

Rocss commented 2 months ago

Description

Whenever text-align is used on a parent element, all sp-menu-item labels inherit that instead of explicitly defining start.

Steps to reproduce

  1. Open any UI with a sp-menu
  2. Manually add text-align:center to .spectrum-Menu
  3. Observe menu items are centered now

Expected behavior

Menu items should not be centered and should perserve the initial text alignment

Screenshots

Screenshot 2024-08-30 at 16 39 08

Environment

Additional context

Additional JIRA information: CCEX-136334

castastrophe commented 1 month ago

Hi @Rocss! Thanks so much for reaching out with your issue. Can I ask what was the original goal in adding the text-align: center to the .spectrum-Menu element in your UI? May I suggest adding the text align property close to the element you are wanting to center? Perhaps if it's a heading to be centered, the text align could be set on the container for that heading, rather than a higher-level DOM element like the menu wrapper?

In our library, we would expect inline or CSS that is added by the application to always take precedence over the CSS assigned by the library. Ours is a utility but not meant to force styles or make them too difficult to override. We also offer a set of modifier custom properties that can be used to customize select parts of the UI. You can find those for Menu here: https://github.com/adobe/spectrum-css/blob/main/components/menu/metadata/mods.md.

Please do reach out if we can be of more assistance!

lazd commented 1 month ago

Hey @castastrophe, this is a regression only became an issue after the Overlay V2 changes since overlays being inserted adjacent to the trigger. This bug will continue to happen in menus in similar situations. sp-menu-item should be explicit about its text alignment to address this, and if we want it to be possible to override the text alignment, that should be done through a --mod-* token, not the cascade.

It's a simple change and the team would be happy to provide a PR if y'all would accept it.

Rocss commented 3 weeks ago

@castastrophe After Larry's comment, would you please let us know if this is something you consider adding to the library?

pfulton commented 3 weeks ago

@Rocss thanks for following up with us on this.

With regard to what @lazd mentioned:

...the Overlay V2 changes since overlays being inserted adjacent to the trigger. This bug will continue to happen in menus in similar situations.

We'd love to understand this more and maybe see the DOM structure you're describing. Would you be able to spin up a small reproduction of the problem/use case?

I'm sure we all know how complex these menu items can get, so we want to be sure that we fully understand the impact of the change.

Thanks!

Rocss commented 2 weeks ago

@pfulton yep, here's one: https://studio.webcomponents.dev/edit/uxXuEl5ZlqeQqxT0dWq3/src/index.ts?p=stories