-
Notifications
You must be signed in to change notification settings - Fork 469
fix(ByRole): filter by name or description when hidden #1161
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
fix(ByRole): filter by name or description when hidden #1161
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 64e243b:
|
src/__tests__/role.js
Outdated
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.
Let's keep this test. I don't think it's relevant to the change. If there is something important here, lets's add a new test instead so that we can better track what changed
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 idea was to test the case where an element had a name available to be filtered but is hidden, before that change the name was always empty. May we can add a test specifically for that case? What do you think?
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.
Always add test instead of re-purposing existing tests. Existing tests should only change their assertions if they relied on buggy behavior.
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! I'm learning about all that tests concepts and patterns.
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.
@eps1lon can you check this again? :3
69b1b96 to
71fcffb
Compare
71fcffb to
57aa847
Compare
Codecov Report
@@ Coverage Diff @@ ## main #1161 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 24 24 Lines 998 998 Branches 326 326 ========================================= Hits 998 998
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
The problem is that the accessible name is different if the element is hidden. See #846 (comment)
So this adds a footgun where you just flip hidden and expect name to return the same set. I wasn't convinced by #846 fully to implement this so let's add some concrete, high level use cases to #846 first (e.g. what are you testing?).
Uh oh!
There was an error while loading. Please reload this page.
What: Fixes the ByRole methods to accept filter hidden elements using name and description. Also fixes the logs to show the accessible name in case of not found error. Related to #846
Why: If we have two hidden elements with the same role we can't filter that using the name. Also when we have hidden elements on logs they don't have the name filled, they are always equal
""How: By adding the hidden option when use
computeAccessibleDescriptionandcomputeAccessibleNameto get the accessible description and nameChecklist:
docs site N/A