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

Arrow Icons are not flipped in RTL #22726

Closed
2 tasks done
AhmadMayo opened this issue Sep 25, 2020 · 19 comments
Closed
2 tasks done

Arrow Icons are not flipped in RTL #22726

AhmadMayo opened this issue Sep 25, 2020 · 19 comments
Labels
component: Icon The React component. component: SvgIcon The React component. out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future)

Comments

@AhmadMayo
Copy link

AhmadMayo commented Sep 25, 2020

I followed the steps in the website to create an Arabic web application. Everything works great until I use any of the (directional) arrow icons.

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The back arrow still points left, and the forward arrows still points right.

Expected Behavior 🤔

They should be flipped, sometimes: https://material.io/design/usability/bidirectionality.html.

Screenshot 2023-02-06 at 20 35 37

Steps to Reproduce 🕹

reproduction

Steps:

  1. Follow the steps in the website to support RTL
  2. use any directional arrow icon

Your Environment 🌎

Tech Version
Material-UI v4.11.0
React v16.9.49
Browser Firefox 80.0.1 (64-bit)
TypeScript v3.9.7
Material-UI Icons v4.9.1
@AhmadMayo AhmadMayo added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 25, 2020
@mbrookes
Copy link
Member

The React SVG icon component names describe the icons as produced by Google, and assume forward is to the right, and back is to the left.

You can use import aliases to rename the imports.

@mbrookes mbrookes added external dependency Blocked by external dependency, we can’t do anything about it package: icons Specific to @mui/icons support: question Community support but can be turned into an improvement out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer support: question Community support but can be turned into an improvement labels Sep 25, 2020
@AhmadMayo
Copy link
Author

AhmadMayo commented Sep 25, 2020

@mbrookes but the arrows will not be flipped automatically if the direction is changed dynamically. all the styles of the page will be flipped except for the arrow, which will need to be handled in userland.

And according to the specs the arrows should be flipped

Edit: Sorry that the link is not to the specific paragraph. It's titled "When to mirror" and the examples used are the back and forward arrows

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 25, 2020

@AhmedSayed77 See an example on how to do such:

{theme.direction === 'rtl' ? <KeyboardArrowRight /> : <KeyboardArrowLeft />}

RTL isn't very frequently used by the community. If we can trust the npm downloads, it's used by less than 2% of the developers we reach. This market share influences the tradeoffs we are willing to make.

@mbrookes
Copy link
Member

@mbrookes but the arrows will not be flipped automatically if the direction is changed dynamically.

Apologies, it wasn't obvious from your issue description that you needed to support both. In addition to @oliviertassinari's suggestion, you could perhaps alternatively try applying transform: flipX(-1) in conjunction with the :dir(rtl) pseudo selector.

@AhmadMayo
Copy link
Author

@oliviertassinari @mbrookes Thanks for your time

@ryancogswell
Copy link
Contributor

Here's a related StackOverflow question: https://stackoverflow.com/questions/64307165/material-ui-icons-wont-flip-when-i-change-to-rtl/64307950#64307950.

In my answer I suggested that this could be handled in the theme with:

overrides: {
  MuiSvgIcon: {
    root: {
      "body[dir=rtl] &": {
        transform: "scaleX(-1)"
      }
    }
  }
}

I have a little concern about this potentially conflicting with usages of transform within Material-UI, but the two main cases I'm aware of (AccordionSummary expanded styles and SelectInput iconOpen styles) seemed to still work fine. We could potentially add something along these lines to the RTL documentation.

@oliviertassinari
Copy link
Member

@ryancogswell What's the impact on the components that already handle RTL for the icons, like the TablePagination?

@ryancogswell
Copy link
Contributor

ryancogswell commented Oct 12, 2020

What's the impact on the components that already handle RTL for the icons, like the TablePagination?

@oliviertassinari The cases of this I found just now are in TablePaginationActions and PaginationItem. There are some additional cases in examples in the docs, but these two were the only cases I found in the component code (though I'm not confident that my search would have found all cases). It would definitely have an undesirable effect in these cases since it would flip an already flipped icon.

One possibility would be to add transform: theme.direction === 'rtl' ? 'scaleX(-1)' : undefined, directly into SvgIcon and remove all the code for swapping icons. This would be a breaking change, since it would mean that any similar icon swapping in user code would need to be removed, but I think people would welcome this being handled automatically. The application I work on deals with about 40 languages including Arabic and Hebrew, so I have some experience with this in our application.

@ryancogswell ryancogswell reopened this Oct 12, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 12, 2020

Is this a behavior we want by default? on YouTube, I have only seen one icon swapped.

Capture d’écran 2020-10-12 à 16 58 42

Capture d’écran 2020-10-12 à 16 59 36

Capture d’écran 2020-10-12 à 16 59 00

Capture d’écran 2020-10-12 à 16 59 30

@ryancogswell
Copy link
Contributor

Is this a behavior we want by default?

Looking at some of these examples, probably not. For most icons, flipping is neither necessary nor harmful, but the "Help" icon would be one that shouldn't flip and same with "Paid memberships" and "YouTube Studio". Probably best for users to deal with on a case-by-case basis since it depends on the nature of the icon whether flipping is appropriate.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 12, 2020

Maybe we could add a prop for that? https://fontawesome.com/how-to-use/on-the-web/styling/power-transforms

@ryancogswell
Copy link
Contributor

Maybe we could add a prop for that?

Yes, I think that would be nice. Perhaps a flipForRtl boolean prop?

@ryancogswell ryancogswell reopened this Oct 12, 2020
@oliviertassinari oliviertassinari added component: Icon The React component. component: SvgIcon The React component. ready to take Help wanted. Guidance available. There is a high chance the change will be accepted and removed external dependency Blocked by external dependency, we can’t do anything about it package: icons Specific to @mui/icons out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) labels Oct 12, 2020
@AhmadMayo
Copy link
Author

It should be the default behaviour, but only for the directional icons - back and forward, not left, right, up or down

@ryancogswell
Copy link
Contributor

It should be the default behaviour, but only for the directional icons - back and forward, not left, right, up or down

@AhmedSayed77 I don't think it would be practical (from a maintenance standpoint) to change the default behavior icon-by-icon and it would be difficult to document this clearly. I think people choose icons primarily by how they look with less concern about how they are named -- the name just helps people find the icon. Icons with names including "left" and "right" could easily be used for "back" and "forward" purposes and vice versa. It would be very confusing for similar icons to behave differently in this regard.

If we add a flipForRtl prop, it makes it very easy for developers to control this explicitly in their own code. The prop could also be used internally within Material-UI components such as TablePaginationActions and PaginationItem rather than swapping in different icons for rtl.

@AhmadMayo
Copy link
Author

I'm expressing what I think is best from a DX standpoint. Left always means left, but back doesn't always mean left. Thank god we only have two setting: ltr and rtl. So handling it case by case makes sense, and shouldn't be difficult, since there are only 2 settings. How to do it in a maintainable way, is our job as developers, that's what we do for a living. Of course I'm not a maintainer, and I don't have the right to tell you what to do, or how to do it. If you decide that you will leave it as it is, and let the users handle it, I can only thank you for time, and your work, that I acknowledge I'm using for free - and I'm sincerely thankful to every one of you. But I felt that you were caught up discussing the issue from maintenance standpoint, but didn't think of what's intuitive for the library users. Yes, as an arabic speaker, I expect "back" and "front' arrows to be flipped in an arabic website, but I don't expect play buttons, logos, or other icons to be flipped too.

@ryancogswell
Copy link
Contributor

Left always means left, but back doesn't always mean left.

@AhmedSayed77 My point is that from a DX standpoint, developers will use ArrowLeft and ArrowBack for overlapping purposes (largely choosing based on aesthetics, not name) and I don't think it would be intuitive for them to behave differently with regard to whether they automatically flip. For instance, TablePaginationActions uses KeyboardArrowLeft and KeyboardArrowRight for "back" and "forward" purposes. In this case, "left" doesn't mean "left", it means "back".

@AhmadMayo
Copy link
Author

Maybe if they knew that back arrow and front arrow handles the direction, they would've used it, instead of handling it themselves, while using ArrowLeft and ArrowRight? I don't want to be insistent, I really don't think I can add anything to what I've said before. Do what you think is best for you and your users, and after all, handling it in userland is not too hard anyways

@siriwatknp
Copy link
Member

I am closing this because I don't think we want to spend effort on this. It should be done on user land, either using useRtl or using CSS.

<Icon sx={{ '[dir=rtl] &': { transform: 'rotate(90deg)' } }} />

@siriwatknp siriwatknp added out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) and removed ready to take Help wanted. Guidance available. There is a high chance the change will be accepted labels Dec 12, 2024
Copy link

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

Note

@AhmadMayo How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Icon The React component. component: SvgIcon The React component. out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future)
Projects
None yet
Development

No branches or pull requests

5 participants