fix(item-divider): add margin to end slots

[thetaPC]

Pull request checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
• Build (npm run build) was run locally and any changes were pushed
• Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

• Bugfix
• Feature
• Code style update (formatting, renaming)
• Refactoring (no functional changes, no api changes)
• Build related changes
• Documentation content changes
• Other (please describe):

What is the current behavior?

End slots are lacking margin of 2px when in RTL.

Issue URL: Part of #17012

What is the new behavior?

• item-md-end-slot-margin-end = 2px

End slots will have a margin-end of 2px regardless of direction.

Screenshots

Does this introduce a breaking change?

• Yes
• No

Other information

N/A

[bolt-new-by-stackblitz]

Run & review this pull request in StackBlitz Codeflow.

[liamdebeasi]

I don't think this is the correct way to solve this bug. According to the report, this issue only impacts content slotted using ion-item-divider. However, the proposed fix impacts anything in ion-item even if ion-item-divider is not being used.

If you look at the computed CSS in the original issue reproduction, you should see the following output in Chrome:

The margin, denoted by margin-inline-end inside of the green highlight, adds the correct margin of 2px on the left side when the content is in RTL. (margin-inline-end docs: https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-end). These are logical properties, so they use "start" and "end" terminology instead of "left" and "right".

If you look at the red highlight you should see the margin-left is being unset. So the problem here is the margin is being set correctly initially, but something else is overriding it.

---

The source for the margin-left: unset is coming from ion-item-divider. You can find this by clicking the "<style>" across from the margin-left: unset. This appears to be the source of the issue:

ionic-framework/core/src/components/item-divider/item-divider.md.scss

Line 29 in f686440

@include margin-horizontal($item-md-end-slot-margin-start, $item-md-end-slot-margin-end);

The margin-horizontal mixin is a custom mixin we wrote to make it easy to use logical properties for RTL support. The source for that is here:

ionic-framework/core/src/themes/ionic.mixins.scss

Lines 304 to 306 in f686440

	@mixin margin-horizontal($start, $end: $start) {
	@include property-horizontal(margin, $start, $end);
	}

It also relies on a more generic property-horizontal mixin that we use for logical margins, padding, and borders:

ionic-framework/core/src/themes/ionic.mixins.scss

Lines 239 to 266 in f686440

	@mixin property-horizontal($prop, $start, $end: $start) {
	@if $start == 0 and $end == 0 {
	#{$prop}-left: $start;
	#{$prop}-right: $end;
	} @else {
	#{$prop}-left: $start;
	#{$prop}-right: $end;
	@at-root {
	@supports ((margin-inline-start: 0) or (-webkit-margin-start: 0)) {
	& {
	@if $start != null {
	#{$prop}-left: unset;
	}
	@if $end != null {
	#{$prop}-right: unset;
	}
	-webkit-#{$prop}-start: $start;
	#{$prop}-inline-start: $start;
	-webkit-#{$prop}-end: $end;
	#{$prop}-inline-end: $end;
	}
	}
	}
	}
	}

In particular, it looks like it sets margin-left/margin-right for browsers that do not support these logical properties and then uses unset inside of the @supports block for browsers that do support logical properties.

---

The investigation here ended up being a little more complex than I originally expected. I also have some thoughts on how we can fix this, so let me know when you have some time and we can sync over Zoom.

[thetaPC]

I don't think this is the correct way to solve this bug. According to the report, this issue only impacts content slotted using ion-item-divider. However, the proposed fix impacts anything in ion-item even if ion-item-divider is not being used.

If you look at the computed CSS in the original issue reproduction, you should see the following output in Chrome:

The margin, denoted by margin-inline-end inside of the green highlight, adds the correct margin of 2px on the left side when the content is in RTL. (margin-inline-end docs: https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-end). These are logical properties, so they use "start" and "end" terminology instead of "left" and "right".

If you look at the red highlight you should see the margin-left is being unset. So the problem here is the margin is being set correctly initially, but something else is overriding it.

The source for the margin-left: unset is coming from ion-item-divider. You can find this by clicking the "<style>" across from the margin-left: unset. This appears to be the source of the issue:

ionic-framework/core/src/components/item-divider/item-divider.md.scss

Line 29 in f686440

@include margin-horizontal($item-md-end-slot-margin-start, $item-md-end-slot-margin-end);

The margin-horizontal mixin is a custom mixin we wrote to make it easy to use logical properties for RTL support. The source for that is here:

ionic-framework/core/src/themes/ionic.mixins.scss

Lines 304 to 306 in f686440

	@mixin margin-horizontal($start, $end: $start) {
	@include property-horizontal(margin, $start, $end);
	}

It also relies on a more generic property-horizontal mixin that we use for logical margins, padding, and borders:

ionic-framework/core/src/themes/ionic.mixins.scss

Lines 239 to 266 in f686440

	@mixin property-horizontal($prop, $start, $end: $start) {
	@if $start == 0 and $end == 0 {
	#{$prop}-left: $start;
	#{$prop}-right: $end;
	} @else {
	#{$prop}-left: $start;
	#{$prop}-right: $end;
	@at-root {
	@supports ((margin-inline-start: 0) or (-webkit-margin-start: 0)) {
	& {
	@if $start != null {
	#{$prop}-left: unset;
	}
	@if $end != null {
	#{$prop}-right: unset;
	}
	-webkit-#{$prop}-start: $start;
	#{$prop}-inline-start: $start;
	-webkit-#{$prop}-end: $end;
	#{$prop}-inline-end: $end;
	}
	}
	}
	}
	}

In particular, it looks like it sets margin-left/margin-right for browsers that do not support these logical properties and then uses unset inside of the @supports block for browsers that do support logical properties.

The investigation here ended up being a little more complex than I originally expected. I also have some thoughts on how we can fix this, so let me know when you have some time and we can sync over Zoom.

Let me know if we should still sync up.

[liamdebeasi]

We're getting much closer I think!

What I was thinking instead is to simplify the property-horizontal mixin:

@mixin property-horizontal($prop, $start, $end: $start) {
  @if $start == 0 and $end == 0 {
    #{$prop}-left: $start;
    #{$prop}-right: $end;
  } @else {
    -webkit-#{$prop}-start: $start;
    #{$prop}-inline-start: $start;
    -webkit-#{$prop}-end: $end;
    #{$prop}-inline-end: $end;
  }
}

The root problem here is we are unsetting the margin inside of this mixin when we should not be. While the code you have works, it does not address the root issue.

We use property-horizontal for margin-inline, padding-inline, and border-inline. Here is what the browser support looks like:

margin-inline-start/end: https://caniuse.com/?search=margin-inline-end

padding-inline-start/end: https://caniuse.com/?search=padding-inline-end

border-inline-start/end: https://caniuse.com/?search=border-inline-end

The only thing we need to be concerned about is border-inline-* which has support starting in Chrome 69. (Ionic v6 supports Chrome 60+). If we moved forward with the mixin change, we could target this fix for v7 instead and get the following benefits:

1. Resolve the underlying issue and prevent this from popping up in other places
2. Reduce the amount of code this mixin generates, which leads to a smaller bundle size.

[thetaPC]

We're getting much closer I think!

What I was thinking instead is to simplify the property-horizontal mixin:

@mixin property-horizontal($prop, $start, $end: $start) {
  @if $start == 0 and $end == 0 {
    #{$prop}-left: $start;
    #{$prop}-right: $end;
  } @else {
    -webkit-#{$prop}-start: $start;
    #{$prop}-inline-start: $start;
    -webkit-#{$prop}-end: $end;
    #{$prop}-inline-end: $end;
  }
}

The root problem here is we are unsetting the margin inside of this mixin when we should not be. While the code you have works, it does not address the root issue.

We use property-horizontal for margin-inline, padding-inline, and border-inline. Here is what the browser support looks like:

margin-inline-start/end: https://caniuse.com/?search=margin-inline-end

padding-inline-start/end: https://caniuse.com/?search=padding-inline-end

border-inline-start/end: https://caniuse.com/?search=border-inline-end

The only thing we need to be concerned about is border-inline-* which has support starting in Chrome 69. (Ionic v6 supports Chrome 60+). If we moved forward with the mixin change, we could target this fix for v7 instead and get the following benefits:

1. Resolve the underlying issue and prevent this from popping up in other places

2. Reduce the amount of code this mixin generates, which leads to a smaller bundle size.

This makes sense. Plus it makes it easier to trace back styles. I'll go ahead and swap to this solution.

[liamdebeasi]

The changes look good! A couple final things:

1. We need to target the feature-7.0 branch instead of main now because these changes are dependent on the updated browser support Ionic 7 has.
2. We either need to update existing screenshots or we need to add a test for this.

[brandyscarney]

Just noting here that tests for this might conflict with this PR updating the item-divider tests. We might want to get one done before the other or keep the tests in separate directories.

[thetaPC]

Just noting here that tests for this might conflict with this PR updating the item-divider tests. We might want to get one done before the other or keep the tests in separate directories.

@brandyscarney This PR has been moved to #27042.

I'll keep an eye out on both PRs. If their PR is merged in first, I'll accommodate. Otherwise, I'll let them know that mine was merged first.

[thetaPC]

Closing since this #27042 will be used to address the issue instead. The reason for the new PR is due to easily swapping to a different base.