webstudio-is / webstudio

Open source website builder and Webflow alternative. An advanced visual builder that connects to any headless CMS, supports all CSS properties, and can be hosted anywhere, including with us.
https://webstudio.is
GNU Affero General Public License v3.0
4.57k stars 509 forks source link

Bug: Fix styles for focus-states management for CSValueListItems #2543

Open JayaKrishnaNamburu opened 10 months ago

JayaKrishnaNamburu commented 10 months ago

https://github.com/webstudio-is/webstudio/assets/11075561/28b9f3c9-f2e4-47c6-b52a-27623700ee94

When opening any CSSValueListItem, and then closing back the panel. The focus is delegated back from the content box back to the CSSValueListItem. And so the actions show/hide and delete are highlighted. As according to how the component is configured.

const ItemWrapper = styled("div", {
  position: "relative",
  width: "100%",
  "&:hover, &[data-active=true]": {
    [`& ${ItemButton}`]: {
      backgroundColor: theme.colors.backgroundHover,
    },
    [`& ${DragHandleIconStyled}`]: {
      visibility: "visible",
    },
  },
  "&:hover, &[data-active=true], &:focus-within": {
    [`& ${IconButtonsWrapper}`]: {
      visibility: "visible",
    },
  },
});

So, the open questions right now.

taylornowotny commented 10 months ago

Thanks for the thorough review of this bug @JayaKrishnaNamburu . I am not 100% sure of the ideal UX here so let's discuss that with @kof

Answering your questions:

  1. Yes, each List Item should be focusable with TAB. Currently you have to use up/down arrow keys to move focus to the next list item, but I think TAB might be a better UX for this component. WYT @kof ?

  2. Yes when the menu closes, focus should return to the item. However I'm not sure that we need to show the focus state at this point.

@kof Is this the way all controls that open menus should work?

kof commented 10 months ago

@taylornowotny tab is focsing the next thing, within list focus can be changed using arrow keys, this is the official pattern for lists https://www.w3.org/WAI/ARIA/apg/patterns/listbox/

Otheriwse if the list is long enough you have to tab several times.

kof commented 10 months ago

Yes when the menu closes, focus should return to the item. However I'm not sure that we need to show the focus state at this point.

I have no idea how we managed to keep the focus but NOT show the outline, because these things should be connected

kof commented 10 months ago

I think @istarkov might know more about the focus behavior there as I think he built that list for backgrounds initially. Item is definitely focused after closing, but somehow outline is not visible and icons are visible, I think really the missing piece is making sure outline is visible whenever the item is focused

istarkov commented 10 months ago

Outline is not always shown on focus. As an example https://developer.mozilla.org/en-US/docs/Web/CSS/:focus-visible

The :focus-visible pseudo-class applies while an element matches the :focus pseudo-class and the UA (User Agent) determines via heuristics that the focus should be made evident on the element. (Many browsers show a "focus ring" by default in this case.)

Sometimes heuristic in case of focus-visible is not working like we expect. Probably this is the issue

taylornowotny commented 10 months ago

Thank you @istarkov That's what I was trying to say

It feels awkward if a menu trigger gets visibly focused when the menu is closed.

This is the ideal in my mind - is this reasonable?