vmware-clarity / ng-clarity

Clarity Angular is a scalable, accessible, customizable, open-source design system built for Angular.
https://clarity.design
Other
341 stars 77 forks source link

Disabled dropdown items could be triggered with the keyboard #599

Closed petarbk closed 1 year ago

petarbk commented 1 year ago

Describe the bug

If the keyboard arrows are used to select a disabled clrDropdownItem that uses [clrDisabled]="true", and Enter key is pressed after the focus is on the disabled item, the associated click handler of the disabled item will be executed.

How to reproduce

Here is Stackblitz reproduction of the issue - https://stackblitz.com/edit/clarity-light-theme-clr13-cds6-ng14-v4rv8z?file=src/app/app.component.html

Steps to reproduce the behavior:

Let's assume we have the following dropdown

 <clr-dropdown [clrCloseMenuOnItemClick]="false">
     <button clrDropdownTrigger class="btn btn-outline-primary">
         dropdown menu
      </button>
      <clr-dropdown-menu *clrIfOpen>
          <button clrDropdownItem>Item 1</button>
          <button
            clrDropdownItem
            [clrDisabled]="true"
            (click)="disabledItemClickHandler()">
                Item 2 (disabled)
          </button>
          <button clrDropdownItem>Item 3</button>
     </clr-dropdown-menu>
</clr-dropdown>
  1. Create a clr-dropdown where clrDisabled is used to disable a dropdown item (see code example above).
  2. Use the keyboard to open the dropdown and using the keyboard arrows select the disabled item that has text "Item 2 (disabled)".
  3. Press 'Enter' key while the disabled item is focused to trigger the click handler disabledItemClickHandler() of the disabled item.

Expected behavior

It should not be possible to trigger with a keyboard a dropdown item that has [clrDisabled]="true" applied on it.

Versions

Clarity version:

Framework version:

Device:

kevinbuhmann commented 1 year ago

This is unfortunately not a bug. Please see our documentation:

A menu item should not use the disabled attribute so disabled items can be focusable. Instead, use [clrDisabled] to add the disabled visual style and ARIA attribute when needed. It is up to the application to ensure that the disabled item is not activatable. -- https://clarity.design/documentation/dropdowns (emphasis added)

This is due to the following ARIA guideline:

Disabled menu items are focusable but cannot be activated. -- https://www.w3.org/WAI/ARIA/apg/patterns/menubar/

Since disabled items must be focusable, we cannot add the native the disabled attribute to prevent activation.

petarbk commented 1 year ago

It's still a bug according to the documentation - an item should be focusable with a keyboard but we should not be able to activate the action.

image

kevinbuhmann commented 1 year ago

As quoted above, our documentation states:

It is up to the application to ensure that the disabled item is not activatable.

There is no way for the clrDropdownitem directive to prevent activation and also allow focus. You must check if the option is disabled in your click event handler. I know this isn't great, but I don't know of any other option.

petarbk commented 1 year ago

From what I see, the clrDropdownItem directive already has logic that tries to prevent the action by calling stopImmediatePropagation on enter/space keydown - https://github.com/vmware-clarity/ng-clarity/blob/510d78c2b87a4ffe4eba1b920d689d6860500401/projects/angular/src/popover/dropdown/dropdown-item.ts#L84

The problem IMO is, event.prevenDefault() is not called and this generates click event on the dropdown item. I think the code there should look something like

  private stopImmediatePropagationIfDisabled($event: Event) {
    if (this.disabled) {
      $event.stopImmediatePropagation();
      $event.preventDefault();
    }
  }
kevinbuhmann commented 1 year ago

Okay, that does seem to work. I remember adding the stopImmediatePropagation code now. I guess it either didn't really work or something changed.

github-actions[bot] commented 1 year ago

:tada: This issue has been resolved in version 13.16.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 1 year ago

:tada: This issue has been resolved in version 15.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 1 year ago

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed issues after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.