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

Add command & commandfor attribute related mappings #2354

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

keithamus
Copy link
Member

@keithamus keithamus commented Oct 11, 2024

This adds the mapping for command and commandfor attributes as per the Invokers Explainer.

Additions:

Test, Documentation and Implementation tracking

Once this PR has been reviewed and has consensus from the working group, tests should be written and issues should be opened on browsers. Add N/A and check when not applicable.

  • "author MUST" tests:
  • "user agent MUST" tests:
  • Browser implementations (link to issue or commit):
    • WebKit:
    • Gecko:
    • Blink:
  • Does this need AT implementations?
  • Related APG Issue/PR:
  • MDN Issue/PR:

Copy link

netlify bot commented Oct 11, 2024

Deploy Preview for wai-aria ready!

Name Link
🔨 Latest commit 2483768
🔍 Latest deploy log https://app.netlify.com/sites/wai-aria/deploys/6760498137044300084d35a8
😎 Deploy Preview https://deploy-preview-2354--wai-aria.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@scottaohara scottaohara self-requested a review October 17, 2024 17:14
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.

feel free to reach out if you have questions / want to talk through what i was suggesting

html-aam/index.html Outdated Show resolved Hide resolved
html-aam/index.html Outdated Show resolved Hide resolved
html-aam/index.html Outdated Show resolved Hide resolved
html-aam/index.html Outdated Show resolved Hide resolved
html-aam/index.html Outdated Show resolved Hide resolved
html-aam/index.html Outdated Show resolved Hide resolved
@keithamus
Copy link
Member Author

Okay @scottaohara I've expanded it as you described, I think it makes a lot more sense. Now each state has its own table which prescibes how that state should map. I've included all currently specified states, toggle/show/hide popover, close, and show modal.

@keithamus keithamus force-pushed the add-commandfor-attribute-related-mappings branch from 345eeda to 0b1053c Compare October 17, 2024 22:29
@scottaohara
Copy link
Member

ok i think this can work.

we still need attribute tables for command and command for. but as far as i'm concerned, those attribute tables could be simplified to just list the attribute, and point to the instances you made in the element section.

e.g., like how the type attribute for inputs is listed - but simpler... i could see that just removing all the individual mapping rows and in the comments, just link to the first of the button (command in the x state) tables you made.

(mental note to reduce redundancy and unnecessary page link by squashing all "not mapped" or "see ..." rows into a single comment row for other attributes too...)

html-aam/index.html Outdated Show resolved Hide resolved
html-aam/index.html Outdated Show resolved Hide resolved
html-aam/index.html Outdated Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Dec 4, 2024

whatwg/html#9841 is close to ready. Is this expected to be merged soon @scottaohara & @aleventhal? Ideally we can merge both around the same time.

taking aaron's feedback, re-combined the popover commands into a single table, since they all should match.

similarly, combined the close and show-modal into a single table - as these also should have matched.  this means i removed all the extra aria-expanded mappings that were in the close command.  but we dont need these.  a close button won't exist outside of a modal dialog, since it'd be inert.  and we don't need expanded states on the close button, because someone is already within the dialog.
moves the command tables to the attributes section.  otherwise, a new table for command would have been necessary, and it would have then just pointed to these other tables.  so, moving them down here negates the need for that.

 adds the commandfor attribute mapping table
@scottaohara scottaohara self-requested a review December 10, 2024 19:00
html-aam/index.html Outdated Show resolved Hide resolved
@scottaohara
Copy link
Member

@keithamus i have taken some of aaron's feedback and reconfigured this a bit.

The related states are now their own mapping tables popover (toggle,show,hide) and dialog (close, show-modal). These have been moved to the attribute section, rather than in the element section. This was more just a partial re-consolidation and moving around, where the only major change I made was removed the mappings for aria-expanded for the close command. Exposing that state should have been similar to a close button within a popover, where the expanded state isnt' necessary to communicate in that context.

i added the missing commandfor table - but it's largely there to indicate that it serves as a way to associate the button and the element it commands - but again the mappings under the command states are where any programmatic associations that need to be exposed to users are defined.

cc @aleventhal @jcsteh @cookiecrook / @rahimabdi for reviews for your platforms - but not much should be different here since these mappings were already similarly specified for popover/popovertarget.

I'm particularly curious if Webkit is looking to implement any of the aria-details relationships that chrome/firefox have implemented, and if anything more specific needs to be added to the ax row of the command=show,hide,toggle (or popovertarget for that matter)

@scottaohara scottaohara self-requested a review December 10, 2024 19:54
@scottaohara
Copy link
Member

@annevk pending reviews, i think @keithamus did a good job getting this setup, and i spent parts of today finishing it up.
pinged reviewers in my previous comment.

@cookiecrook cookiecrook self-requested a review December 12, 2024 22:54
Copy link
Contributor

@cookiecrook cookiecrook left a comment

Choose a reason for hiding this comment

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

@muan and @nt1m may have some comments on this in the context of:

html-aam/index.html Outdated Show resolved Hide resolved
Comment on lines +9299 to +9317
<div class="general">Not mapped</div>
</td>
</tr>
<tr>
<th><a href="https://msdn.microsoft.com/en-us/library/ms726297%28v=VS.85%29.aspx">UIA</a></th>
<td>
<div class="general">Not mapped</div>
</td>
</tr>
<tr>
<th><a href="https://gnome.pages.gitlab.gnome.org/atk/">ATK</a></th>
<td>
<div class="general">Not mapped</div>
</td>
</tr>
<tr>
<th><a href="https://developer.apple.com/reference/appkit/nsaccessibility">AX</a></th>
<td>
<div class="general">Not mapped</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think TBD may be better than not mapped in these cases? "Not mapped" always seems purposeful rather than, "TBD," aka "let's determine how to map this before publishing the next snapshot."

Co-authored-by: James Craig <[email protected]>
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