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

[material-ui][ButtonBase] Allow disabling the ripple when right-clicked (or other buttons) #44154

Open
NotYoojun opened this issue Oct 19, 2024 · 12 comments
Assignees
Labels
component: button This is the name of the generic UI component, not the React module! component: ButtonBase The React component. new feature New feature or request package: material-ui Specific to @mui/material waiting for 👍 Waiting for upvotes

Comments

@NotYoojun
Copy link

NotYoojun commented Oct 19, 2024

Summary

In most cases, a Button doesn't really do something when it's clicked with middle or right buttons. However currently the ripple will still show when clicking a button with any button. I don't think this is really a good idea, since it will mislead the user.

My idea is to add a property similar to 'disableTouchRipple' to all components that have a ripple, but it allows the developer to control which mouse button will let the ripple will be displayed. For example,

// More about mouse button is here: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button#value
<Button showTouchRippleOnButton={[0, 3, 4]} {...other}/>

And the default value of this property should be [0], which means only the main button (usually the left button) will trigger ripples. This property enables the developer to control it, cause sometimes one may listen to, for example, right-click events. In this scenario, this gives the full-control to adapt different using cases.

Examples

You can also define an enum for MouseButtons, or a string-union. In our projects we use an enum to represent mouse buttons like this, and you guys can use it as long as you like:

/**
 * The mouse buttons. This is a subset of the `MouseEvent.button` values.
 * 
 * @remarks buttons may be configured differently to the standard "left to right" layout. 
 * A mouse configured for left-handed use may have the button actions reversed. 
 * Some pointing devices only have one button and use keyboard or other input mechanisms to indicate main, 
 * secondary, auxiliary, etc. Others may have many buttons mapped to different functions and button values.
 * 
 * @link https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button#value
 */
export enum MouseButton
{
    /** Main button, usually the left button or the un-initialized state */
    Main = 0,

    /** Auxiliary button, usually the wheel button or the middle button (if present) */
    Auxiliary = 1,

    /** Secondary button, usually the right button */
    Secondary = 2,

    /** Fourth button, typically the Browser Back button */
    Fourth = 3,

    /** Fifth button, typically the Browser Forward button */
    Fifth = 4
}

Motivation

This idea is really important for our projects. If this can be added, we'll be so happy and appreciated.
And when we have some income, we'll considering buying you guys a cup of coffee by donating.

Thanks for your work!

Search keywords: ripple, right, button, buttonbase, mouse, click

@NotYoojun NotYoojun added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Oct 19, 2024
@NotYoojun NotYoojun changed the title Allow disabling the ripple when a ButtonBase is right-clicked (or other buttons) Allow disabling the ripple when a ButtonBase (and those with a ripple) is right-clicked (or other buttons) Oct 19, 2024
@zannager zannager added component: button This is the name of the generic UI component, not the React module! component: ButtonBase The React component. labels Oct 21, 2024
@mj12albert
Copy link
Member

mj12albert commented Oct 24, 2024

I think it would even make sense to change this to be the default instead of making it configurable, I can't think of a real use-case when I'd want the <Button/> to ripple when clicking any button that's not the left/main button 😬

@NotYoojun
Copy link
Author

NotYoojun commented Oct 24, 2024

@mj12albert I think it's really significant to make it configurable. There are some cases, for example, left-click a button to do a primary action while right-click it to do another action or open a menu of other actions. In some other cases, it's not a Button, it's a list item based on ButtonBase, which will use left-click to select the item, right-click to open a operation menu.

In some industry-inside software like dashboards, the other buttons (like the middle, fourth and fifth) will be used to trigger different actions. I know this is not a really good idea, but it's kinda like industry consensus.

These are not that common thought, but I think it's really necessary to allow developers to make their own choices. It's a good idea to set the default choice to 'only left click raises ripples', but it needs to be configurable.

@DiegoAndai DiegoAndai added new feature New feature or request waiting for 👍 Waiting for upvotes and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 4, 2024
@DiegoAndai
Copy link
Member

Hey @NotYoojun, thanks for the feature request!

I added the waiting for 👍🏼 label so the community can vote for this new feature. If you want to see this implemented, please add a 👍🏼 reaction to the issue's description.

@aarongarciah, what do you think about this? On the one hand, it seems helpful. On the other, it increases the API surface, and there might be better alternatives that don't require adding a new prop, such as improving composition and/or exposing/documenting the ripple utilities.

@DiegoAndai DiegoAndai changed the title Allow disabling the ripple when a ButtonBase (and those with a ripple) is right-clicked (or other buttons) [material-ui][ButtonBase] Allow disabling the ripple when right-clicked (or other buttons) Nov 4, 2024
@DiegoAndai DiegoAndai added the package: material-ui Specific to @mui/material label Nov 4, 2024
@aarongarciah
Copy link
Member

aarongarciah commented Dec 25, 2024

@DiegoAndai I'm not sure about increasing the API surface to support this. Do we consider the ripple effect as part of the :active state styles? If the answer is yes, then I'm not sure we should separate the ripple effect from the rest of the :active styles. AFAIK native HTML buttons display :active styles when activating them via middle or right mouse clicks.

@NotYoojun I'd appreciate some real world examples (with visuals) to support this.

@DiegoAndai
Copy link
Member

Do we consider the ripple effect as part of the :active state styles?

I'm not sure we should consider it as such. MD2 labels it as "pressed" state: https://m2.material.io/design/interaction/states.html#pressed, different from "active". From the documentation, I understand that the ripple should indicate taps/clicks, should the right-click should be considered as such?

As a benchmark:

I agree, increasing the API surface for this doesn't sound great.

@NotYoojun
Copy link
Author

NotYoojun commented Dec 28, 2024

No need to increase the API. Just add more types to the disableTouchRipple prop. If this is not a good idea, then add a new OPTIONAL property. This property is optional and for advanced uses only, and this has almost no impact to for most users. Why keeping worrying about increasing the API surface?

@DiegoAndai
Copy link
Member

No need to increase the API. Just add more types to the disableTouchRipple prop.

This might be a good idea. What do you think @aarongarciah?

We could add numbers/keywords to the disableRipple prop following the values in https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button#value, so disableRipple =

  • false: Ripple on for all clicks
  • true: Ripple off for all clicks
  • 2: Ripple off for secondary button
  • [1, 2]: Ripple off for auxiliary and secondary button

@aarongarciah
Copy link
Member

@DiegoAndai from an API perspective looks good! But I think we haven't answered this:

I understand that the ripple should indicate taps/clicks, should the right-click should be considered as such?

wdyt?

@NotYoojun
Copy link
Author

I understand that the ripple should indicate taps/clicks, should the right-click should be considered as such?

wdyt?

In my point of view, the right click is not supposed to be considered as a tap by default. But it's really important to allow the developers to configure this behavior for some of the buttons.

We only use the left button click on most buttons, but right click should be considered for some special ones (which should be configured manually).

@aarongarciah
Copy link
Member

I understand that the ripple should indicate taps/clicks, should the right-click should be considered as such?

After thinking about it a bit more, I'm landing on center/right-clicks that are not handled by a button to not trigger the ripple by default. The problem is that we can't detect this. So for now I think expanding the disableRipple prop to accept more values as @DiegoAndai described makes sense to me. Does disableTouchRipple also need to be expanded?

Another option could be to not trigger the ripple effect by default on center/right-click, and provide an opt-in mechanism to trigger it in the next major.

I don't have a strong opinion on wether the opt-in or opt-out mechanism is better. Thoughts @DiegoAndai?


I wish the Material Design spec was more explicit about this. They say:

A pressed state communicates a user-initiated tap or click by a cursor, keyboard, or voice input method.

I couldn't find anything that properly clarifies the behavior.

Let's look at what official Material Design web libraries do:

For completeness, I'd like to know what happens on a modern Android app when doing a long-press on a MD3 official button, since long-press on touch could be compared to right-click. I couldn't find relevant info about this. But since there seems to not be a general consensus, let's do whatever we think is best.

@NotYoojun
Copy link
Author

@aarongarciah I'm always for the opinion to extend the prop as much as possible for high customizations. I think the disableTouchRipple should be extended as well.

As for the default value, 'only raise the ripple when left click' is great.

FYI, even if this library is called 'Material UI', there are plenty of companies that create their own design systems.

@aarongarciah
Copy link
Member

FYI, even if this library is called 'Material UI'

We know! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: button This is the name of the generic UI component, not the React module! component: ButtonBase The React component. new feature New feature or request package: material-ui Specific to @mui/material waiting for 👍 Waiting for upvotes
Projects
None yet
Development

No branches or pull requests

5 participants