feat: expect(locator).toHaveAccessibleErrorMessage#33904
Merged
dgozman merged 20 commits intomicrosoft:mainfrom Dec 27, 2024
Merged
feat: expect(locator).toHaveAccessibleErrorMessage#33904dgozman merged 20 commits intomicrosoft:mainfrom
dgozman merged 20 commits intomicrosoft:mainfrom
Conversation
This comment has been minimized.
This comment has been minimized.
e68009a to
cccdd97
Compare
This comment has been minimized.
This comment has been minimized.
cccdd97 to
6623a77
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
95d0978 to
0a4ee89
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0a4ee89 to
c963a6d
Compare
This comment has been minimized.
This comment has been minimized.
yury-s
reviewed
Dec 10, 2024
c963a6d to
2195194
Compare
This comment has been minimized.
This comment has been minimized.
dgozman
reviewed
Dec 13, 2024
f764780 to
139b5dd
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
dgozman
reviewed
Dec 16, 2024
Collaborator
dgozman
left a comment
There was a problem hiding this comment.
This looks very good! Just a few comments about aria spec/semantics, and this will be good to do.
3c01c81 to
41921ed
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
dgozman
reviewed
Dec 17, 2024
Collaborator
dgozman
left a comment
There was a problem hiding this comment.
Thank you for the PR, looks very good!
| export const kInputValidityAttributes = ['required', 'pattern', 'min', 'max', 'minlength', 'maxlength', 'step', 'type', 'value']; | ||
|
|
||
| export function hasValidationAttributes(element: Element): boolean { | ||
| if (!(element instanceof HTMLInputElement)) |
Collaborator
There was a problem hiding this comment.
validity is defined for a lot of elements, e.g. <select>, <textarea>, <object>, <button>, <fieldset>, probably more. I'd drop this check, and assume that element.validity?.valid === false means it is invalid.
| const role = getAriaRole(element) || ''; | ||
| if (!role || !kAriaInvalidRoles.includes(role)) | ||
| return 'false'; | ||
| if (element instanceof HTMLInputElement && hasValidationAttributes(element) && element.validity) |
Collaborator
There was a problem hiding this comment.
Perhaps check both aria-invalid and element.validity?.valid === false, and consider the element invalid when either of the indicators is present?
Contributor
Author
There was a problem hiding this comment.
-
I understood it as being asked to remove hasValidationAttributes and evaluate only with
element.validity?.valid === falsewithout assessing thekAriaInvalidRoles attributes. Would that be correct? :) -
According to the spec, it seems that the return values of the
getAriaInvalidfunction should be'false' | 'true' | 'grammar' | 'spelling'.
Rather than evaluatingvaliditywithin this function, I think it would be better to separate it into a dedicated function, such asgetValidityInvalid. I look forward to your feedback on this! :)
41921ed to
9378431
Compare
This comment has been minimized.
This comment has been minimized.
9378431 to
b28ad83
Compare
This comment has been minimized.
This comment has been minimized.
…essage references
b28ad83 to
cab504b
Compare
Contributor
Test results for "tests 1"8 flaky37461 passed, 650 skipped Merge workflow run. |
dgozman
approved these changes
Dec 27, 2024
Collaborator
dgozman
left a comment
There was a problem hiding this comment.
This looks great! Thank you for the PR! and following through the review process. Merging in.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #31249
toHaveAccessibleErrorMessagematcher forexpect(locator).Looking forward to teams feedback. Thanks! :)