-
Notifications
You must be signed in to change notification settings - Fork 126
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
Clarify wording in "Including Elements in the Accessibility Tree" section #1100
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is better than the original, but still not sufficiently clear.
The item that addresses aria-activedescendant is not accurate. I provided a suggestion. It's a tricky one to get right. I hope the suggestion is clear. Alternatively, because there are 4 important conditions, it might be more clear if those conditions were formatted in a sublist. However, that would make the list structure itself a bit more complex by creating 3 layers of conditions.
- remove "already" - merge "aria-hidden" item into "focusable" item
@mcking65 please re-review |
@jnurthen @joanmarie @mcking65 Please review this. Other recent issues have been quoting the including/excluding sections that this PR covers, and I'm worried that pieces of the old sections may be reworded, and then we will have complicated conflicting words and thoughts to merge. It would be way easier to merge this first, before people start layering on new content. |
- add meter - remove math
@jnurthen @joanmarie @mcking65 - ping. Note that this PR will now close #1172 as well (from the wide review). |
I'm not sure, if the following at role=presentation is clear enough:
Because not all descendants are included, but only those that are not required for the current role with role=presentation |
The information is there... but unfortunately finding it requires reading the whole presentation role section, and not just the conflict resolution subsection. Specifically, these 3 paragraphs, when taken together, explain fairly clearly:
So I will keep the sentence you pointed out:
but I will change the sentence right after that to point to the presentation role section instead of the conflict resolution subsection:
|
@jnurthen Build is passing, there are no conflicts, and all review comments have been addressed, so even though pr-preview is failing, the 2 links in the opening comment still show an up-to-date preview: So, this PR is ready for final review. |
@cookiecrook @jnurthen @joanmarie @mcking65 Please review. The preview links are good. Note that both the Excluding elements and the Including elements sections have changes. The diff is broken - it's only showing deletions for some reason - so you'll have to look at the code to see what changed. 😄 Please note: this PR and it's (closed) parent PR have been waiting for almost a year (April 11, 2019). All previous review comments have been addressed, but there were enough changes that this should be reviewed again by all concerned parties. :) Also please note that this contains changes that address wide review comments for 1.2. It would be great if this could go in really soon, because I've started forgetting what's in here and accidentally making some of the same changes again. 😅 |
- adds math back in to children presentational list
@cookiecrook @mcking65 @joanmarie please re-review. I need to merge this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The aria-activedescendant wording is still not quite accurate. Made a suggestion that avoids duplicating the aria-activedescendant specification.
Co-Authored-By: Matt King <[email protected]>
Make sure "focusable" will point to definition in aria-common
@cookiecrook @joanmarie @mcking65 All comments addressed. Ready for re-re-review. :) Note that I had to add the definition of focusable over in w3c/aria-common#39. Here's the prose:
I was afraid to open cans of worms if I mentioned tabindex or activedescendant. Keep it simple... Let me know if any further changes are needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting pretty close. Thanks @carmacleod
for consistency with other points.
Just waiting on 2 things:
|
@carmacleod wrote:
We can address that in a follow-up issue. I don't think it needs to delay this one... Marking PR as Reviewed. |
Only waiting on 1 thing: @jamesn @cookiecrook Please review PR w3c/aria-common#39 - you may not be receiving notifications from that repo. All that PR does is add a definition for focusable. For convenience, here are the words:
@jnurthen Please merge this (#1100) as soon as possible. Today would be good. People are still trying to make conflicting changes in these 2 sections because this PR (and its parent) have been sitting here for so long. I don't think you need to wait for @joanmarie and @mcking65 to re-re-review. They have seen it many times. |
…tion (#1100) Co-Authored-By: Matt King <[email protected]> Co-authored-by: James Nurthen <[email protected]>
…tion (#1100) Co-Authored-By: Matt King <[email protected]> Co-authored-by: James Nurthen <[email protected]>
Resolves #841
Starting fresh with only the relevant changes from PR #950 to ensure that nothing bogus gets merged.
So... "take two": @cookiecrook @jnurthen @joanmarie @mcking65
Preview (#tree_exclusion) (#tree_inclusion) | Diff