search

patternfly / patternfly-react

A set of React components for the PatternFly project.
https://react-staging.patternfly.org/
MIT License
761 stars 349 forks source link

Bug - Menus (dropdown, menutoggle, etc.) - first menu item in a dropdown has a highlight by default (and it shouldn't) #10393

Open garrett opened 2 months ago

garrett commented 2 months ago

Describe the problem This is an issue with

[?] Patternfly 6

Please give a clear and concise description of the problem. Which components are affected?

When clicking on a dropdown or menutoggle, the first item in a menu is already selected.

How do you reproduce the problem?

Open any dropdown, menutoggle, or other menu-based component and look at the first menu item. It looks like it's being hovered, but it isn't. This affects all the new menus on the PatternFly website as well.

Expected behavior

An item that isn't hovered or focused shouldn't look hovered or focused.

Is this issue blocking you?

We had this as part of a blocker for our release, as it's a regression from the previous (now deprecated) dropdowns and we recently ported to the new ones.

Screenshots

From PatternFly's website:

image

image

image

image

image

This affects Chrome too:

image

And also WebKit:

image

What is your environment?

What is your product and what release date are you targeting?

Cockpit. Yesterday was the release. We're unfortunately now shipping with this bug, as we cannot indefinitely postpone the release to wait on a fix in a PatternFly release. :frowning_face:

thatblindgeye commented 2 months ago

@garrett thanks for bringing this to our attention!

For what it's worth, the first item upon clicking open a toggle is being focused rather than being styled to look hovered. The focus and hover styles are the same, though, which may be adding to this confusion. This functionality was added in React in a PR to focus the first item when opened by click, which helped resolve an issue regarding not being able to traverse the menu items via keyboard after clicking the toggle.

With that in mind does that help alleviate the concern with this behavior, or is the behavior still unexpected and/or confusing?

martnaum commented 2 months ago

This also seems to cause a problem with fields that allow text input via the keyboard. Previously, we were able to click directly into the field and filter the displayed elements via keyboard input. This is now only possible after an additional click in the field. You can easily see that by comparing the UX of https://www.patternfly.org/components/menus/select/#typeahead to:

garrett commented 3 weeks ago

Any news on this being fixed?

We still see this all the time in Cockpit, and would love for this regression to be fixed... and in PF5 (where it was introduced in April), not just PF6.

See this dropdown where the first one is selected and the one at the bottom has a highlight:

image

It's really quite bad and confusing to have the same focus and hover stylings and have two items selected at the same time.

thatblindgeye commented 3 weeks ago

@garrett we currently have https://github.com/patternfly/patternfly-react/pull/10642 open for v5 that will allow disabling the auto-focus, but will require manual logic to focus an item (which will be needed when the dropdown is opened via keyboard at the very least). This would more help resolve scenarios where a dropdown may have something other than dropdown items (e.g. an input before the dropdown items) that should receive focus first. The issue with the hover/focus styling being the same may not be something we land for v5 since it's a topic that stretches beyond just Dropdown styling, though.

We also have https://github.com/patternfly/patternfly-react/issues/10543 for a post-v6 spike to investigate a more robust/customizable way to handle the autofocus, though again that wouldn't address the styling being the same for hover/focus.

garrett commented 3 weeks ago

Thanks for the response!

The issue with the hover/focus styling being the same may not be something we land for v5

It's a very visible, confusing regression that happened in a v5 point release. Projects cannot simply update to v6, as it introduces all kinds of breakages, including a radically different style that has fonts that are illegibly small. It will break all visual tests for projects too, so we're all at risk of having bugs slip through uncaught. (There will simply be way too many screens to compare to catch everything, without investing tons of time and hoping that everything is caught and addressed, including combinations of widgets the PatternFly team doesn't test together.)

Back to the issue filed here: This isn't one of the many bugs that can be papered over with local CSS overrides — it requires fixes in JavaScript, which means that all of the projects using PatternFly are depending on a fix from PatternFly directly.

This really is something that should be addressed in v5.

thatblindgeye commented 2 weeks ago

Does https://github.com/patternfly/patternfly-react/pull/10642 aid in addressing this for v5? The PR adds a prop to prevent that auto-focus behavior. Or is the ask more to only prevent that auto-focus behavior when using a mouse to click open the Dropdown (matching the behavior previously seen in Dropdown)?

garrett commented 2 weeks ago

@thatblindgeye: Thanks for looking into this!

The proposed solution doesn't fix the issue; it just adds a workaround. The default behavior should be correct without having to do anything special. Relying on a workaround means developers need to know about the problem and add extra code, which isn't ideal. This bug needs a direct fix.

For menus and menu-based components:

This issue seems to stem from a code change that prioritized keyboard interactions over mouse interactions. Keyboard and mouse actions should behave differently, and their visual cues for focus and highlight should also differ. (Currently, in PatternFly 5, they look the same. But this bug should be able to be fixed without changing the style.)

Fixing this in PatternFly 5 is quite imporant as software using this version will be in many enterprise products for around a decade. Upgrading to PatternFly 6 will require significant changes and moving away from deprecated components, so many projects will not do this in the immediately and will be stuck with PatternFly 5 (and this confusing issue, unless fixed) for a very long time.

thatblindgeye commented 2 weeks ago

The reason for this behavior is due to the change made for https://github.com/patternfly/patternfly-react/issues/9788, which we feel also helps alleviate an accessibility issue. While the original issue was more of a general ask for multi-device support and the intent of the PR was to prioritize combo mouse+keyboard interaction rather than solely one or the other (e.g. mouse click a menu toggle, but be able to immediately navigate a menu via keyboard arrow keys), in some testing we found that certain AT looked to benefit from this behavior as well; where voice input is used to command actions, stating "click"/"touch" on the toggle acted similarly to a native mouse click, and without auto-focusing the menu content it could be very tedious and require more-than-necessary effort to get into the menu/click a menu item.

While it's a bit confusing at first that the first item in a Dropdown menu has that focus styling and another item will have the hover styling when hovered via mouse, the behavior itself is intended. It's unfortunate that we're relying on native browser focus outlines which causes both a focused item and hovered item to have the same styling under certain circumstances, and that there's some odd browser behavior happening where:

It's worth noting that the behavior where 1 item can have hover style and another item can have focus styles predates this recent behavior, and I don't believe these components fully mimicked a native select styling behavior (which a native element looks to match the styling as you described in your bullet points above). The main difference really being that we autofocus on mouse click now. Though this behavior of auto-focusing on mouse click is newer and different behavior than previously, it was an intentional update due to the aforementioned consumer request + feeling it helps alleviate a11y.

Depending on bandwidth this may be something we could look into addressing for v5 after v6 is released, though @nicolethoen may have more insight on that once we get further to the v6 release.

garrett commented 1 week ago

which we feel also helps alleviate an accessibility issue

Right, which is one of the several reasons why I strongly feel that a proper fix is to not have an option to disable it.

I looked at Windows, macOS, GNOME, and KDE and no other widget set does this (and for good reason):

Screenshot: image

Video:

Kooha-2024-07-04-16-19-49.webm

(It finally lost the style when I clicked outside of the browser.)

In fact, out of the survey of all OSes and desktops, only GNOME (the desktop we ship in RHEL) preselects, but it importantly acts differently than what PatternFly does:

Kooha-2024-07-04-16-23-20.webm

Note that when you're using the mouse and hover over something else, the selection style actually changes? You don't get two rows that have a highlight.

When you use the keyboard, it also moves the selection. It looks the same on focus or hover, but there's only ever one. They made it so that it adapts and does the expected thing when your using a keyboard vs a mouse.

Additionally, when their menus have other widgets that are within the menu that aren't just standard text-based menuitems, their backgrounds are not preselected or hovered (the widgets get their own native focus ring when using keyboard focus):

Kooha-2024-07-04-16-28-33.webm

To be clear, this exactly what I'm asking for:

  1. Fixing the default, in v5, so apps that will be supported for a decade can do the right thing.
  2. Having only one row with a highlight at any given time.
  3. Using the mouse to activate the widget can preselect, but hovering over other items should change the visual selection, instead of adding to it.
  4. Keyboard support should work (and not be turned off with an option), and that it should update the row that has a highlight.
  5. Widgets that are inside the menu should not have a double-highlight or double-focus effect. The children widgets should be able to handle their own highlight and focus state.

While this isn't 100% what Windows or macOS do as they do not preselect, what I'm asking for happens to be what all GNOME and GTK apps do in RHEL, which is what we're shipping at Red Hat too, so there's a precedence for how this should be done and we're already doing it correctly elsewhere.

Currently, PatternFly has a half-baked implementation that does not work as expected, as the well-intentioned accessibility commit did not consider interaction with a mouse, nor did it consider menus that have anything other than menuitems with text only.

I'm asking that this confusing visual regression be fully addressed in such a way that the applications that we ship to customers are not confusingly broken for the next decade due to this regression. Thanks.