-
-
Notifications
You must be signed in to change notification settings - Fork 442
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
feat(lint): useSemanticElements #2867
feat(lint): useSemanticElements #2867
Conversation
CodSpeed Performance ReportMerging #2867 will not alter performanceComparing Summary
|
de4f3a9
to
8425dcd
Compare
e8c3f99
to
df3a915
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @fujiyamaorange for working on this.
I left few suggestions to make the code better. However, I think the rule isn't finished and it requires more work.
My advise is the following:
- change the PR description from "Closes ..." to "Implements ...", so Github won't close the issue
- open a PR to add more tests, ideally for all roles that have an element
- open a PR to expose a function that is able to give us the "concepts" of a role. With this function, we will be able to suggest the correct semantic elements that the user
Let me know what you think
for attr in attributes { | ||
let attr_value = attr.as_jsx_attribute().unwrap().name_value_token().unwrap(); | ||
let attr_name = attr_value.text_trimmed(); | ||
if attr_name == "role" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The node JsxOpeningElement
has function called find_attribute_by_name
:
pub fn find_attribute_by_name( |
It will remove a lot of code and you won't need to use a loop anymore
let element: biome_rowan::SyntaxToken<biome_js_syntax::JsLanguage> = | ||
node.name().ok()?.as_jsx_name()?.value_token().ok()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to cast element
:
let element: biome_rowan::SyntaxToken<biome_js_syntax::JsLanguage> = | |
node.name().ok()?.as_jsx_name()?.value_token().ok()?; | |
let element = node.name().ok()?.as_jsx_name()?.value_token().ok()?; |
// | ||
// Read our guidelines to write great diagnostics: | ||
// https://docs.rs/biome_analyze/latest/biome_analyze/#what-a-rule-should-say-to-the-user | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// | |
// Read our guidelines to write great diagnostics: | |
// https://docs.rs/biome_analyze/latest/biome_analyze/#what-a-rule-should-say-to-the-user | |
// |
/// ```js,expect_diagnostic | ||
/// <div role="checkbox"> | ||
/// <div role="img"> | ||
/// ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I advise you to read our documentation guide, it's full of useful information. For example, this isn't correct:
When adding invalid snippets in the ### Invalid section, you must use the expect_diagnostic code block property. We use this property to generate a diagnostic and attach it to the snippet. A snippet must emit only ONE diagnostic.
Since you're showing two elements, you must use two code blocks. Also, we're showing JSX code, so use the jsx
language code block instead of js
/// ```js | ||
/// // var a = 1; | ||
/// <div>...</div> | ||
/// <header>...</header> | ||
/// <img alt="" src="image.jpg" /> | ||
/// ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// ```js | |
/// // var a = 1; | |
/// <div>...</div> | |
/// <header>...</header> | |
/// <img alt="" src="image.jpg" /> | |
/// ``` | |
/// ```jsx | |
/// <div></div> | |
/// <header></header> | |
/// <img alt="" src="image.jpg" /> | |
/// ``` |
Some( | ||
RuleDiagnostic::new( | ||
rule_category!(), | ||
state.range(), | ||
markup! { | ||
"This JSX element uses a `role` attribute. Use a semantic element instead." | ||
}, | ||
) | ||
.note(markup! { | ||
"Semantic elements like `<button>`, `<input>`, `<textarea>`, `<a>`, `<img>`, `<table>`, `<article>`, `<section>`, `<nav>`, `<aside>`, `<header>`, `<footer>`, `<main>`, `<figure>`, `<figcaption>`, `<details>`, `<summary>`, `<dialog>`, `<menu>`, `<menuitem>`, `<fieldset>`, `<legend>`, `<caption>`, `<colgroup>`, `<col>`, `<optgroup>`, `<option>`, `<select>`, `<datalist>`, `<output>`, `<progress>`, `<meter>`, `<time>`, `<audio>`, `<video>`, `<track>`, `<source>`, `<embed>`, `<object>`, `<param>`, `<iframe>`, `<canvas>`, `<map>`, `<area>`, `<svg>`, `<math>` are more accessible and provide better semantics." | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are few things to unpack here, please bear with me:
- Follow the rule pillars. Remember that we want to provide high-quality errors to our users.
- The first pillar says that we need to message the error, but
This JSX element uses a role attribute
doesn't feel right because having arole
attribute isn't wrong. What about "The element with this role can be changed to a DOM element that already this role". I am not very good at writing, so feel free to come up with a better message. - Use the
footer_list
API here.
"This JSX element uses a `role` attribute. Use a semantic element instead." | ||
}, | ||
) | ||
.note(markup! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we might be able to identify the DOM element that corresponds to that role. We have the metadata for that, but we need a function for that.
If you want, I think this is a nice task to enhance the rule, maybe later with another PR. Let us know what you think.
This is the logic we are interested in:
biome/crates/biome_aria/src/roles.rs
Lines 1185 to 1239 in 9a05b77
for element in Self::ROLE_WITH_CONCEPTS { | |
let role = match *element { | |
"checkbox" => &CheckboxRole as &dyn AriaRoleDefinitionWithConcepts, | |
"radio" => &RadioRole as &dyn AriaRoleDefinitionWithConcepts, | |
"option" => &OptionRole as &dyn AriaRoleDefinitionWithConcepts, | |
"combobox" => &ComboBoxRole as &dyn AriaRoleDefinitionWithConcepts, | |
"heading" => &HeadingRole as &dyn AriaRoleDefinitionWithConcepts, | |
"separator" => &SeparatorRole as &dyn AriaRoleDefinitionWithConcepts, | |
"button" => &ButtonRole as &dyn AriaRoleDefinitionWithConcepts, | |
"article" => &ArticleRole as &dyn AriaRoleDefinitionWithConcepts, | |
"dialog" => &DialogRole as &dyn AriaRoleDefinitionWithConcepts, | |
"alert" => &AlertRole as &dyn AriaRoleDefinitionWithConcepts, | |
"alertdialog" => &AlertDialogRole as &dyn AriaRoleDefinitionWithConcepts, | |
"cell" => &CellRole as &dyn AriaRoleDefinitionWithConcepts, | |
"columnheader" => &ColumnHeaderRole as &dyn AriaRoleDefinitionWithConcepts, | |
"definition" => &DefinitionRole as &dyn AriaRoleDefinitionWithConcepts, | |
"figure" => &FigureRole as &dyn AriaRoleDefinitionWithConcepts, | |
"form" => &FormRole as &dyn AriaRoleDefinitionWithConcepts, | |
"graphics-document" => &GraphicsDocumentRole as &dyn AriaRoleDefinitionWithConcepts, | |
"graphics-object" => &GraphicsObjectRole as &dyn AriaRoleDefinitionWithConcepts, | |
"grid" => &GridRole as &dyn AriaRoleDefinitionWithConcepts, | |
"gridcell" => &GridCellRole as &dyn AriaRoleDefinitionWithConcepts, | |
"group" => &GroupRole as &dyn AriaRoleDefinitionWithConcepts, | |
"img" => &ImgRole as &dyn AriaRoleDefinitionWithConcepts, | |
"link" => &LinkRole as &dyn AriaRoleDefinitionWithConcepts, | |
"list" => &ListRole as &dyn AriaRoleDefinitionWithConcepts, | |
"listbox" => &ListBoxRole as &dyn AriaRoleDefinitionWithConcepts, | |
"listitem" => &ListItemRole as &dyn AriaRoleDefinitionWithConcepts, | |
"navigation" => &NavigationRole as &dyn AriaRoleDefinitionWithConcepts, | |
"row" => &RowRole as &dyn AriaRoleDefinitionWithConcepts, | |
"rowgroup" => &RowGroupRole as &dyn AriaRoleDefinitionWithConcepts, | |
"rowheader" => &RowHeaderRole as &dyn AriaRoleDefinitionWithConcepts, | |
"search" => &SearchboxRole as &dyn AriaRoleDefinitionWithConcepts, | |
"searchbox" => &SearchboxRole as &dyn AriaRoleDefinitionWithConcepts, | |
"table" => &TableRole as &dyn AriaRoleDefinitionWithConcepts, | |
"term" => &TermRole as &dyn AriaRoleDefinitionWithConcepts, | |
"textbox" => &TextboxRole as &dyn AriaRoleDefinitionWithConcepts, | |
"generic" => &GenericRole as &dyn AriaRoleDefinitionWithConcepts, | |
"caption" => &CaptionRole as &dyn AriaRoleDefinitionWithConcepts, | |
"main" => &MainRole as &dyn AriaRoleDefinitionWithConcepts, | |
"time" => &TimeRole as &dyn AriaRoleDefinitionWithConcepts, | |
"p" => &ParagraphRole as &dyn AriaRoleDefinitionWithConcepts, | |
"aside" => &ComplementaryRole as &dyn AriaRoleDefinitionWithConcepts, | |
"blockquote" => &BlockQuoteRole as &dyn AriaRoleDefinitionWithConcepts, | |
"associationlist" => &AssociationListRole as &dyn AriaRoleDefinitionWithConcepts, | |
"status" => &StatusRole as &dyn AriaRoleDefinitionWithConcepts, | |
"contentinfo" => &ContentInfoRole as &dyn AriaRoleDefinitionWithConcepts, | |
"region" => &RegionRole as &dyn AriaRoleDefinitionWithConcepts, | |
_ => return false, | |
}; | |
if let Some(mut concepts) = role.concepts_by_element_name(element_name) { | |
if concepts.any(|(name, _)| *name == element_name) && !role.is_interactive() { | |
return true; | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ematipico
Thank you for telling me.
Actually, I read the code and it seems we can't get the alternative element for the role for now.
However, it can be big task for me so let me try in the next PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's exactly what I was suggesting
const checkbox = () => ( | ||
<div role="checkbox" ></div> | ||
); | ||
|
||
const Image = () => ( | ||
<div role="img" ></div> | ||
); | ||
|
||
const Button = () => ( | ||
<div aria-label="foo" role="button" >button</div> | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const checkbox = () => ( | |
<div role="checkbox" ></div> | |
); | |
const Image = () => ( | |
<div role="img" ></div> | |
); | |
const Button = () => ( | |
<div aria-label="foo" role="button" >button</div> | |
); | |
<> | |
<div role="checkbox" ></div> | |
<div role="img" ></div> | |
<div aria-label="foo" role="button" >button</div> | |
</> |
Way less code. Also, this file should be called invalid.jsx
, not invalid.js
.
Also, we should be more thorough with testing, especially with a11y. This means that we should add more tests. Ideally we should cover all roles contained here:
biome/crates/biome_aria/src/roles.rs
Lines 1185 to 1239 in 9a05b77
for element in Self::ROLE_WITH_CONCEPTS { | |
let role = match *element { | |
"checkbox" => &CheckboxRole as &dyn AriaRoleDefinitionWithConcepts, | |
"radio" => &RadioRole as &dyn AriaRoleDefinitionWithConcepts, | |
"option" => &OptionRole as &dyn AriaRoleDefinitionWithConcepts, | |
"combobox" => &ComboBoxRole as &dyn AriaRoleDefinitionWithConcepts, | |
"heading" => &HeadingRole as &dyn AriaRoleDefinitionWithConcepts, | |
"separator" => &SeparatorRole as &dyn AriaRoleDefinitionWithConcepts, | |
"button" => &ButtonRole as &dyn AriaRoleDefinitionWithConcepts, | |
"article" => &ArticleRole as &dyn AriaRoleDefinitionWithConcepts, | |
"dialog" => &DialogRole as &dyn AriaRoleDefinitionWithConcepts, | |
"alert" => &AlertRole as &dyn AriaRoleDefinitionWithConcepts, | |
"alertdialog" => &AlertDialogRole as &dyn AriaRoleDefinitionWithConcepts, | |
"cell" => &CellRole as &dyn AriaRoleDefinitionWithConcepts, | |
"columnheader" => &ColumnHeaderRole as &dyn AriaRoleDefinitionWithConcepts, | |
"definition" => &DefinitionRole as &dyn AriaRoleDefinitionWithConcepts, | |
"figure" => &FigureRole as &dyn AriaRoleDefinitionWithConcepts, | |
"form" => &FormRole as &dyn AriaRoleDefinitionWithConcepts, | |
"graphics-document" => &GraphicsDocumentRole as &dyn AriaRoleDefinitionWithConcepts, | |
"graphics-object" => &GraphicsObjectRole as &dyn AriaRoleDefinitionWithConcepts, | |
"grid" => &GridRole as &dyn AriaRoleDefinitionWithConcepts, | |
"gridcell" => &GridCellRole as &dyn AriaRoleDefinitionWithConcepts, | |
"group" => &GroupRole as &dyn AriaRoleDefinitionWithConcepts, | |
"img" => &ImgRole as &dyn AriaRoleDefinitionWithConcepts, | |
"link" => &LinkRole as &dyn AriaRoleDefinitionWithConcepts, | |
"list" => &ListRole as &dyn AriaRoleDefinitionWithConcepts, | |
"listbox" => &ListBoxRole as &dyn AriaRoleDefinitionWithConcepts, | |
"listitem" => &ListItemRole as &dyn AriaRoleDefinitionWithConcepts, | |
"navigation" => &NavigationRole as &dyn AriaRoleDefinitionWithConcepts, | |
"row" => &RowRole as &dyn AriaRoleDefinitionWithConcepts, | |
"rowgroup" => &RowGroupRole as &dyn AriaRoleDefinitionWithConcepts, | |
"rowheader" => &RowHeaderRole as &dyn AriaRoleDefinitionWithConcepts, | |
"search" => &SearchboxRole as &dyn AriaRoleDefinitionWithConcepts, | |
"searchbox" => &SearchboxRole as &dyn AriaRoleDefinitionWithConcepts, | |
"table" => &TableRole as &dyn AriaRoleDefinitionWithConcepts, | |
"term" => &TermRole as &dyn AriaRoleDefinitionWithConcepts, | |
"textbox" => &TextboxRole as &dyn AriaRoleDefinitionWithConcepts, | |
"generic" => &GenericRole as &dyn AriaRoleDefinitionWithConcepts, | |
"caption" => &CaptionRole as &dyn AriaRoleDefinitionWithConcepts, | |
"main" => &MainRole as &dyn AriaRoleDefinitionWithConcepts, | |
"time" => &TimeRole as &dyn AriaRoleDefinitionWithConcepts, | |
"p" => &ParagraphRole as &dyn AriaRoleDefinitionWithConcepts, | |
"aside" => &ComplementaryRole as &dyn AriaRoleDefinitionWithConcepts, | |
"blockquote" => &BlockQuoteRole as &dyn AriaRoleDefinitionWithConcepts, | |
"associationlist" => &AssociationListRole as &dyn AriaRoleDefinitionWithConcepts, | |
"status" => &StatusRole as &dyn AriaRoleDefinitionWithConcepts, | |
"contentinfo" => &ContentInfoRole as &dyn AriaRoleDefinitionWithConcepts, | |
"region" => &RegionRole as &dyn AriaRoleDefinitionWithConcepts, | |
_ => return false, | |
}; | |
if let Some(mut concepts) = role.concepts_by_element_name(element_name) { | |
if concepts.any(|(name, _)| *name == element_name) && !role.is_interactive() { | |
return true; | |
} | |
} |
It's a lot, so I suggest you defer it to another PR.
.footer_list( | ||
markup! { | ||
"For examples and more information, see" <Hyperlink href="https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles">"WAI-ARIA Roles"</Hyperlink> | ||
}, | ||
&["serif", "sans-serif", "monospace", "etc."], | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fujiyamaorange The link that I shared was an example to show you how to use the footer_list
API, but in the array should put all the elements you were listing before (<button>
, <input>
, <textarea>
, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ematipico
Let me confirm,
&["<button>", "<input>", "<textarea>", "<a>", "<img>", "<table>", "<article>", "<section>", "<nav>", "<aside>", "<header>", "<footer>", "<main>", "<figure>", "<figcaption>", "<details>", "<summary>", "<dialog>", "<menu>", "<menuitem>", "<fieldset>", "<legend>", "<caption>", "<colgroup>", "<col>", "<optgroup>", "<option>", "<select>", "<datalist>", "<output>", "<progress>", "<meter>", "<time>", "<audio>", "<video>", "<track>", "<source>", "<embed>", "<object>", "<param>", "<iframe>", "<canvas>", "<map>", "<area>", "<svg>", "<math>"]
This will work properly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's what you wanted, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I'll commit it!
Summary
jsx-a11y/prefer-tag-over-role
#2614Test Plan
Invalid
Valid