RFC: Triggers and multiple layers of button handling#24960
Conversation
b711d92 to
3fbc4de
Compare
📊 Bundle size report🤖 This report was generated against b48083b3009bc75f28c328de0024eb400b989145 |
|
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:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: b48083b3009bc75f28c328de0024eb400b989145 (build) |
| </Trigger> | ||
| ``` | ||
|
|
||
| That way, even for `Button`, `useARIAButtonProps` will end up providing a bunch of unnecessary props. |
There was a problem hiding this comment.
Is it the problem? Will be something broken?
There was a problem hiding this comment.
Yup, that's the problem, nothing will be broken. It's just a bunch of redundant listeners and properties such as role and tabIndex
There was a problem hiding this comment.
But redundant properties will break nothing, correct?
There was a problem hiding this comment.
Yes, nothing will be broken, they'll just override one another.
There was a problem hiding this comment.
Should be then a third option in RFC called "Do nothing"? 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
similarly keydown, keyup and click handlers on button should be respected
|
|
||
| #### Cons | ||
|
|
||
| 1. won't work with `CustomButton` wrapping `Button` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
wrapping is very common. moreover we have scenarios where a button is wrapped with popover and a tooltip.
3fbc4de to
5e151a8
Compare
5e151a8 to
400bc78
Compare
400bc78 to
4531e7c
Compare
ling1726
left a comment
There was a problem hiding this comment.
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 I see that the RFC was merged, but what solution is a way to go and what are discarded? Can you please update it? |
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.That way, even for
Button,useARIAButtonPropswill end up providing a bunch of unnecessary props.