Fix HeaderButton endIcon styles

[r100-stack]

Changes

Noticed that the HeaderButton's endIcon was not getting the end icon styles similar to the end icon styles in HeaderDropdownButton and HeaderSplitButton (e.g. still have the down/up caret end icon). This PR clones endIcon in HeaderBasicButton to add the iui-header-breadcrumb-button-dropdown-icon class to get those end icon styles. It also adds aria-hidden: true.

Since HeaderBasicButton now adds that class and aria-hidden, HeaderDropdownButton no longer needs to add that class to the endIcon in its internal usage of HeaderBasicButton.

---

This PR is particularly helpful for users who want to show a Popover in HeaderButton but keep the button styles exactly similar to HeaderDropdownButton. This is because passing non-MenuItems in HeaderButton's menuItem prop is bad for accessibility since role: menu expects role: menuitem children (among other supported children).

Code

i.e. The following is not recommended for non-MenuItem content:

<HeaderButton
  name='HeaderButton'
  menuItems={() => [
    <MenuExtraContent>Floating Content</MenuExtraContent>,
  ]}
/>,

Following is the recommended alternative. In this implementation, the endIcon will now have the correct style and accessibility properties:

<Popover
  visible={visible}
  onVisibleChange={setVisible}
  content={
    <Surface>
      <Surface.Body isPadded>Floating Content</Surface.Body>
    </Surface>
  }
>
  <HeaderButton
    name='HeaderButton'
    endIcon={
      visible ? <SvgCaretUpSmall /> : <SvgCaretDownSmall />
    }
  />
</Popover>

Testing

CI still passing.

Tested in the playground:

Before	After

Docs

Added changeset.

[mayank99]

This is a no-go, because it will affect even icons that aren't part of a dropdown/popover button. I do agree that the problem needs to be solved, but this is not the way.

(Also, we should try to avoid cloning nodes and referencing styles within components. But that's not the main reason for blocking this PR.)

[r100-stack]

This is a no-go, because it will affect even icons that aren't part of a dropdown/popover button.

True, but that was actually intended. Because, for example, I felt even if the endIcon is not part of the dropdown/popover Header button, it would be seem incorrect to have a gray icon even when the Header button is active.

I updated the playground linked in the PR description to also show a non dropdown/popover HeaderButton and a regular Button with an endIcon.

After this PR, the endIcon of the HeaderButton will have the same color as the endIcon of the regular Button. Additionally, for the active HeaderButton, the icon will now have the active color:

Before this PR	After this PR

I do agree that the problem needs to be solved, but this is not the way.

Open to other suggestions too 🙂

[mayank99]

This is a no-go, because it will affect even icons that aren't part of a dropdown/popover button.

True, but that was actually intended. Because, for example, I felt even if the endIcon is not part of the dropdown/popover Header button, it would be seem incorrect to have a gray icon even when the Header button is active.

If it's intended, this change should probably be made separately (or maybe this PR can be repurposed). It sounds unrelated to the issue this PR is aimed at solving, i.e. helping consumers use Popover instead of HeaderDropdownButton. I assume it would be a much simpler change to just change the fill.

If you take a look at the code for iui-header-breadcrumb-button-dropdown-icon, it's also adding a negative margin, which is meant for visually balancing the dropdown caret icon. This negative margin shouldn't be set for icons in non-dropdown/non-popover buttons.

I do agree that the problem needs to be solved, but this is not the way.

Open to other suggestions too 🙂

We can discuss potential solutions separately outside this PR. It will require a larger change and maybe new APIs.

One thing that comes to mind is we should probably deprecate HeaderDropdownButton (and maybe HeaderSplitButton too). And then we can recommend using the regular HeaderButton inside DropdownMenu or Popover. This would allow us to conditionally adjust the margin internally, without the consumers needing to think about it.