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

Add null-check to aria_utils verifyLabelsBySelector #44361

Merged
merged 2 commits into from
Feb 3, 2024

Conversation

dholbert
Copy link
Contributor

@dholbert dholbert commented Feb 1, 2024

Before this commit, verifyLabelsBySelector calls test_driver.get_computed_label and then does string-manipulation on the result. For some reason, this get_computed_label invocation returns null in Firefox, and so the string-manipulation causes a JS exception to be thrown, which makes for awkward test failure results.

Let's just explicitly assert that the result is not-null, to make that expectation clearer and to give a cleaner test-failure in cases where it is unexpectedly null.

Before this commit, verifyLabelsBySelector calls test_driver.get_computed_label
and then does string-manipulation on the result. For some reason, this
get_computed_label invocation returns null in Firefox, and so the
string-manipulation causes a JS exception to be thrown, which makes for awkward
test failure results.

Let's just explicitly assert that the result is not-null, to make that
expectation clearer and to give a cleaner test-failure in cases where it is
unexpectedly null.
@dholbert
Copy link
Contributor Author

dholbert commented Feb 1, 2024

For reference, I noticed this coming in to play on this WPT in particular:
https://wpt.fyi/results/css/css-display/accessibility/display-contents-role-and-label.html

Before this PR, Firefox has some failures there that look like this:
image

Fail	Label: g element with display: contents, as child of svg, is labelled via title element	promise_test: Unhandled rejection with value: object "TypeError: can't access property "replace", computedLabel is null"
Asserts run
No asserts ran

After this PR, that failure looks like this instead:
image

Fail	Label: g element with display: contents, as child of svg, is labelled via title element	assert_not_equals: get_computed_label(el) shouldn't return null for <g style="display: contents;" fill="silver" stroke="blue" data-expectedlabel="group label" data-expectedrole="group" data-testname="g element with display: contents, as child of svg, is labelled via title element" class="ex-role-and-label ex-label-only ex-role-only"> <title>group label</title> <circle cx="20" cy="20" r="10"></circle> <circle cx="20" cy="20" r="10"></circle> </g> got disallowed value null

verifyLabelsBySelector/<@http://web-platform.test:8000/wai-aria/scripts/aria-utils.js:147:26

Asserts run
Fail	

assert_not_equals(null, null, "get_computed_label(el) shouldn't return null for <g style=\"display: contents;\" fill=\"silver\" stroke=\"blue\" data-expectedlabel=\"group label\" data-expectedrole=\"group\" data-testname=\"g element with display: contents, as child of svg, is labelled via title element\" class=\"ex-role-and-label ex-label-only ex-role-only\">\n      <title>group label</title>\n      <circle cx=\"20\" cy=\"20\" r=\"10\"></circle>\n      <circle cx=\"20\" cy=\"20\" r=\"10\"></circle>\n    </g>")
verifyLabelsBySelector/< /wai-aria/scripts/aria-utils.js:147:26

(I'm including el.outerHTML for consistency with the other nearby assertions. This makes the failure pretty verbose, but substantially more informative/actionable than the promise_test: Unhandled rejection with value: object "TypeError: ..." result that we were getting before.)

@dholbert dholbert requested a review from rahimabdi February 2, 2024 19:44
Copy link
Contributor

@cookiecrook cookiecrook left a comment

Choose a reason for hiding this comment

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

Approving with one formatting nit diff. Thanks.

wai-aria/scripts/aria-utils.js Outdated Show resolved Hide resolved
@dholbert dholbert enabled auto-merge (squash) February 3, 2024 15:50
@dholbert dholbert merged commit faa6d42 into web-platform-tests:master Feb 3, 2024
17 checks passed
mbrodesser-Igalia pushed a commit to mbrodesser-Igalia/wpt that referenced this pull request Feb 19, 2024
…ts#44361)

* Add null-check to aria_utils verifyLabelsBySelector

Before this commit, verifyLabelsBySelector calls test_driver.get_computed_label
and then does string-manipulation on the result. For some reason, this
get_computed_label invocation returns null in Firefox, and so the
string-manipulation causes a JS exception to be thrown, which makes for awkward
test failure results.

Let's just explicitly assert that the result is not-null, to make that
expectation clearer and to give a cleaner test-failure in cases where it is
unexpectedly null.
marcoscaceres pushed a commit that referenced this pull request Feb 23, 2024
* Add null-check to aria_utils verifyLabelsBySelector

Before this commit, verifyLabelsBySelector calls test_driver.get_computed_label
and then does string-manipulation on the result. For some reason, this
get_computed_label invocation returns null in Firefox, and so the
string-manipulation causes a JS exception to be thrown, which makes for awkward
test failure results.

Let's just explicitly assert that the result is not-null, to make that
expectation clearer and to give a cleaner test-failure in cases where it is
unexpectedly null.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants