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

AccName: additional text node tests #42407

Merged
merged 11 commits into from
Nov 3, 2023

Conversation

cookiecrook
Copy link
Contributor

@cookiecrook
Copy link
Contributor Author

cookiecrook commented Oct 6, 2023

@sivakusayan @aleventhal @MelSumner @jnurthen @jcsteh

Do you agree with my test expectations about interior whitespace? (currently lines 56–72, but that could change)

For example, several tests are failing across the board (see CI results in the checks above: WebKit, Chromium, Gecko) b/c of a distinction between regular space chars and non-breaking space chars. It's interior space, which is less strictly specified than leading/trailing space. I think my pasteboard even normalized the differences here:

!EQ("button label", "button label")

Of Note

The WPT helper method verifyLabelsBySelector() is explicitly trimming leading/trailing whitespace b/c in most cases, it doesn't matter.
https://github.com/web-platform-tests/wpt/blob/master/wai-aria/scripts/aria-utils.js#L141

We could also normalize interior whitespace to match (these tests would then pass across the board) and add a new method that preserves whitespace exactly, which we could use on a limited number of tests.

@MelSumner
Copy link
Contributor

We could also normalize interior whitespace to match (these tests would then pass across the board) and add a new method that preserves whitespace exactly, which we could use on a limited number of tests.

This is my preference FWIW. Maybe with a comment in there to revisit in the future. I think there are some whitespace issues that need to be resolved just because we can't seem to get consensus on where they should be or where they should be compressed due to different interpretations and implementations.

@cookiecrook
Copy link
Contributor Author

cookiecrook commented Oct 11, 2023

Seems like that’s the consensus from the related AccName issue too, so I’ll make it happen. Thanks all.

@cookiecrook cookiecrook marked this pull request as draft October 11, 2023 20:59
@cookiecrook cookiecrook marked this pull request as ready for review October 12, 2023 01:26
@cookiecrook cookiecrook requested a review from rahimabdi October 12, 2023 01:28
@cookiecrook cookiecrook requested a review from zcorpan October 13, 2023 01:49
wai-aria/scripts/aria-utils.js Outdated Show resolved Hide resolved
wai-aria/scripts/aria-utils.js Outdated Show resolved Hide resolved
@cookiecrook cookiecrook marked this pull request as draft October 13, 2023 06:09
don't normalize/trim the expectation, only the result
@cookiecrook cookiecrook marked this pull request as ready for review October 13, 2023 07:16
@cookiecrook
Copy link
Contributor Author

Recycling CI.

@cookiecrook cookiecrook reopened this Oct 16, 2023
wai-aria/scripts/aria-utils.js Outdated Show resolved Hide resolved
wai-aria/scripts/aria-utils.js Outdated Show resolved Hide resolved
thx jugglinmike

Co-authored-by: jugglinmike <[email protected]>
Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

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

Sorry I missed this on my first review!

accname/name/comp_text_node.html Outdated Show resolved Hide resolved
Copy link
Contributor

@rahimabdi rahimabdi left a comment

Choose a reason for hiding this comment

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

Looks great!


<h1>text/element/text nodes, no space</h1>
<span role="button" tabindex="0" class="ex" data-expectedlabel="buttonlabel" data-testname="span[role=button] with text/element/text nodes, no space">button<span></span>label</span>
<div role="heading" class="ex" data-expectedlabel="headinglabel" data-testname="div[role=heading] with text/element/text nodes, no space">heading<span></span>label</div>
Copy link
Contributor

@rahimabdi rahimabdi Nov 3, 2023

Choose a reason for hiding this comment

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

The test looks good, somewhat related observation:

I noticed that HTML validation passes for this role="heading" instance without aria-level however, it's currently a MUST that aria-level be provided. Should this fail validation?

(This seems related to ARIA-only parsing errors like lack of explicit grouping for role="radio").

Copy link
Contributor Author

@cookiecrook cookiecrook Nov 3, 2023

Choose a reason for hiding this comment

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

It's an Author MUST, but I don't think it will change the results of these tests... If you know otherwise (e.g. missing aria-level changes the computedlabel?), then we should have a new file testing how that and similar required attributes affect the label... Otherwise, heading level will get tested after we have a direct accessor to it. (See WICG/aom#197 and WICG/aom#203)

@cookiecrook cookiecrook merged commit f027bb0 into web-platform-tests:master Nov 3, 2023
19 checks passed
@cookiecrook cookiecrook deleted the comp_text_node branch November 3, 2023 21:45
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.

AccName: tests/todos in accname/name/comp_text_node
7 participants