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.

Sign up for GitHub

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

Jump to bottom

fix(label-content-name-mismatch): better dismiss and wysiwyg symbolic text characters #4402

Merged
merged 5 commits into from
Apr 15, 2024
Merged

fix(label-content-name-mismatch): better dismiss and wysiwyg symbolic text characters #4402

merged 5 commits into from
Apr 15, 2024

Conversation

gaiety-deque
Copy link
Contributor

@gaiety-deque gaiety-deque commented Apr 9, 2024 •
edited
Loading

adds exceptions for (dismiss) ×, wysiwyg characters b, aA, abc

fix: #4386

This does not handle all potential use cases. Potential shortcomings worth discussing and opening further issues for are as follows:

@gaiety-deque
fix(label-content-name-mismatch): better dismiss and wysiwyg symbolic…
c215a9d 
… text characters

adds exceptions for (dismiss) ×, wysiwyg characters b, aA, abc

fix: #4386
@gaiety-deque gaiety-deque requested a review from a team as a code owner April 9, 2024 19:28
@CLAassistant
Copy link

CLAassistant commented Apr 9, 2024 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@gaiety-deque
fix(label-content-name-mismatch): fewer blocklists, handle all single…
0cd9d71 
… char alpha the same

fix: #4386
@gaiety-deque
fix(label-content-name-mismatch): refactor isHumanInterpretable, test…
9ed16e6 
…s var -> const

isHumanInterpretable was getting lengthy, refactored into several small functions
unit test used var, switched to const

Refs: #4386
straker
straker previously requested changes Apr 11, 2024
View reviewed changes
lib/commons/text/is-human-interpretable.js Outdated Show resolved Hide resolved
test/commons/text/is-human-interpretable.js Show resolved Hide resolved
test/commons/text/is-human-interpretable.js Outdated Show resolved Hide resolved
lib/commons/text/is-human-interpretable.js Outdated Show resolved Hide resolved
@gaiety-deque
Copy link
Contributor Author

Should switch to case insensitive for symbolic text

@straker
Copy link
Contributor

straker commented Apr 11, 2024 •
edited
Loading

As discussed, we'll want to add a test that uses a single CJK character (any of the 3) for the label and a different one for the visible text and show that it returns incomplete, that way we document the behavior (previous to this change CJK would pass/fail for single characters if different).

@gaiety-deque
fix(label-content-name-mismatch): 0 for all single char, cjk & x test…
30d8388 
…, ignore cap

Addresses all feedback on PR #4402

- Simpler symbolic text check, ignoring case, with unit test for both
- Added back "x" and "X" in unit test
- Removed trim step, uneeded
- Reverted set to list, uneeded
- removed /i in non-digit regex
- test for a CJK character
- adjusted integration test labels for single character incompletes, noticed an ordering bug and corrected

Refs: #4386
@gaiety-deque
fix(label-content-name-mismatch): simpler non-digit check, test added
c4aa062 
Followed Steven's advice for a simpler non-digit single character check, added a test to show a digit isn't incompleted

Refs: #4386
@gaiety-deque
Copy link
Contributor Author

As discussed, we'll want to add a test that uses a single CJK character (any of the 3) for the label and a different one for the visible text and show that it returns incomplete, that way we document the behavior (previous to this change CJK would pass/fail for single characters if different).

@straker addressed in 30d8388

@gaiety-deque gaiety-deque dismissed stale reviews from straker and dbjorge April 12, 2024 13:55

requested changes addressed

dbjorge
dbjorge approved these changes Apr 13, 2024
View reviewed changes
Copy link
Contributor

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, LGTM!

lib/checks/label/label-content-name-mismatch-evaluate.js

if (
isHumanInterpretable(accText) < 1 ||
isHumanInterpretable(visibleText) < 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to think about this for a while, but I think I agree with your changes here. I think this creates 2 (but really 1) behavior changes:

  • If accText is not interpretable and visibleText is empty
    • Previously incomplete, now pass
    • This is irrelevant in practice because label-content-name-mismatch-matches filters out cases without visible text
  • If accText is interpretable, visibleText is not intepretable, and visibleText is contained in accText
    • Previously pass, now incomplete
    • Example: <button aria-label="bold">B</button>

Previously I think that case would pass (return true) - with this change, I think it incompletes (returns undefined). I think it's more consistent with the change (I don't think it makes sense for a visible "x" button to give different results for labels "close" vs "exit").

@gaiety-deque gaiety-deque merged commit 56e139a into develop Apr 15, 2024
21 checks passed
@gaiety-deque gaiety-deque deleted the agw--label-content-name-mismatch-exceptions branch April 15, 2024 14:06
@attest-team-ci attest-team-ci mentioned this pull request May 6, 2024
Merged
WilcoFiers added a commit that referenced this pull request May 6, 2024
@WilcoFiers
chore(release): v4.9.1 (#4446)
fff9b9d 
###
[4.9.1](v4.9.0...v4.9.1)
(2024-05-06)

### Bug Fixes

- Prevent errors when loading axe in a page with prototype.js
- **aria-allowed-attr:** allow meter role allowed aria-\* attributes on
meter element
([#4435](#4435))
([7ac6392](7ac6392))
- **aria-allowed-role:** add gridcell, separator, slider and treeitem to
allowed roles of button element
([#4398](#4398))
([4788bf8](4788bf8))
- **aria-roles:** correct abstract roles (types) for
aria-roles([#4421](#4421))
- **aria-valid-attr-value:** aria-controls & aria-haspopup incomplete
([#4418](#4418))
- fix building axe-core translation files with region locales
([#4396](#4396))
([5c318f3](5c318f3)),
closes [#4388](#4388)
- **invalidrole:** allow upper and mixed case role names
([#4358](#4358))
([105016c](105016c)),
closes [#2695](#2695)
- **isVisibleOnScreen:** account for position: absolute elements inside
overflow container
([#4405](#4405))
([2940f6e](2940f6e)),
closes [#4016](#4016)
- **label-content-name-mismatch:** better dismiss and wysiwyg symbolic
text characters
([#4402](#4402))
- **region:** Decorative images ignored by region rule
([#4412](#4412))
- **target-size:** ignore descendant elements in shadow dom
([#4410](#4410))
([6091367](6091367))
- **target-size:** pass for element that has nearby elements that are
obscured ([#4422](#4422))
([3a90bb7](3a90bb7)),
closes [#4387](#4387)


This PR was opened by a robot 🤖 🎉 (And updated by @WilcoFiers
)
@data-sync-user data-sync-user mentioned this pull request Jun 3, 2024
Merged
@wingn8t wingn8t mentioned this pull request Jun 14, 2024
Open
@gurudeep9 gurudeep9 mentioned this pull request Jun 23, 2024
Open
@abdulrahman305 abdulrahman305 mentioned this pull request Jul 3, 2024
Open
@Stanislav1975 Stanislav1975 mentioned this pull request Aug 9, 2024
Open
@Stanislav1975 Stanislav1975 mentioned this pull request Aug 19, 2024
Open
@Stanislav1975 Stanislav1975 mentioned this pull request Sep 9, 2024
Open
@organich organich mentioned this pull request Sep 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbjorge dbjorge dbjorge approved these changes

@straker straker Awaiting requested review from straker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

label-content-name-mismatch incorrectly fails close button with
4 participants
@gaiety-deque @CLAassistant @straker @dbjorge