ActionMenu.Anchor should only accept button

[siddharthkp]

Description

ActionMenu has preferred API of ActionMenu.Button which uses a Primer Button and wires it up correctly.

However, if you want to customise the anchor, we also provide a ActionMenu.Anchor that can be used to give a custom element.

While this API is required, it's possible to use it incorrectly.

Spotted in the wild: In this example, the developer is trying to add an "active indicator" on the IconButton by adding an additional element (with aria-label) and positioning it

<ActionMenu.Anchor>
  <div className="relative">
    <IconButton aria-label="Filter files in tree" icon={FilterIcon} />
    {filterEnabled && (
      <div aria-label="Showing only files changed" className="active-indicator" />
    )}
  </div>
</ActionMenu.Anchor>

The above JSX renders inaccessible html:

<div
  class="relative"
  id=":rku:" <!-- used to label the menu when open, this should have been on the button? -->
  aria-haspopup="true"
  aria-expanded="false"
  tabindex="0" <!-- tabindex=0 added by ActionMenu to make sure anchor gets focus -->
>
  <button
    data-component="IconButton"
    type="button"
    aria-labelledby=":rl0:" <!-- points to tooltip -->
    aria-describedby=":rl1:-loading-announcement"
  >
    <svg aria-hidden="true"></svg>
  </button>
  <span
    class="Tooltip__StyledTooltip-sc-e45c7z-0 iBBTma"
    id=":rl0:"
    aria-hidden="true"
    popover="auto"
  >
    Filter files in tree
  </span>
  <div aria-label="Showing only changed files" class="absolute active-indicator"/>
</div>

iconbutton-grouped.mov

Video description:

1. Pressing tab on the close button seems to focus the filter button but does not show tooltip.
2. The screen reader reads out "Filter files in tree, Showing only changed files, menu pop-up, group"
3. You'd expect tabbing again would focus the text input, but it focuses the button instead. Now a tooltip is visible with text "Filter files in tree".
4. The screen reader now reads "Filter files in tree, button, Filter files in tree, Showing only changed files, menu pop-up, group" (still reading out the group)
5. Tabbing again finally focuses the text input

---

Proposed Solution

I have 2 suggestions:

1. Reduce: The most common use case of ActionMenu.Anchor is to use an IconButton, we should create a shortcut ActionMenu.IconButton as a companion to ActionMenu.Button.
   A blessed shortcut would reduce the chances of implementing it incorrectly.
2. Validate: ActionMenu.Anchor should validate it's children, if it receives an incorrect element as the root, it should throw a warning and guide the developer to correct usage.
   My guess is that only button is valid, but we need to validate that assumption. Non-interactive element is definitely a violation. For prior art, we have similar (if not more advanced) checks in Tooltip

Suggested prioritisation:

I have fixed the instance where this was spotted so I am not blocked.

But there are 258 instances of ActionMenu.Anchor that need to be audited for their children to decide if this is a widespread bug or a good to have

---

Steps to reproduce

Navigate to custom anchor story and replace the Anchor with:

<ActionMenu.Anchor>
  <div className="relative">
    <IconButton aria-label="Filter files in tree" icon={FilterIcon} />
    {filterEnabled && (
      <div aria-label="Showing only files changed" className="active-indicator" />
    )}
  </div>
</ActionMenu.Anchor>

Version

v37.5.0

Browser

Chrome

[joshblack]

Did a quick scan with Primer Query as a part of FR to try to evaluate scope of this change, some overall info:

• Around 140 instances of ActionMenu.Anchor (cutting in half the total amount since I think this query is double-counted)
• A lot of instances correctly use IconButton, the remaining ones use:
  • ActionList.Item
  • Button
  • Token

[server-mm404]

Description

ActionMenu has preferred API of ActionMenu.Button which uses a Primer Button and wires it up correctly.

However, if you want to customise the anchor, we also provide a ActionMenu.Anchor that can be used to give a custom element.

While this API is required, it's possible to use it incorrectly.

Spotted in the wild: In this example, the developer is trying to add an "active indicator" on the IconButton by adding an additional element (with aria-label) and positioning it

<ActionMenu.Anchor>
  <div className="relative">
    <IconButton aria-label="Filter files in tree" icon={FilterIcon} />
    {filterEnabled && (
      <div aria-label="Showing only files changed" className="active-indicator" />
    )}
  </div>
</ActionMenu.Anchor>

The above JSX renders inaccessible html:

<div
  class="relative"
  id=":rku:" <!-- used to label the menu when open, this should have been on the button? -->
  aria-haspopup="true"
  aria-expanded="false"
  tabindex="0" <!-- tabindex=0 added by ActionMenu to make sure anchor gets focus -->
>
  <button
    data-component="IconButton"
    type="button"
    aria-labelledby=":rl0:" <!-- points to tooltip -->
    aria-describedby=":rl1:-loading-announcement"
  >
    <svg aria-hidden="true"></svg>
  </button>
  <span
    class="Tooltip__StyledTooltip-sc-e45c7z-0 iBBTma"
    id=":rl0:"
    aria-hidden="true"
    popover="auto"
  >
    Filter files in tree
  </span>
  <div aria-label="Showing only changed files" class="absolute active-indicator"/>
</div>

iconbutton-grouped.mov
Video description:

1. Pressing tab on the close button seems to focus the filter button but does not show tooltip.
2. The screen reader reads out "Filter files in tree, Showing only changed files, menu pop-up, group"
3. You'd expect tabbing again would focus the text input, but it focuses the button instead. Now a tooltip is visible with text "Filter files in tree".
4. The screen reader now reads "Filter files in tree, button, Filter files in tree, Showing only changed files, menu pop-up, group" (still reading out the group)
5. Tabbing again finally focuses the text input

Proposed Solution

I have 2 suggestions:

1. Reduce: The most common use case of ActionMenu.Anchor is to use an IconButton, we should create a shortcut ActionMenu.IconButton as a companion to ActionMenu.Button.
   A blessed shortcut would reduce the chances of implementing it incorrectly.
2. Validate: ActionMenu.Anchor should validate it's children, if it receives an incorrect element as the root, it should throw a warning and guide the developer to correct usage.
   My guess is that only button is valid, but we need to validate that assumption. Non-interactive element is definitely a violation. For prior art, we have similar (if not more advanced) checks in Tooltip

Suggested prioritisation:

I have fixed the instance where this was spotted so I am not blocked.

But there are 258 instances of ActionMenu.Anchor that need to be audited for their children to decide if this is a widespread bug or a good to have

Steps to reproduce

Navigate to custom anchor story and replace the Anchor with:

<ActionMenu.Anchor>
  <div className="relative">
    <IconButton aria-label="Filter files in tree" icon={FilterIcon} />
    {filterEnabled && (
      <div aria-label="Showing only files changed" className="active-indicator" />
    )}
  </div>
</ActionMenu.Anchor>

Version

v37.5.0

Browser

Chrome

[siddharthkp]

@joshblack This is great, thank you for pulling this information! Super nice that there is no nesting and it's always a primer component 🎉

• Button is perfect (not sure why they are not using ActionMenu.Button, but okay)
• ActionList.Item: will need to see the use cases, depends on the role I guess?
• Token: Anchor would make this interactive, just need to make sure if it's wired up correctly then

Side note: How do you pull information about children from primer query, do you also have also have the files so that we can look at the code for these instances?