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

fix(icon): add :dir fallback #1223

Merged
merged 8 commits into from
May 30, 2023
Merged

fix(icon): add :dir fallback #1223

merged 8 commits into from
May 30, 2023

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented May 26, 2023

There are a couple things we missed with our first pass of #1210:

  1. Browsers that support neither :host-context nor :dir have no fallback to at least set the dir correctly on page load.
  2. @supports selector(:dir(rtl)) evaluates to true in Safari <16.4 even though :dir was supported starting in Safari 16.4. So even if we made a fallback such as above, it would not work on older versions of Safari.

Solutions:

  1. I added a manual isRTL check so at least the icon respects the dir on component load
  2. Targeted WebKit browsers with -webkit-named-image and applied the RTL fallback. WebKit browsers that support :dir(rtl) will override this fallback and get the reactive RTL behavior.

Safari Bug Example: https://codepen.io/liamdebeasi/pen/eYPXPZY

Browsers that do not support :dir(rtl) should render a red square. Browsers that do support :dir(rtl) should render a green square only when .square has dir="rtl" (which it does in this template.

Chrome: Red (Chrome doesn't support :dir yet so this result is fine)
Firefox: Green
Safari 16.4: Green
Safari <16.4: Nothing

Safari <16.4 reports that it supports :dir(rtl) so it never renders the red fallback. However, it never renders the green color either since it does not actually support :dir(rtl).

@liamdebeasi liamdebeasi changed the title Safari fallback fix(icon): add :dir fallback May 26, 2023
@supports selector(:dir(rtl)) {
:host(.flip-rtl:dir(rtl)) .icon-inner {
transform: scaleX(-1);
}
:host(.flip-rtl:dir(ltr)) .icon-inner {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed for WebKit otherwise the fallback will always cause the icon to be flipped if the document loads in RTL

Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be nice to have this comment in the code.

Copy link
Contributor

@mapsandapps mapsandapps left a comment

Choose a reason for hiding this comment

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

I no longer have the right version of Safari to test this with, but it looks good to me. Thanks for figuring this all out!

@supports selector(:dir(rtl)) {
:host(.flip-rtl:dir(rtl)) .icon-inner {
transform: scaleX(-1);
}
:host(.flip-rtl:dir(ltr)) .icon-inner {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be nice to have this comment in the code.

Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Changes look good to me. +1 to adding a comment for why we have scaleX(1) for the ltr direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants