fix(icon): respond to changes of document dir

[mapsandapps]

Resolves #1196

Use CSS to ensure that the icon's direction can change when needed*, if state changes after initial page load

* if the icon has the flip-rtl prop set or if it's an arrow or chevron or if the element has a dir attribute

[liamdebeasi]

Ionic Framework has some mixins that let us accomplish RTL styles without JavaScript: https://github.com/ionic-team/ionic-framework/blob/main/core/src/themes/ionic.mixins.scss

I know Maria had done some work to improve these mixins for broader browser compatibility, so I wonder if we could use that here. Her PR in ionic-team/ionic-framework#27203 uses :host-context for Chrome support and :dir for Firefox/Safari support.

This would enable us to remove the MutationObserver and do something like this:

:host(.flip-rtl) .icon-inner {
  @include rtl() {
    transform: scaleX(-1);
  }
}

:host-context is supported by all versions of Chrome/Edge that Ionic 7 supports: https://caniuse.com/?search=host-context
:dir is supported by all versions of Firefox that we support: https://caniuse.com/?search=%3Adir

Support for :dir in Safari is new, but this was determined to be an acceptable tradeoff in the Framework PR I mentioned above since iOS has a pretty solid upgrade rate.

[mapsandapps]

oh great! i was hoping for a css-only solution, but i didn't come across the :host-context/:dir combo to make compatibility ok.

i'm not quite sure how or if we can go about using the mixin. we're not even using sass in this project 😅 for now, i updated the code to just use the css directly.

[liamdebeasi]

Did we create a dev build to verify that the original Ionic Framework issue is fixed?

[liamdebeasi]

i'm not quite sure how or if we can go about using the mixin. we're not even using sass in this project 😅 for now, i updated the code to just use the css directly.

Yeah, using host-context/:dir directly is fine for now since we are only using it once. If we end up using it >=3 times we should probably consider adding mixin/scss support.

[liamdebeasi]

Suggested change

	this.shouldAutoFlip =
	(this.iconName?.includes('arrow') || this.iconName?.includes('chevron')) && this.flipRtl !== false;
	this.shouldBeFlippable = this.flipRtl || this.shouldAutoFlip || this.el.hasAttribute('dir');
	const shouldAutoFlip = (iconName && (iconName.indexOf('arrow') > -1 || iconName.indexOf('chevron') > -1) && flipRtl !== false);
	const shouldBeFlippable = flipRtl || shouldAutoFlip || this.el.hasAttribute('dir');

A few things:

1. IMO if we are always computing these values on render then there does not seem to be much value in caching them as global variables within the class. A local const should be fine here.
2. Using optional chaining has caused issues in the past with boolean/numeric values, so let's keep the old syntax just to be safe. (See fix(ripple-effect): ripple displays on click or touch ionic-framework#25102 for an example)
3. Do we need this.el.hasAttribute('dir')? I believe :dir() should account for this. Or is this for browsers that support neither :host-context or :dir?

[mapsandapps]

Oh yeah, good call on this.el.hasAttribute('dir'). I think that was left over from when there were observers.

[mapsandapps]

Did we create a dev build to verify that the original Ionic Framework issue is fixed?

Took me a while to figure out how to do this 😅 I guess you have to create an Ionicons dev build and then make a branch of framework that uses that Ionicons build and then make a dev build of framework?

Anyway, it does indeed work with the reproduction from the original ticket!

    "@ionic/angular": "7.0.8-dev.11684448544.19bd9148",
    "ionicons": "7.1.1-dev.11684440744.15dddc23",

[liamdebeasi]

Great job!