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

bug: :host selector on element combined with rtl mixins does not create the proper selector #25285

Closed
6 of 7 tasks
crazyserver opened this issue May 13, 2022 · 5 comments · Fixed by #27203
Closed
6 of 7 tasks
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@crazyserver
Copy link

crazyserver commented May 13, 2022

Prerequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x
  • Nightly

Current Behavior

With the following CSS:

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

When using the rtl mixin so, also add-root-selector it translates the selector as:

:host-context([dir=rtl])

So part of it is lost.

Expected Behavior

With the following CSS:

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

When using the rtl mixin so, also add-root-selector it translates the selector as:

:host-context([dir=rtl]) .element-selector

Maintaining the whole selector.

Steps to Reproduce

  1. Create a blank template
  2. Add a div.test-element to the HTML
  3. Add the following code to the related CSS:
:host .test-element {
    @include position(0, 0, null, null);
}
  1. Run the app, inspect code of the created div and check css is not included on the element.
  2. You can also check the resulting selector is not correct on the generated css.

Code Reproduction URL

No response

Ionic Info

Ionic:

Ionic CLI : 6.19.0 (/Users/pau/.nvm/versions/node/v16.14.2/lib/node_modules/@ionic/cli)
Ionic Framework : @ionic/angular 5.9.2
@angular-devkit/build-angular : 0.1000.8
@angular-devkit/schematics : 10.0.8
@angular/cli : 10.0.8
@ionic/angular-toolkit : 2.3.3

Cordova:

Cordova CLI : 11.0.0
Cordova Platforms : android 10.1.1, ios 6.2.0
Cordova Plugins : cordova-plugin-ionic-keyboard 2.2.0, (and 22 other plugins)

Utility:

cordova-res (update available: 0.15.4) : 0.15.3
native-run (update available: 1.5.0) : 1.4.0

System:

ios-sim : 8.0.2
NodeJS : v16.14.2 (/Users/pau/.nvm/versions/node/v16.14.2/bin/node)
npm : 8.5.0
OS : macOS Monterey
Xcode : Xcode 13.3 Build version 13E113

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label May 13, 2022
@crazyserver crazyserver changed the title bug: bug: :host selector on element combined with rtl mixins does not create the proper selector May 13, 2022
@liamdebeasi
Copy link
Contributor

Thanks for the issue. I can reproduce this behavior. We incorrectly assume that if the selector contains :host then we are only targeting :host:

// If the selector contains :host it means it is targeting just the host
// element so we can change it to look for host-context
} @else if str-contains($selector, ":host") {

As a result, using a selector such as :host .test-element does not work. Note that if the selector is simply .test-element then this works as expected.

@liamdebeasi liamdebeasi added package: core @ionic/core package type: bug a confirmed bug report labels May 13, 2022
@ionitron-bot ionitron-bot bot removed the triage label May 13, 2022
@liamdebeasi
Copy link
Contributor

Some clarified steps to reproduce:

  1. Open an existing E2E HTML file in core.
  2. Set the body to the following:
<ion-img src="https://picsum.photos/200/300"></ion-img>
  1. In src/components/img/img.tsx add the following right before the <img> declaration:
<div class="test-element">my test element</div>
  1. In src/components/img/img.scss add the following styles:
@import "../../themes/ionic.globals";

...

:host {
  width: 300px;
  height: 300px;
}

:host .test-element {
  @include position(null, null, null, 50px);

  position: relative;

  background: red;
}
  1. Open the E2E HTML file in the browser. Observe that the image and the test element are adjusted on the left by 50px.
  2. Add ?rtl=true to the URL and press "Enter". Observe that the image is adjusted on the right by 50px, but the test element is not.
  3. Apply the fix in fix(css): rtl mixin preserves selectors after :host #25264. Observe that both the image and the test element are adjusted on the right by 50px.

thetaPC added a commit that referenced this issue May 9, 2023
Issue URL: resolves #26103, resolves #25285

---------

<!-- Please refer to our contributing documentation for any questions on
submitting a pull request, or let us know here if you need any help:
https://ionicframework.com/docs/building/contributing -->

<!-- Some docs updates need to be made in the `ionic-docs` repo, in a
separate PR. See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation
for details. -->

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

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

<!-- Issues are required for both bug fixes and features. -->


## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

Issue was coming from `:host-context`. Firefox would keep [removing the
entire](https://www.w3.org/TR/selectors-3/#grouping) 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
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

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

- Updating `add-root-selector()` would also fix another
[issue](#25285)
unintentionally

---------

Co-authored-by: ionitron <hi@ionicframework.com>
Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
@thetaPC
Copy link
Contributor

thetaPC commented May 9, 2023

Thank you for the issue! The fix has been applied through PR #27203.

@thetaPC
Copy link
Contributor

thetaPC commented May 11, 2023

The fix has been released in v7.0.6.

@ionitron-bot
Copy link

ionitron-bot bot commented Jun 10, 2023

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Jun 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants