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

Update required owned elements and required context role #1454

Merged
merged 20 commits into from
May 1, 2023

Conversation

smhigley
Copy link
Contributor

@smhigley smhigley commented Apr 13, 2021

Resolves #1033

Updates "Required Owned Elements" to "Allowed Child Elements", and "Required Context Role" to "Required Parent Role", with added definitions for parent, child, controlled element, and controlling element.

I did some support tests for having intermediate elements in composite widgets, and secondary actions in composite widgets, here: https://cdpn.io/smhigley/debug/vYgLZgK

This is still a POC, so I didn't update all the references to required owned elements/context role. Hopefully that makes it a bit easier to see the relevant diffs for discussion.

Should also resolve #1486

PR merge check list


Preview | Diff


Preview | Diff

Copy link
Member

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

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

since this is still a draft, just a few comments/suggestions. hope they're helpful

common/terms.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@smhigley
Copy link
Contributor Author

I updated the PR based on the comments, updated all references to "required owned elements"/"required context role", and moved the aria-controls allowed child stuff to a separate branch. It should be ready for a full review now.

@smhigley smhigley marked this pull request as ready for review April 30, 2021 06:02
common/terms.html Outdated Show resolved Hide resolved
index.html Outdated
@@ -10909,7 +10909,7 @@ <h2>Definitions of States and Properties (all aria-* attributes)</h2>
<p>The default value of <code>aria-busy</code> is <code>false</code> for all elements. When <code>aria-busy</code> is <code>true</code> for an element, assistive technologies MAY ignore changes to content owned by that element and then process all changes made during the busy period as a single, atomic update when <code>aria-busy</code> becomes <code>false</code>.</p>
<p>If it is necessary to make multiple additions, modifications, or removals within a container element that is already either partially or fully rendered, authors MAY set <code>aria-busy</code> to <code>true</code> on the container element before the first change, and then set it to <code>false</code> when the last change is complete. For example, if multiple changes to a <a>live region</a> should be spoken as a single unit of speech, authors MAY set <code>aria-busy</code> to <code>true</code> while the changes are being made and then set it to <code>false</code> when the changes are complete and ready to be spoken.</p>
<p>If an element with role <rref>feed</rref> is marked busy, assistive technologies MAY defer rendering changes that occur inside the <code>feed</code> with the exception of user-initiated changes that occur inside the <rref>article</rref> that the user is reading during the busy period.</p>
<p>If changes to a rendered <rref>widget</rref> would create a state where the <rref>widget</rref> is missing <a href="#mustContain">required owned elements</a> during script execution, authors MUST set <code>aria-busy</code> to <code>true</code> on the <rref>widget</rref> during the update process. For example, if a rendered tree grid required a set of simultaneous updates to multiple discontiguous branches, an alternative to replacing the complete tree element with a single update would be to mark the tree busy while each of the branches are modified.</p>
<p>If changes to a rendered <rref>widget</rref> would create a state where the <rref>widget</rref> is modifying <a href="#mustContain">allowed child roles</a> during script execution, authors MAY set <code>aria-busy</code> to <code>true</code> on the <rref>widget</rref> during the update process. For example, if a rendered tree grid required a set of simultaneous updates to multiple discontiguous branches, an alternative to replacing the complete tree element with a single update would be to mark the tree busy while each of the branches are modified.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Can we just remove this. Does setting aria-busy in this state actually do anything?

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'm happy to remove it, I just didn't want to muddy this PR with other changes if they'd be controversial 😅

@jnurthen
Copy link
Member

Please also move the new terms to index.html. The content of terms.html has been moved there. You'll need to pull the changes from main though to pick this up.

@smhigley
Copy link
Contributor Author

All the previous comments should be addressed. @scottaohara / @aleventhal, how does it look now?

@smhigley smhigley requested a review from cyns May 13, 2021 18:29
@smhigley
Copy link
Contributor Author

also pinging @jcsteh because for some reason github isn't letting me add him to the reviewers ¯_(ツ)_/¯

index.html Outdated Show resolved Hide resolved
index.html Outdated
<p class="note">An element with a <a href="#subclassroles">subclass role</a> of the 'required owned element' does not fulfill this requirement. For example, the <rref>listbox</rref> role requires ownership of an element using the <rref>option</rref> or <rref>group</rref> role. Although the <rref>group</rref> role is the superclass of <rref>row</rref>, adding an <a>owned</a> element with a role of <rref>row</rref> will not fulfill the requirement that <rref>listbox</rref> owns an <rref>option</rref> or a <rref>group</rref>.</p>
<h3>Allowed Child Roles</h3>
<p>Any <a>element</a> that is a direct <a>child</a> of the element with this <a>role</a>. For example, an element with the role <rref>list</rref> may own elements with the role <rref>listitem</rref>, but not elements with the role <rref>option</rref>.</p>
<p>To determine whether an element is the direct <a>child</a> of a role, <a>user agents</a> MUST ignore any elements with the role <rref>generic</rref> or <rref>presentation</rref>.</p>
Copy link

Choose a reason for hiding this comment

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

It's definitely good that we're clarifying what happens concerning generics here. That will clear up implementer confusion like this.

Again, this is clear if you read the definition of "child", but I wonder whether this could be misconstrued as discussing siblings rather than intervening (depth-wise) elements. Perhaps we could clarify this by saying "intervening elements" here?

Also, I assume this isn't a statement about whether intervening generics get exposed in the a11y tree or not, but rather, only a statement about what is considered valid (thus impacting implicit posinset/setsize calculation, etc.)? Pruning intervening generics would be a much bigger change for implementers which would need further discussion.

Copy link
Contributor Author

@smhigley smhigley Jul 1, 2021

Choose a reason for hiding this comment

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

I updated to "intervening elements" -- thanks for the suggestion, I like that better :).

And yup! This is guidance on allowed role structure, and isn't prescriptive of API mappings. The only change is implementers would officially need to support e.g. listbox > generic > option (which most already do), but could do so with or without pruning the generic.

@smhigley
Copy link
Contributor Author

Just want to ping @jcsteh, @scottaohara, and @aleventhal on this to see if the updates have addressed past comments as desired :)

Copy link
Member

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

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

wrote these review comments some time ago... sorry for the delay.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
common/terms.html Outdated Show resolved Hide resolved
common/terms.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@jnurthen
Copy link
Member

jnurthen commented May 2, 2022

should also resolve #1486

@jnurthen
Copy link
Member

jnurthen commented Jun 9, 2022

@smhigley would this PR take care of #1300 ?

@smhigley
Copy link
Contributor Author

@spectranaut, @scottaohara, and @adampage -- I updated with a lot of the suggestions you mentioned, and some comments. Let me know what you think!

Copy link
Contributor

@spectranaut spectranaut 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!

I don't think there are any changes to the validator tests, right, Sarah? Seems to me like this is all clarification/rewording of existing "author MUST" requirements.

@smhigley
Copy link
Contributor Author

@spectranaut I think the validator change would be checking something like this:

<div role="listbox">
  <div>
    <div role="option">hi I'm valid now</div>
  </div>
</div>

@spectranaut
Copy link
Contributor

Oh right what was I thinking, we even talked about this. I just got so wrapped up in the "rewrite" and new terms I forgot about this key change XD

Do you want to do that here, or in a follow up issue? If a follow issue, please make one, otherwise, you can add a validator test here.

index.html Outdated Show resolved Hide resolved
Co-authored-by: Adam Page <[email protected]>
@smhigley
Copy link
Contributor Author

@spectranaut issue is made! #1917

@spectranaut
Copy link
Contributor

@pkra or @jnurthen do you think we can merge this? :)

Copy link
Contributor

@spectranaut spectranaut left a comment

Choose a reason for hiding this comment

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

I spoke too soon once again! The new CI check caught an errant "may", we need to fix that THEN I think it is ready for merge

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Member

@jnurthen jnurthen left a comment

Choose a reason for hiding this comment

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

I think a #mustContain was missed

Co-authored-by: James Nurthen <[email protected]>
@spectranaut spectranaut dismissed stale reviews from cookiecrook and themself May 1, 2023 21:51

outdated

@spectranaut spectranaut merged commit 232ae78 into main May 1, 2023
github-actions bot added a commit that referenced this pull request May 1, 2023
SHA: 232ae78
Reason: push, by spectranaut

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
jnurthen pushed a commit that referenced this pull request Oct 10, 2023
Updates "Required Owned Elements" to "Allowed Child Elements", and "Required Context Role" to "Required Parent Role", with added definitions for parent, child, controlled element, and controlling element.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allowed children of role=tablist includes any role including tabpanel? Clarify "required owned element"