-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
initial pass at updating inert content #8122
Conversation
nitial pass at updating inert content incorporating content suggestions from @jcsth, @alice and @cookiecrook. This is not a complete PR at this point, but wanted to get some content up for at least some initial review. Was hoping to get some other specific examples in place of what to do / not do.
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.
A few nits, but overall I'm happy with this. Who are the main stakeholders whose buy-in we should wait for before merging?
thanks @domenic. I would have accepted many of your suggestions but for the fact GitHub keeps erroring out on me when I do so. If it's not giving you trouble to merge, you can go ahead and do so, or I can make the changes in my local file and re-push. Whichever you prefer. Definitely want @cookiecrook, @jcsteh and @aleventhal. I mentioned in the PR, but many of the additions here were from feedback from @jcsteh and @alice - so if I have not done what was necessary to make sure they get credited as co-authors, I'd very much like to make sure they will be once this has been given its further review. I had mentioned that I would add some more examples/guidance to this PR, but now I'm wondering if MDN is not the place for that. Does anyone have a preference for that? |
Yeah, the HTML source file is too big for GitHub to handle. Please feel free to make the changes locally and then push. We can be sure to add them as co-authors using Co-authored-by in the commit message when it comes time to merge! I generally like putting examples in the spec, myself, so it might indeed be worth including them. |
ok thanks. will get those updates in and add in an example or two |
pull in edits/suggestions from @domenic’s review
@domenic had made the suggested edits/fixes from your previous review and have added in an example. |
c7d6b1d
to
6893830
Compare
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.
LGTM! How long should we wait for reviews from the others, do you think?
Also, it looks like you're no longer passing the participation checker, so we should get that sorted out... feel free to reach out if we can help.
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.
+1 to @marcoscaceres's suggestion. Otherwise LGTM.
The worked example in particular is a thing of beauty; belated thanks for doing this work. |
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.
Thanks so much for working on this. This looks great and definitely addresses my concerns.
data-x="concept-command">commands</span> will also get disabled.</p> | ||
<p class="note">Inert nodes generally cannot be focused, and user agents do not expose the inert | ||
nodes to accessibility APIs or assistive technologies. Inert nodes that are <span | ||
data-x="concept-command">commands</span> will become inoperable to users.</p> | ||
|
||
<p>User agents may allow the user to override the restrictions on search and text selection, |
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.
When we say "search" here, do we mean "find-in-page" or something else? If "find-in-page", does that contradict the "The user agent should ignore the node for the purposes of find-in-page" statement above?
If the user can override the search and text selection restrictions, since inert content isn't exposed to a11y APIs, that would make this override inaccessible to users depending on a11y APIs.
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.
It's find-in-page (I'll update it). This sentence is specifically meant to override the above sentence, saying that the above is the default behavior web authors specify, but users should be able to override the default behavior.
In practice, this is not implemented anywhere. But you could imagine a checkbox in the Find-in-page dialog/bar that said something like "Search even inert content", or some keybind that you hold down which lets you select even inert content.
Presumably any overriding the user agent allows here should also carry over to AT. Do you have a suggestion on how to phrase that?
Or maybe we should remove this "should" since no user agent has ever respected it...
Edit: I realized that it's not a "should", it's a "may". That makes the case for removing it weaker...
I fixed a few of the code review comments, and will merge this since it's been sitting around for some time. However there's still a discussion going on about this paragraph (which is preexisting before this PR):
Since that paragraph is preexisting, I don't think it should block merging this. But if @jcsteh or others want to discuss it further, we can definitely do that in a new issue. |
@domenic thank you! |
initial pass at updating inert content
incorporating content suggestions from @jcsth, @alice and @cookiecrook.
This is not a complete PR at this point, but wanted to get some content up for at least some initial review. Was hoping to get some other specific examples in place of what to do / not do.
Any feedback on what's here so far is very welcome. Thanks
closes #7796
/interaction.html ( diff )