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

RFC: Triggers and multiple layers of button handling #24960

Conversation

bsunderhus
Copy link
Contributor

@bsunderhus bsunderhus commented Sep 27, 2022

PREVIEW

Problem statement

On the case of a custom component being as a trigger child, it's impossible for useARIAButtonProps (which is used internally) to find out if the returned element will be an actual button or not.

<Trigger>
  <Button>Trigger something</Button>
</Trigger>

That way, even for Button, useARIAButtonProps will end up providing a bunch of unnecessary props.

<button onclick="fn" onkeydown="fn" onkeyup="fn" role="button" tabindex="0">Trigger something</button>

@bsunderhus bsunderhus self-assigned this Sep 27, 2022
@bsunderhus bsunderhus marked this pull request as ready for review September 27, 2022 07:23
@bsunderhus bsunderhus force-pushed the rfc/triggers-and-multiple-layers-of-button-handling branch from b711d92 to 3fbc4de Compare September 27, 2022 07:23
@fabricteam
Copy link
Collaborator

fabricteam commented Sep 27, 2022

📊 Bundle size report

🤖 This report was generated against b48083b3009bc75f28c328de0024eb400b989145

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 27, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4531e7c:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@size-auditor
Copy link

size-auditor bot commented Sep 27, 2022

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: b48083b3009bc75f28c328de0024eb400b989145 (build)

@bsunderhus bsunderhus requested review from a team September 27, 2022 07:38
</Trigger>
```

That way, even for `Button`, `useARIAButtonProps` will end up providing a bunch of unnecessary props.
Copy link
Member

Choose a reason for hiding this comment

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

Is it the problem? Will be something broken?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, that's the problem, nothing will be broken. It's just a bunch of redundant listeners and properties such as role and tabIndex

Copy link
Member

Choose a reason for hiding this comment

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

But redundant properties will break nothing, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, nothing will be broken, they'll just override one another.

Copy link
Member

Choose a reason for hiding this comment

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

Should be then a third option in RFC called "Do nothing"? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

we will need to make sure that when tabindex is used explicitly, it won't be overriden by trigger:
should still render a with tabindex -1 and menuitem role.

Copy link
Contributor

Choose a reason for hiding this comment

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

similarly keydown, keyup and click handlers on button should be respected


#### Cons

1. won't work with `CustomButton` wrapping `Button`
Copy link
Member

Choose a reason for hiding this comment

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

This is the biggest issue as it works against our composition model and composition model in React as requires to know about isFluentTriggerComponent. It works for MenuTrigger, PopoverTrigger more or less because there are no reasons to recompose *Trigger components.

Copy link
Contributor

Choose a reason for hiding this comment

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

wrapping is very common. moreover we have scenarios where a button is wrapped with popover and a tooltip.

@bsunderhus bsunderhus force-pushed the rfc/triggers-and-multiple-layers-of-button-handling branch from 3fbc4de to 5e151a8 Compare September 27, 2022 11:07
@bsunderhus bsunderhus marked this pull request as draft October 5, 2022 16:55
@bsunderhus bsunderhus force-pushed the rfc/triggers-and-multiple-layers-of-button-handling branch from 5e151a8 to 400bc78 Compare October 6, 2022 08:04
@bsunderhus bsunderhus marked this pull request as ready for review October 6, 2022 08:54
@bsunderhus bsunderhus force-pushed the rfc/triggers-and-multiple-layers-of-button-handling branch from 400bc78 to 4531e7c Compare October 6, 2022 11:24
Copy link
Member

@ling1726 ling1726 left a comment

Choose a reason for hiding this comment

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

I would go for disableButtonEnhancement as a prop, it would let users fall into the 'pit of success' and there is an out for them if they need it.

Perhaps this could be done for existing triggers, any unstable or future triggers can revert to a behaviour where they expect an ARIA compliant button as a contract ?

@bsunderhus bsunderhus merged commit c5377aa into microsoft:master Oct 12, 2022
@layershifter
Copy link
Member

@bsunderhus I see that the RFC was merged, but what solution is a way to go and what are discarded? Can you please update it?

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