Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

It is not possible to remove the role attribute which results in invalid HTML #4136

Closed
1 task done
SantosGuillamot opened this issue Sep 19, 2023 · 5 comments · Fixed by #4137
Closed
1 task done

Comments

@SantosGuillamot
Copy link

  • Check if updating to the latest Preact version resolves the issue

Describe the bug
It seems impossible to toggle between having a role attribute (dialog for example) and removing the attribute. For example, I want to have a role="dialog" when a menu is open, but remove the role attribute when it is closed. It currently returns an empty value. This affects accessibility because having an empty role is not valid HTML. You can check that using an HTML validator:

Screenshot 2023-09-19 at 16 20 43

This seems to be the behavior in Preact with the following options:

  • Using null: It keeps the role attribute in the element.
  • Using an empty string: It keeps the role attribute in the element.
  • Using false: It sets role="false".

I also tried using another role value as none, but it changes the accessibility tree.

I made a quick video using this CodeSandbox explaining that better:

Sandbox.-.CodeSandbox.-.19.September.2023.mp4

I've tested it in React, and it seems possible to remove the role attribute by using null. I used this other CodeSandbox as shown in this other video:

React.App.-.19.September.2023.mp4

To Reproduce

Preact behavior

Using this CodeSandbox, toggle between the different options to see how the role attribute of the div containing the <pre> element changes.

React behavior

Using this CodeSandbox, toggle between the different options to see how the role attribute of the div containing the <pre> element changes.

Expected behavior
It should be possible to remove the role attribute as it happens in React.

@rschristian
Copy link
Member

rschristian commented Sep 19, 2023

Interestingly, your Preact sandbox seems to work as you'd expect in FireFox (false and null both cause removal). Can reproduce what you show in Chrome.

Edit: Ah, FF doesn't add it (role) to the DOM, while Chrome does.

@DAreRodz
Copy link
Contributor

Edit: Ah, FF doesn't add it (role) to the DOM, while Chrome does.

Exactly; most browsers seem to have implemented the properties defined in the ARIAMixin interface, whereas Firefox hasn't implemented them yet.

Examples:

@DAreRodz
Copy link
Contributor

In the browsers where Element.role exists, this condition inside setProperty becomes true:

name in dom

That makes Preact assign an empty string to Element.role when the following line is reached shortly after:

dom[name] = value == null ? '' : value;

However, the value assigned in this case should be null instead.

Maybe the fix is just a matter of adding another check above? Like those for width, height, href, list, etc.? 🤔

@marvinhagemeister
Copy link
Member

@DAreRodz That analysis is spot on. Feel free to make a PR that adds it to the list (+ a test) 👍

@DAreRodz
Copy link
Contributor

Thanks @marvinhagemeister! I've created this PR, ready for review. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants