search

vmware-clarity / core

Clarity is a scalable, accessible, customizable, open-source design system built with web components. Works with any JavaScript framework, created for enterprises, and designed to be inclusive.
https://clarity.design
MIT License
163 stars 42 forks source link

CdsDropdown triggers an error related to the popup API #140

Closed hippee-lee closed 1 year ago

hippee-lee commented 2 years ago

Describe the bug

When using cds-dropdown elements Chrome triggers the following error:

Found a 'popup' attribute. If you are testing the popup API, you must enable Experimental Web Platform Features. If not, note that custom attributes must start with 'data-': https://html.spec.whatwg.org/multipage/dom.html#custom-data-attribute. This usage will likely cause site breakage when the popup API ships: https://chromestatus.com/feature/5463833265045504.

The errors are not so bad when there are only one or two dropdowns. However when this is used with CdsGrid and the detail view, each row with a preview button triggers an error.

How to reproduce

  1. Go to the docs
  2. open dev tools console
  3. Observe the errors

Expected behavior

Chrome/edge will not throw errors when using the cds-dropdown element.

Versions

Clarity project:

Clarity version:

Device:

Additional notes

This feature is slated to ship in Chrome v110.

ashleyryan commented 2 years ago

Thanks @hippee-lee , I noticed this a week or so ago too, but didn't have a chance to investigate.

ashleyryan commented 2 years ago

Ok digging into this some more. Open UI has proposed adding a new popup attribute: https://open-ui.org/components/popup.research.explainer#html-content-attribute which is what chromium is implementing. The attribute would go on the element that is actually the popup (instead of the anchor in our case), and it would only have specific values. So I think we're going to have to rename this attribute, which would be a breaking change. However, the dropdown component is still in beta, so I think it's okay - or we can keep both and log a deprecation warning if you use popup...

According to the documentation, it sounds like you can put it on any element that is the anchor element and it will apply aria attributes for you. This would make me learn towards calling it cds-popup. In practice, you can only put it on elements that have the reactive controller, so any clarity component that extends CdsButton elements. In that case something less specific, like popup-element could work. But if we're going to change popup to popup-element, then perhaps we should also change trigger to trigger-element for consistency...

https://github.com/vmware-clarity/core/blob/main/projects/core/src/internal/controllers/aria-popup-trigger.controller.ts https://github.com/vmware-clarity/core/blob/main/projects/core/src/internal/controllers/aria-popup.controller.ts

github-actions[bot] commented 1 year ago

:tada: This issue has been resolved in version 6.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.