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

Tweak <aside> conditional mappings to check for more cases #43013

Merged

Conversation

sivakusayan
Copy link
Contributor

Follow-up from this comment.

@wpt-pr-bot
Copy link
Collaborator

There are no reviewers for this pull request. Please reach out on the chat room to get help with this. Thank you!

@sivakusayan sivakusayan changed the title Add contextual role case for <aside> in <main> element Tweak <aside> conditional mappings to check for more cases Nov 8, 2023
@scottaohara scottaohara self-requested a review November 9, 2023 18:24
@cookiecrook
Copy link
Contributor

cookiecrook commented Nov 9, 2023

There may be two similar PRs trampling each other ATM... #43028

@sivakusayan
Copy link
Contributor Author

sivakusayan commented Nov 9, 2023

Yeah, that PR seems to add the role case while this is adding more nesting cases. So I can pull and re-adjust based on what happens in #43028. @joone

@cookiecrook
Copy link
Contributor

Yeah, that PR seems to add the role case while this is adding more nesting cases. So I can pull and re-adjust based on what happens in #43028. @joone

I approved that one since it's a downstream export. I like test names to be even more specific so that we never need to rename and breaking the pass/fail history, so if you don't mind, I'll my editorial test name feedback for that one on this PR.

@cookiecrook
Copy link
Contributor

I merged #43028 so you can cherry-pick 72b8ea6 or rebase. Thanks.

@cookiecrook cookiecrook self-requested a review November 9, 2023 19:11
@sivakusayan
Copy link
Contributor Author

sivakusayan commented Nov 9, 2023

I like test names to be even more specific so that we never need to rename and breaking the pass/fail history, so if you don't mind, I'll my editorial test name feedback for that one on this PR.

Yeah, that should be fine. I'll rebase and apply the test name changes + other feedback later after work (or sometime during the weekend if I don't get to it today).

<main>
<aside data-testname="el-aside-in-main" data-expectedrole="complementary" class="ex">x</aside>
<section>
<aside data-testname="el-aside-in-section-in-main" class="ex-generic">x</aside>
Copy link
Contributor

Choose a reason for hiding this comment

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

noting that while this is what the spec currently says to do, because the section is without a name it is treated no differently than a div in the accTree.

but if that section were given a name, then it would be a role=region, and then it would would be more appropriate for this to be generic.

https://github.com/w3c/html-aam/pull/484/files will eventually address this, but mentioning it here if we want to do anything about it now / modify html aam to at least rectify this single case

Copy link
Contributor Author

@sivakusayan sivakusayan Nov 13, 2023

Choose a reason for hiding this comment

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

Would it make sense to name the <sections> in this WPT change and to ignore the un-named <section> case until w3c/html-aam#484 is merged? That way, browsers that are following to the spec word-for-word aren't dinged yet, but browsers that eagerly implement the behavior in w3c/html-aam#484 can still pass the WPT.

Alternatively, we could include the un-named <section> case here and assert that:

<body>
  <section>
      <aside data-expectedrole="complementary">x</aside>
  </section>
</body>

if people are okay with it, but I don't know if people feel weird about WPT tests that aren't formally in the spec yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is general agreement to the spec change and no objection, I think it's fine to merge the test.

ARIA WG Chairs changed the process so it's now the expectation that any spec change PR will be approved, but not merged until there are 1. tests, and 2. multiple passing implementations. So if we didn't merge the test, it would cause a circular dependency with the spec PR. 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if you two have fully resolved this thread, but the rest of the PR looks good to me, so I'm approving.

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 noticed that w3c/html-aam#484 still seems to be under discussion, so I ended up doing the half measure of just naming the <section> in this PR, while ignoring the un-named <section> case. We can always refine these tests as needed once people seem to come into an agreement there.

<main>
<aside data-testname="el-aside-in-main" data-expectedrole="complementary" class="ex">x</aside>
<section>
<aside data-testname="el-aside-in-section-in-main" class="ex-generic">x</aside>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if you two have fully resolved this thread, but the rest of the PR looks good to me, so I'm approving.

@cookiecrook
Copy link
Contributor

@sivakusayan @scottaohara is this ready to be merged?

@sivakusayan
Copy link
Contributor Author

I think it's ready to merge if @scottaohara does.

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.

5 participants