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

💅 noAriaHiddenOnFocusable doesn't handle a bracketed number for tabIndex #3689

Closed
1 task done
anthonyshew opened this issue Aug 20, 2024 · 4 comments
Closed
1 task done
Assignees
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Bug-confirmed Status: report has been confirmed as a valid bug

Comments

@anthonyshew
Copy link
Contributor

anthonyshew commented Aug 20, 2024

Environment information

CLI:
  Version:                      1.8.3
  Color support:                true

Platform:
  CPU Architecture:             aarch64
  OS:                           macos

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "screen-256color"
  JS_RUNTIME_VERSION:           "v20.11.1"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "npm/10.2.4"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    true
  VCS disabled:                 false

Linter:
  JavaScript enabled:           true
  JSON enabled:                 true
  CSS enabled:                  false
  Recommended:                  false
  All:                          false
  Enabled rules:
  suspicious/noCatchAssign
  correctness/noVoidElementsWithChildren
  suspicious/noUnsafeNegation
  style/useAsConstAssertion
  suspicious/noMisleadingInstantiator
  complexity/noMultipleSpacesInRegularExpressionLiterals
  a11y/useValidLang
  complexity/noUselessLoneBlockStatements
  suspicious/noDebugger
  suspicious/noThenProperty
  suspicious/useNamespaceKeyword
  suspicious/noGlobalAssign
  a11y/useAriaActivedescendantWithTabindex
  suspicious/useValidTypeof
  suspicious/noCommentText
  complexity/noEmptyTypeParameters
  a11y/noPositiveTabindex
  correctness/noConstructorReturn
  complexity/noExcessiveNestedTestSuites
  security/noDangerouslySetInnerHtmlWithChildren
  a11y/useKeyWithMouseEvents
  suspicious/noDuplicateParameters
  suspicious/useGetterReturn
  correctness/noUnusedLabels
  correctness/noRenderReturnValue
  correctness/noUnreachableSuper
  security/noGlobalEval
  suspicious/noDuplicateJsxProps
  suspicious/noExtraNonNullAssertion
  correctness/useIsNan
  suspicious/noCompareNegZero
  correctness/noConstAssign
  suspicious/noGlobalIsFinite
  suspicious/noAsyncPromiseExecutor
  style/useNodejsImportProtocol
  a11y/noDistractingElements
  style/useLiteralEnumMembers
  complexity/noWith
  suspicious/noDuplicateClassMembers
  a11y/useValidAriaProps
  suspicious/noConfusingLabels
  correctness/noPrecisionLoss
  suspicious/noMisleadingCharacterClass
  correctness/noStringCaseMismatch
  correctness/noSetterReturn
  correctness/noInvalidConstructorSuper
  suspicious/noFallthroughSwitchClause
  suspicious/noUnsafeDeclarationMerging
  suspicious/noFocusedTests
  complexity/noThisInStatic
  suspicious/noDuplicateCase
  a11y/noAriaUnsupportedElements
  correctness/noFlatMapIdentity
  correctness/noSelfAssign
  style/useShorthandFunctionType
  correctness/useValidForDirection
  complexity/noUselessLabel
  complexity/noUselessCatch
  correctness/noInvalidUseBeforeDeclaration
  a11y/noAriaHiddenOnFocusable
  correctness/noUnsafeFinally
  complexity/noUselessRename
  correctness/noInvalidNewBuiltin
  correctness/noNonoctalDecimalEscape
  a11y/noAccessKey
  a11y/useHtmlLang
  suspicious/noDuplicateTestHooks
  complexity/noStaticOnlyClass
  suspicious/noImportAssign
  style/useNumericLiterals
  complexity/useSimpleNumberKeys
  correctness/useYield
  suspicious/useDefaultSwitchClauseLast
  suspicious/noLabelVar
  suspicious/noApproximativeNumericConstant
  a11y/noHeaderScope
  correctness/noGlobalObjectCalls
  suspicious/noMisrefactoredShorthandAssign
  suspicious/noClassAssign
  suspicious/noSuspiciousSemicolonInJsx
  suspicious/noSparseArray

Rule name

noAriaHiddenOnFocusable

Playground link

https://biomejs.dev/playground/?code=YwBvAG4AcwB0ACAAVABoAGkAbgBnACAAPQAgACgAKQAgAD0APgAgAHsACgAgACAAcgBlAHQAdQByAG4AIAAoAAoAIAAgACAAIAA8AD4ACgAgACAAIAAgACAAIAA8AGkAIABhAHIAaQBhAC0AaABpAGQAZABlAG4APQAiAHQAcgB1AGUAIgAgAHQAYQBiAEkAbgBkAGUAeAA9AHsALQAxAH0AIAAvAD4ACgAgACAAIAAgACAAIAA8AGkAIABhAHIAaQBhAC0AaABpAGQAZABlAG4APQAiAHQAcgB1AGUAIgAgAHQAYQBiAEkAbgBkAGUAeAA9ACIALQAxACIAIAAvAD4ACgAgACAAIAAgADwALwA%2BAAoAIAAgACkACgB9AAoACgAKAA%3D%3D

Expected result

I'd expect both of the lines of JSX to pass since their rendered output are identical.

Extra bit of context, I tried to use the string variant as documented. This produces a TypeScript error, so I either have to ignore a TypeScript diagnostic or a Biome diagnostic.
CleanShot 2024-08-20 at 16 28 26@2x

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@ematipico ematipico added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Bug-confirmed Status: report has been confirmed as a valid bug labels Aug 21, 2024
@anthonyshew
Copy link
Contributor Author

anthonyshew commented Aug 22, 2024

@ematipico, I'm happy to fix this up if I could bother you for pointers. First time stepping into the codebase so have that lovely "I'm completely lost" feeling. 🫠

I've added a line of <button aria-hidden="true" tabIndex={-1} /> into valid.jsx for this rule. Throwing in some quick dbg!s at crates/biome_js_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rs:80, I get:

---- specs::a11y::no_aria_hidden_on_focusable::valid_jsx stdout ----
[crates/biome_js_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rs:80:21] &tabindex_attr = JsxAttribute {
    name: JsxName {
        value_token: [email protected] "tabIndex" [] [],
    },
    initializer: JsxAttributeInitializerClause {
        eq_token: [email protected] "=" [] [],
        value: JsxString {
            value_token: [email protected] "\"-1\"" [] [Whitespace(" ")],
        },
    },
}
[crates/biome_js_analyze/src/lint/a11y/no_aria_hidden_on_focusable.rs:81:21] &tabindex_static = String(
    [email protected] "\"-1\"" [] [Whitespace(" ")],
)

I've never worked with AST's but It's suspicious to me that the value_token in the first log doesn't show a -1. 🤔 Have I found a deeper parser bug or am I misunderstanding where the issue lies?

@ematipico
Copy link
Member

Thank you! I will assign the issue to you then, feel free to use this issue to ask all your questions.

The log may belong to another use case.

For testing and debugging, we usually recommend another approach.

If you want to inspect the AST of some source code, we usually recommend:

  • using the playground
  • using VSCode the extension (if you use VSCode). There is a palette command that shows the AST

I believe the issue is lurking here, but I haven't looked at the code for a while:

if let Some(tabindex_static) = tabindex_attr.as_static_value() {
let tabindex_text = tabindex_static.text();
let tabindex_val = tabindex_text.trim().parse::<i32>();
if let Ok(num) = tabindex_val {
return (num >= 0).then_some(());
}
}

@anthonyshew
Copy link
Contributor Author

Beautiful, that was the block I was logging from. 😄 Will do some more investigating with this refined debugging loop.

@ematipico
Copy link
Member

Closed by #3708

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages S-Bug-confirmed Status: report has been confirmed as a valid bug
Projects
None yet
Development

No branches or pull requests

2 participants