-
Notifications
You must be signed in to change notification settings - Fork 13.5k
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
Conversation
Run & review this pull request in StackBlitz Codeflow. |
@@ -286,13 +286,13 @@ | |||
overflow: hidden; | |||
} | |||
|
|||
.alert-button:first-child { | |||
:host .alert-button:first-child { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this added?
There was a problem hiding this comment.
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() {
...
}
}
} @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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 {
...
}
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
This reverts commit 006adaa.
So I used the following input SCSS against the functions on both // 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 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 I think once the screenshots are passing this should be good to go. |
There was a problem hiding this 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:
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.
There was a problem hiding this 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!
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.add-root-selector()
add-root-selector()
:dir()
Does this introduce a breaking change?
Other information
Co-authored-by: brandyscarney brandyscarney@users.noreply.github.com
add-root-selector()
would also fix another issue unintentionally