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

feat: expect(locator).toHaveAccessibleErrorMessage #33904

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

feat: expect(locator).toHaveAccessibleErrorMessage #33904

wants to merge 11 commits into from
+208 −1

Conversation

pengooseDev
Copy link
Contributor

Closes #31249

  • Add new toHaveAccessibleErrorMessage matcher for expect(locator).

Looking forward to teams feedback. Thanks! :)

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@pengooseDev pengooseDev force-pushed the feat-toHaveAccessibleErrorMessage branch 2 times, most recently from e68009a to cccdd97 Compare December 7, 2024 16:10
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@pengooseDev pengooseDev force-pushed the feat-toHaveAccessibleErrorMessage branch from cccdd97 to 6623a77 Compare December 7, 2024 16:12
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@pengooseDev pengooseDev force-pushed the feat-toHaveAccessibleErrorMessage branch from 95d0978 to 0a4ee89 Compare December 8, 2024 03:28
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@pengooseDev pengooseDev force-pushed the feat-toHaveAccessibleErrorMessage branch from 0a4ee89 to c963a6d Compare December 10, 2024 04:46
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
yury-s
yury-s reviewed Dec 10, 2024
View reviewed changes
packages/playwright-core/src/server/injected/roleUtils.ts
@@ -972,6 +1004,8 @@ let cacheAccessibleName: Map<Element, string> | undefined;
let cacheAccessibleNameHidden: Map<Element, string> | undefined;
let cacheAccessibleDescription: Map<Element, string> | undefined;
let cacheAccessibleDescriptionHidden: Map<Element, string> | undefined;
let cacheAccessibleErrorMessage: Map<Element, string> | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

these caches should be reset similar to other in beginAriaCaches/endAriaCaches below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the code to include the missed part. Thanks for the nice feedback. :)

packages/playwright-core/src/server/injected/roleUtils.ts Outdated
accessibleErrorMessage = '';

const ariaInvalid = element.getAttribute('aria-invalid');
if (ariaInvalid === 'true') {
Copy link
Member

Choose a reason for hiding this comment

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

The spec says "Any value not recognized in the list of allowed values MUST be treated by user agents as if the value true had been provided.", should we take this into account here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const ariaInvalid = element.getAttribute('aria-invalid');
const isAriaInvalid = ariaInvalid !== null && ariaInvalid.toLowerCase() !== 'false';

After making the code changes, I added regression tests.
If there's anything I missed, I would appreciate your feedback.

packages/playwright-core/src/server/injected/roleUtils.ts Outdated
const errorMessageId = element.getAttribute('aria-errormessage');
if (errorMessageId) {
// Ensure the ID is valid (no whitespace)
if (!/\s+/.test(errorMessageId)) {
Copy link
Member

Choose a reason for hiding this comment

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

According to the spec it can be an id reference list (i.e. multiple elements), we should handle that case too.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use getIdRefs() helper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review! :)
I've updated the code to handle multiple aria-errormessage references using getIdRefs(). Added a regression test to verify the behavior. Let me know if further changes are needed. :)

@pengooseDev pengooseDev force-pushed the feat-toHaveAccessibleErrorMessage branch from c963a6d to 2195194 Compare December 11, 2024 01:51
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@pengooseDev pengooseDev marked this pull request as draft December 11, 2024 03:56
dgozman
dgozman reviewed Dec 13, 2024
View reviewed changes
packages/playwright-core/src/server/injected/roleUtils.ts Outdated
const errorMessageId = element.getAttribute('aria-errormessage');
if (errorMessageId) {
// Ensure the ID is valid (no whitespace)
if (!/\s+/.test(errorMessageId)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use getIdRefs() helper.

packages/playwright-core/src/server/injected/roleUtils.ts Outdated
@@ -461,6 +461,38 @@ export function getElementAccessibleDescription(element: Element, includeHidden:
return accessibleDescription;
}

export function getElementAccessibleErrorMessage(element: Element, includeHidden: boolean): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like includeHidden: true is never passed, so we might as well not support it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The includeHidden parameter has been removed. I appreciate your feedback! :)

@pengooseDev pengooseDev force-pushed the feat-toHaveAccessibleErrorMessage branch from 2195194 to f764780 Compare December 14, 2024 16:00
@pengooseDev pengooseDev force-pushed the feat-toHaveAccessibleErrorMessage branch from f764780 to 139b5dd Compare December 14, 2024 16:11
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@pengooseDev pengooseDev marked this pull request as ready for review December 14, 2024 17:01
@github-actions GitHub Actions
Copy link
Contributor

Test results for "tests 1"

3 failed
❌ [playwright-test] › runner.spec.ts:118:5 › should ignore subprocess creation error because of SIGINT @macos-latest-node18-1
❌ [webkit-page] › page/page-leaks.spec.ts:82:5 › click should not leak @webkit-ubuntu-22.04-node18
❌ [webkit-page] › page/page-leaks.spec.ts:107:5 › fill should not leak @webkit-ubuntu-22.04-node18

4 flaky ⚠️ [firefox-page] › page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [webkit-library] › library/browsertype-connect.spec.ts:274:5 › launchServer › disconnected event should be emitted when browser is closed or server is closed @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › library/video.spec.ts:381:5 › screencast › should capture navigation @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › page/page-set-input-files.spec.ts:147:3 › should upload large file @webkit-ubuntu-22.04-node18

37337 passed, 650 skipped
✔️✔️✔️

Merge workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgozman dgozman dgozman left review comments

@yury-s yury-s yury-s left review comments

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.

[Feature]: expect(locator).toHaveAccessibleErrorMessage(…)
3 participants
@pengooseDev @yury-s @dgozman