feat(rome_js_analyzer): add useAltText lint rule for JSX expression #3371

b4s36t4 · 2022-10-08T18:53:39Z

Summary

Closes #3298 following the document given for valid and in-valid conditions.

Test Plan

A lint rule to check for alt attribute in JSX for tags like input type='image', img, area.

netlify · 2022-10-08T18:53:42Z

❌ Deploy Preview for docs-rometools failed.

Name	Link
🔨 Latest commit	`2014015`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/634daa06b70bb9000befc44f

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

ematipico

Thank you for the contribution! I left some suggestion and correction to the cases covered

ematipico · 2022-10-10T09:09:00Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+}
+
+declare_node_union! {
+    pub (crate) UseAltTextQuery = JsxSelfClosingElement  | JsCallExpression


I think we should limit ourselves only to JsxSelfClosingElement nodes. Which means that union is not needed anymore, and the Query will become:

type Query = Ast<JsxSelfClosingElement>;

ematipico · 2022-10-10T09:10:28Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+    None
+}
+
+fn is_valid_tag(name: &JsxAnyElementName) -> Option<bool> {


Can we document this function? "Valid tag" is not clear, unless we look at the implementation of the function.

ematipico · 2022-10-10T09:11:07Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+    }
+}
+
+fn is_valid_input(element: &JsxSelfClosingElement) -> Option<bool> {


Can we document this function? "Valid input" needs to have a context; we have to tell developers what a valid input means in this case.

Also, this function could be called input_has_type_image

ematipico · 2022-10-10T09:12:34Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+            UseAltTextQuery::JsxSelfClosingElement(element) => {
+                let name = element.name().ok()?;
+                if is_valid_tag(&name)? {
+                    let _attributes = element.attributes();


Doesn't seem this is used... so we can remove it?

ematipico · 2022-10-10T09:18:46Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+                    let is_spread_available = element
+                        .attributes()
+                        .iter()
+                        .any(|attribute| attribute.as_jsx_spread_attribute().is_some());


The usage of the spread props is sneaky and we need to be careful to where it's placed inside the component.

Here's two examples:

let first = <img {...props} alt />; let second = <img alt {...props} />;

The first case is invalid, meaning if should trigger the rule, because alt overrides what's inside props.
The second case is valid, meaning it should not trigger the rule, because props might have the alt property and override the one passed before it. This is a case we don't have enough information to trigger the rule, so has rule of thumb, if we don't know if a rule should be triggered, we don't trigger it.

Check for the spread property might be complex, and I think we should have an API for that. What I would suggest for now, is to avoid checking for spread prop for the time being.

ematipico · 2022-10-10T09:21:58Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+                        }
+                        return Some(UseAltTextState {
+                            node: UseAltTextNode::from(element.clone()),
+                            missing_alt_prop: true,


Every time UseAltTextState is returned, missing_alt_prop is true, which means that we don't need this property

ematipico · 2022-10-10T09:23:13Z

crates/rome_js_analyze/tests/specs/nursery/useAltText/area.jsx.snap

+// invalid
+
+<>
+	<area {...props} />


This case is valid. Inside props we might have alt. We don't have enough info so we should not trigger the rule.

This applies to other tags as well?

ematipico · 2022-10-10T09:23:54Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+            .footer(
+                Severity::Note,
+                markup! {
+                    "Meaningful alternative text on elements helps users relying on screen
+                readers to understand content's purpose within a page."
+                },
+            ),


nit: you can footer_note function and pass only the text.

ematipico · 2022-10-10T09:25:40Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+
+impl Rule for UseAltText {
+    type Query = Semantic<UseAltTextQuery>;
+    type State = UseAltTextState;


I think State can be a TextRange

type State = TextRange

And when you find a property that is incorrect, you just do

return Some(element.syntaxt().text_range_trimmed())

And the state can be used inside the diagnostic. As you noticed the state is only used to retrieve the range.

b4s36t4 · 2022-10-17T19:18:12Z

@ematipico I have updated the code, please check it.

Note: as part of skipping cases including {...props} spread items existing cases mentioned in the doc will became valid. Please check the snapshots for more context.

ematipico · 2022-10-18T10:10:28Z

crates/rome_js_analyze/tests/specs/nursery/useAltText/area.jsx

+// invalid
+
+<>
+	<area alt={undefined} />


If we add a case for undefined, I think we should also add a case for alt={null}

ematipico · 2022-10-18T10:12:21Z

crates/rome_js_analyze/tests/specs/nursery/useAltText/img.jsx

+	<img {...props} alt />
+	<img {...props} /> {/* Skipping*/}
+	<img {...props} alt={undefined} /> {/* Skipping*/}


Can we remove these cases? It would be better to tackle spread props in a different PR (it's a feature missing in different rules, not just this one). And for example, the first and third cases are false positives.

ematipico · 2022-10-18T10:13:30Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+            return None;
+        }
+        let message = markup!(
+            "Provide "<Emphasis>"alt"</Emphasis>" when using "<Emphasis>"img"</Emphasis>", "<Emphasis>"area"</Emphasis>", "<Emphasis>"input type='image'"</Emphasis>""


Suggested change

"Provide "<Emphasis>"alt"</Emphasis>" when using "<Emphasis>"img"</Emphasis>", "<Emphasis>"area"</Emphasis>", "<Emphasis>"input type='image'"</Emphasis>""

"Provide the attribute"<Emphasis>"alt"</Emphasis>" when using "<Emphasis>"img"</Emphasis>", "<Emphasis>"area"</Emphasis>" or "<Emphasis>"input type='image'"</Emphasis>""

You would need to regenerate the snapshots and documentation

ematipico · 2022-10-18T10:14:02Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+        ).to_owned();
+
+        Some(
+            RuleDiagnostic::new(rule_category!(), state, message).footer(


You can just use footer_note, so you don't need to pass Severity

ematipico · 2022-10-18T10:14:45Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+/*
+This function checks for the attribute `type` for input element where we checking for the input type which is image.
+ */


Suggested change

/*

This function checks for the attribute `type` for input element where we checking for the input type which is image.

*/

/// This function checks for the attribute `type` for input element where we checking for the input type which is image.

We should use rust documentation comments

ematipico · 2022-10-18T10:15:09Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+/*
+Function to check that the HTML/JSX tag is valid for the lint rule, which check if the tag should be `img`, `input type='image' and `area`.
+
+ */


Suggested change

/*

Function to check that the HTML/JSX tag is valid for the lint rule, which check if the tag should be `img`, `input type='image' and `area`.

*/

/// Function to check that the HTML/JSX tag is valid for the lint rule, which check if the tag should be `img`, `input type='image' and `area`.

We should use rust documentation comments

ematipico · 2022-10-18T10:16:25Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+            let is_spread_available = element
+                .attributes()
+                .iter()
+                .any(|attribute| attribute.as_jsx_spread_attribute().is_some());
+
+            // To-Do - Complex cases might invlove, not triggering rules.
+            if is_spread_available {
+                return None;
+            }


I think it's best, for now, to remove the check for spread props. It's a feature/functionality that is missing from various rules and the implementation should do more checks. It's OK to remove it, I will work on it later.

ematipico · 2022-10-18T10:17:27Z

website/src/docs/lint/rules/UseAltText.md

@@ -0,0 +1,57 @@
+---


This file seems incorrect. It should be called useAltText.md. Can you delete it and try to generate it again?

ematipico · 2022-10-18T10:18:00Z

website/src/docs/lint/rules/index.md

+<<<<<<< HEAD
+<h3 data-toc-exclude id="useAltText">
+	<a href="/docs/lint/rules/useAltText">useAltText (since v0.10.0)</a>
+	<a class="header-anchor" href="#useAltText"></a>
+</h3>
+It asserts that alternative text to images or areas, help to rely on to screen readers to understand the purpose and the context of the image.
+=======


There's some issue with this file too... You should delete it and generate again

xunilrj · 2022-10-19T12:48:31Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+Function to check that the HTML/JSX tag is valid for the lint rule, which check if the tag should be `img`, `input type='image' and `area`.
+
+ */
+fn is_valid_tag(name: &JsxAnyElementName) -> Option<bool> {


"text_trimmed"and the navigation are expensive methods. You can simplify this method, and its call to:

match name.as_jsx_name().map(|x| x.text_trimmed()) { Some("input") => {...} Some("img" | "area") => {...} }

xunilrj · 2022-10-19T17:28:02Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs

+                return None;
+            }
+
+            let alt_prop = element.find_attribute_by_name("alt").ok()?;


You may also look at: #3458.
It will improve the code here.

ematipico · 2022-10-26T12:46:41Z

Closing in favour of #3502

@b4s36t4 I opened a new PR with your commits. We want to have this rule merged before the release.

feat: add useAltText lint rule for JSX expression

bcaba04

b4s36t4 requested review from leops, ematipico and xunilrj as code owners October 8, 2022 18:53

b4s36t4 requested a review from a team October 8, 2022 18:53

leops approved these changes Oct 10, 2022

View reviewed changes

crates/rome_js_analyze/src/semantic_analyzers/nursery/use_alt_text.rs Outdated Show resolved Hide resolved

ematipico suggested changes Oct 10, 2022

View reviewed changes

b4s36t4 added 2 commits October 18, 2022 00:34

fix: refactoring and vscode config add

2488226

merge with master

2014015

ematipico changed the title ~~feat: add useAltText lint rule for JSX expression~~ feat(rome_js_analyzer): add useAltText lint rule for JSX expression Oct 18, 2022

ematipico approved these changes Oct 18, 2022

View reviewed changes

ematipico reviewed Oct 18, 2022

View reviewed changes

ematipico suggested changes Oct 18, 2022

View reviewed changes

xunilrj reviewed Oct 19, 2022

View reviewed changes

ematipico changed the base branch from main to ematipico October 26, 2022 12:32

ematipico changed the base branch from ematipico to main October 26, 2022 12:32

ematipico closed this Oct 26, 2022

ematipico mentioned this pull request Oct 26, 2022

feat: add useAltText lint rule for JSX expression #3502

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_js_analyzer): add useAltText lint rule for JSX expression #3371

feat(rome_js_analyzer): add useAltText lint rule for JSX expression #3371

b4s36t4 commented Oct 8, 2022 •

edited by ematipico

Loading

netlify bot commented Oct 8, 2022 •

edited

Loading

ematipico left a comment

ematipico Oct 10, 2022

ematipico Oct 10, 2022

ematipico Oct 10, 2022

ematipico Oct 10, 2022

ematipico Oct 10, 2022

ematipico Oct 10, 2022

ematipico Oct 10, 2022

b4s36t4 Oct 17, 2022

ematipico Oct 10, 2022

ematipico Oct 10, 2022

b4s36t4 commented Oct 17, 2022 •

edited

Loading

ematipico Oct 18, 2022

ematipico Oct 18, 2022

ematipico Oct 18, 2022

ematipico Oct 18, 2022

ematipico Oct 18, 2022

ematipico Oct 18, 2022

ematipico Oct 18, 2022

ematipico Oct 18, 2022

ematipico Oct 18, 2022

xunilrj Oct 19, 2022

xunilrj Oct 19, 2022

ematipico commented Oct 26, 2022 •

edited

Loading

	"Provide "<Emphasis>"alt"</Emphasis>" when using "<Emphasis>"img"</Emphasis>", "<Emphasis>"area"</Emphasis>", "<Emphasis>"input type='image'"</Emphasis>""
	"Provide the attribute"<Emphasis>"alt"</Emphasis>" when using "<Emphasis>"img"</Emphasis>", "<Emphasis>"area"</Emphasis>" or "<Emphasis>"input type='image'"</Emphasis>""

feat(rome_js_analyzer): add useAltText lint rule for JSX expression #3371

feat(rome_js_analyzer): add useAltText lint rule for JSX expression #3371

Conversation

b4s36t4 commented Oct 8, 2022 • edited by ematipico Loading

Summary

Test Plan

netlify bot commented Oct 8, 2022 • edited Loading

❌ Deploy Preview for docs-rometools failed.

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b4s36t4 commented Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico commented Oct 26, 2022 • edited Loading

b4s36t4 commented Oct 8, 2022 •

edited by ematipico

Loading

netlify bot commented Oct 8, 2022 •

edited

Loading

b4s36t4 commented Oct 17, 2022 •

edited

Loading

ematipico commented Oct 26, 2022 •

edited

Loading