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(item-sliding): options display on rtl #27203

Merged
merged 27 commits into from
May 9, 2023
Merged

fix(item-sliding): options display on rtl #27203

merged 27 commits into from
May 9, 2023

Conversation

thetaPC
Copy link
Contributor

@thetaPC thetaPC commented Apr 13, 2023

Issue URL: resolves #26103, resolves #25285


What is the current behavior?

Options in item-sliding will not display when using RTL with Firefox and Safari.

What is the new behavior?

Issue was coming from :host-context. Firefox would keep removing the entire compiled style when using this unsupported style. This would led to the RTL styles to not being applied to the component.

  • Split the CSS group from add-root-selector()
  • Added comments to make it easier to navigate through add-root-selector()
  • Added :dir()

Does this introduce a breaking change?

  • Yes
  • No

Other information

Co-authored-by: brandyscarney brandyscarney@users.noreply.github.com

  • Updating add-root-selector() would also fix another issue unintentionally

@stackblitz
Copy link

stackblitz bot commented Apr 13, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added the package: core @ionic/core package label Apr 13, 2023
@thetaPC thetaPC changed the title fix(item-sliding): apply rtl styles regardless of browser fix(item-sliding): removal of host-context Apr 18, 2023
@thetaPC thetaPC marked this pull request as ready for review April 18, 2023 16:46
@thetaPC thetaPC requested a review from a team as a code owner April 18, 2023 16:46
@liamdebeasi liamdebeasi requested review from liamdebeasi, brandyscarney and a team and removed request for a team April 19, 2023 18:19
core/src/components/popover/popover.tsx Outdated Show resolved Hide resolved
@@ -286,13 +286,13 @@
overflow: hidden;
}

.alert-button:first-child {
:host .alert-button:first-child {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without host-context then it leads to compiling the wrong css. Notice the lack of space between [dir=rtl] and .sc-ion-alert-ios

[dir=rtl].sc-ion-alert-ios .alert-button.sc-ion-alert-ios:last-child {
 border-right:0.55px solid rgba(var(--ion-text-color-rgb, 0, 0, 0), 0.2)
}

By adding :host, the compiled css has the space:

[dir=rtl] .sc-ion-alert-ios-h .alert-button.sc-ion-alert-ios:last-child {
 border-right:0.55px solid rgba(var(--ion-text-color-rgb, 0, 0, 0), 0.2)
}

This only happens when the component is scoped.

If the component is not scoped or a shadow then it would have worked without :host:

.alert-button:first-child {
  @include rtl() {
    ...
  }
}

core/src/components/popover/popover.ios.scss Outdated Show resolved Hide resolved
} @else {
// Add back any selectors that followed the host after transforming the
// first selector:
// :host(.fixed) ::slotted(ion-icon) -> [dir=rtl] :host(.fixed) ::slotted(ion-icon)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the output comment here. [dir=rtl] :host won't match anything: https://codepen.io/liamdebeasi/pen/ExdgJzg

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 outcome originated from input outline.

Without dir in the front then it wouldn't compile the correct styling which is the following:

[dir=rtl] :host(.input-fill-outline) .input-outline-start {
...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

So this only applies to non-shadow DOM components, correct?

Copy link
Contributor Author

@thetaPC thetaPC Apr 20, 2023

Choose a reason for hiding this comment

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

That is correct. The new changes would cause add-root-selector to not work with scoped or shadow DOM components. The main reason that host-context was removed is because when Firefox sees it then it completely removes it along with the attached list.

:host(.popover-side-end) .popover-arrow, :host-contenxt(...)... {
...
}

Firefox wouldn't even apply the example above. It sees the host-context and ignores the entire style (this is the cause of the issue on item-sliding).

Another approach to solving the issue is to determine if the browser is Chrome then add host-context to the list. Thoughts?

@thetaPC thetaPC changed the title fix(item-sliding): removal of host-context fix(item-sliding): split rtl css group Apr 26, 2023
@brandyscarney
Copy link
Member

So I used the following input SCSS against the functions on both main and FW-2608:

// shadow element
:host {
  @include rtl() {
    color: blue;
  }
}

// shadow element targeting class
:host(.fixed) {
  @include rtl() {
    color: red;
  }
}

// shadow element targeting class and child icon
:host(.icon-hide) ::slotted(ion-icon) {
  @include rtl() {
    color: blue;
  }
}

// shadow element targeting class and child icon
:host(.icon-left) ::slotted(ion-icon) {
  @include rtl() {
    color: blue;
  }
}

// shadow element targeting child class
.shadow {
  @include rtl() {
    color: green;
  }
}

// non-shadow element
.not-shadow {
  @include rtl() {
    color: pink;
  }
}

:host .element-selector {
  @include position(0, 0, null, null);
}

On main the resulting selectors are separated by commas, so I split those out to their own rules and then compared that output with the output from this PR in order to make the diff easier to compare:

Screenshot 2023-04-28 at 1 41 50 PM

Screenshot 2023-04-28 at 1 41 56 PM

The diff looks good as all of the selectors that shouldn't have changed have not (besides splitting them up instead of having them comma separated which is intentional). The new selectors seem to have the desired changes.

I checked the item-sliding tests in Safari and Firefox and they seem to be working correctly in RTL.

I think once the screenshots are passing this should be good to go.

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

I think change is looking really good so far!

One thing I am noticing is the bundle size can increase in some places because we are splitting up selectors:
image

Is is possible to optimize the code a bit so that we can use multiple selectors for a single code block?

So in the screenshot above the preferred output would be:

[dir=rtl] .range-knob-handle,
:host-context([dir=rtl]) .range-knob-handle,
.range-knob-handle:dir(rtl) {
 ...
}

I realize this task is taking longer than we originally pointed, so if you'd like to come back to it next sprint when we can re-point it that's fine too.

@thetaPC
Copy link
Contributor Author

thetaPC commented May 3, 2023

I think change is looking really good so far!

One thing I am noticing is the bundle size can increase in some places because we are splitting up selectors: image

Is is possible to optimize the code a bit so that we can use multiple selectors for a single code block?

So in the screenshot above the preferred output would be:

[dir=rtl] .range-knob-handle,
:host-context([dir=rtl]) .range-knob-handle,
.range-knob-handle:dir(rtl) {
 ...
}

I realize this task is taking longer than we originally pointed, so if you'd like to come back to it next sprint when we can re-point it that's fine too.

We can't group them back in that sense because the entire styling would be ignored on Firefox and Safari as soon as they detect :hosts-context. I did manage to group them up this way:

:host-context([dir=rtl]) .range-knob-handle,
:host-context([dir=rtl]) .... {
 ...
}

[dir=rtl] .range-knob-handle,
[dir=rtl] ... {
 ...
}

All host-context will be grouped. Any vanilla selectors will be grouped. As for :dir(), those have also been grouped. But will be based on @supports selector.

@thetaPC thetaPC requested a review from liamdebeasi May 3, 2023 19:05
@liamdebeasi liamdebeasi changed the title fix(item-sliding): split rtl css group fix(item-sliding): options display on rtl May 9, 2023
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

The screenshot test threshold was reduced from 0.2 to 0.1, so it got more sensitive. As a result, I updated your PR to pull in the latest main and re-ran the update screenshot job. We were able to capture the fix that you noted with popover arrows in RTL in 5786f92

Good to merge once CI passes. Great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
4 participants