Skip to content

fix(rtl): allow :host to use rtl() - #28353

Merged
thetaPC merged 9 commits into
mainfrom
fix-rtl-host
Oct 18, 2023
Merged

fix(rtl): allow :host to use rtl()#28353
thetaPC merged 9 commits into
mainfrom
fix-rtl-host

Conversation

@thetaPC

@thetaPC thetaPC commented Oct 13, 2023

Copy link
Copy Markdown
Contributor

Issue number: N/A


What is the current behavior?

While working on a safe area padding mixin, I realized that rtl() wasn't being applied for :host when using Firefox or Safari. This is happening because the syntax for :dir() is wrong. The placement needs to be updated for Firefox and Safari to register it.

:host {
   @include rtl() { // <- won't work
      // styles
   }
}

// generates
:host-context([dir=rtl]) {
  // styles
}

:host:dir(rtl) { // <- wrong syntax
   // styles
}
:host(.class) {
   @include rtl() { // <- won't work
      // styles
   }
}

// generates
:host-context([dir=rtl]):host(.class) {
  // styles
}

:host-context([dir=rtl]).class {
   // styles
}

:host(.class):dir(rtl) { // <- wrong syntax
   // styles
}

What is the new behavior?

I updated rtl() to use :dir() as the addHostSelector in the add-root-selector function. This generates all the correct selectors for Firefox and Safari. However, :dir() does not have the structure of :host-context() so I had to add a new parameter to add-root-selector to determine whether to use :host-context() or not. I set the default to true since the function originally used :host-context().

An extra win is that the updated function will be ready for when :host-context() can be removed from the codebase.

:host {
   @include rtl() { // <- works
      // styles
   }
}

// generates
:host-context([dir=rtl]) {
  // styles
}

-  :host:dir(rtl) {
+  :host(:dir(rtl)) {
   // styles
}
:host(.class) {
   @include rtl() { // <- works
      // styles
   }
}

// generates
:host-context([dir=rtl]):host(.class) {
  padding-right: 40px;
}

:host-context([dir=rtl]).class {
   // styles
}

-  :host(.class):dir(rtl) {
+  :host(.class:dir(rtl)) {
   // styles
}

Does this introduce a breaking change?

  • Yes
  • No

Other information

N/A

@github-actions github-actions Bot added the package: core @ionic/core package label Oct 13, 2023
@thetaPC
thetaPC marked this pull request as ready for review October 16, 2023 14:56

@brandyscarney brandyscarney left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I tested it against my PR here where I ran into this issue: #28371

It is working as expected in Firefox but I am still not seeing it resolved in Safari.

Below is the change I see in the generated code for the following code snippet between main and this branch:

.item { 
  @include ltr() {
    padding-left: 40px;
  }

  @include rtl() {
    padding-right: 40px;
  }
}
main branch
.item {
  padding-left: 40px;
}
:host-context([dir=rtl]) .item {
  padding-right: 40px;
}
[dir=rtl] .item {
  padding-right: 40px;
}
@supports selector(:dir(rtl)) {
  .item:dir(rtl) {
    padding-right: 40px;
  }
}
      
.item {
  padding-left: 40px;
}
:host-context([dir=rtl]) .item {
  padding-right: 40px;
}
[dir=rtl] .item {
  padding-right: 40px;
}
@supports selector(:dir(rtl)) {
  :dir(rtl) .item {
    padding-right: 40px;
  }
}
      

Is there something missing?

Comment thread core/src/themes/ionic.functions.string.scss Outdated
@brandyscarney

brandyscarney commented Oct 18, 2023

Copy link
Copy Markdown
Member

Maria and I looked into the issue in my last comment. It turns out that I was running into this bug while adding ?rtl=true to the browser url of the test I was previewing. If I added dir="rtl" to the html I did not run into this. This bug was fixed in v17 of Safari but I am using v16.6 which is why I was seeing it and Maria was not. She is going to add a comment in the code in case anyone else runs into it while testing.

…into fix-rtl-host

it merges an updated upstream into a topic branch. # # Lines starting with '#' will be
ignored, and an empty message aborts # the commit.
@thetaPC
thetaPC requested a review from brandyscarney October 18, 2023 18:21
@thetaPC
thetaPC added this pull request to the merge queue Oct 18, 2023
Merged via the queue into main with commit 6b7d288 Oct 18, 2023
@thetaPC
thetaPC deleted the fix-rtl-host branch October 18, 2023 21:35
sean-perkins pushed a commit that referenced this pull request Oct 27, 2023
Issue number: N/A

---------

<!-- 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. -->

While working on a safe area padding mixin, I realized that `rtl()`
wasn't being applied for `:host` when using Firefox or Safari. This is
happening because the syntax for `:dir()` is wrong. The placement needs
to be updated for Firefox and Safari to register it.

```scss
:host {
   @include rtl() { // <- won't work
      // styles
   }
}

// generates
:host-context([dir=rtl]) {
  // styles
}

:host:dir(rtl) { // <- wrong syntax
   // styles
}
```

```scss
:host(.class) {
   @include rtl() { // <- won't work
      // styles
   }
}

// generates
:host-context([dir=rtl]):host(.class) {
  // styles
}

:host-context([dir=rtl]).class {
   // styles
}

:host(.class):dir(rtl) { // <- wrong syntax
   // styles
}
```

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

I updated `rtl()` to use `:dir()` as the `addHostSelector` in the
`add-root-selector` function. This generates all the correct selectors
for Firefox and Safari. However, `:dir()` does not have the structure of
`:host-context()` so I had to add a new parameter to `add-root-selector`
to determine whether to use `:host-context()` or not. I set the default
to `true` since the function originally used `:host-context()`.

An extra win is that the updated function will be ready for when
`:host-context()` can be removed from the codebase.


```diff
:host {
   @include rtl() { // <- works
      // styles
   }
}

// generates
:host-context([dir=rtl]) {
  // styles
}

-  :host:dir(rtl) {
+  :host(:dir(rtl)) {
   // styles
}
```

```diff
:host(.class) {
   @include rtl() { // <- works
      // styles
   }
}

// generates
:host-context([dir=rtl]):host(.class) {
  padding-right: 40px;
}

:host-context([dir=rtl]).class {
   // styles
}

-  :host(.class):dir(rtl) {
+  :host(.class:dir(rtl)) {
   // styles
}
```

## 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. -->

N/A

---------

Co-authored-by: ionitron <hi@ionicframework.com>
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

Development

Successfully merging this pull request may close these issues.

3 participants