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

Hotfix: tiny CSS update to List component -- workaround to wrapping related issue in Safari #1805

Closed
wants to merge 1 commit into from

Conversation

sghoweri
Copy link
Contributor

@sghoweri sghoweri commented Apr 1, 2020

Jira

Summary

Adds a tiny 1px addition to the two calc(width: 100% + X) rules in the List component as a workaround to address quirky wrapping behavior.

Safari - Before
safari--before

Safari - After
safari--after

Details

Technically adding 0.4px seems to work (making me suspect that this could be related to a Webkit sub-pixel rounding issue / quirk) -- in either case, this change should basically be unnoticable, other than addressing the main layout wrapping issue in Safari.

How to test

  • Review the List component demos (specifically the inline / flex demos) to confirm there aren't any unintentional side-effects from this change
  • Review the updated Completed Training page (especially in Safari) to confirm the wrapping issue is addressed when compared to the Completed Training Page from the v2.20.2 release.

@sghoweri sghoweri changed the base branch from master to release/2.x April 1, 2020 12:41
@sghoweri sghoweri added the patch label Apr 1, 2020
@sghoweri sghoweri added this to the v2.20.3 milestone Apr 1, 2020
Copy link
Collaborator

@mikemai2awesome mikemai2awesome left a comment

Choose a reason for hiding this comment

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

@danielamorse @sghoweri

I'm ok with rolling with this for now. If you look at the history, we keep adding and removing this 1px. When this 1px is added, it will create horizontal scroll in other browsers IF the list is edge to edge (either on the page container or some kind of box), which is not that often. Let's add a comment to the code to explain that.

Copy link
Collaborator

@danielamorse danielamorse left a comment

Choose a reason for hiding this comment

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

I confirmed that this fix causes List to overflow the container by some fraction of a pixel, even in Chrome. We don't see it because there is overflow-x: hidden on the body. I forget the exact scenario where this popped up before, but it becomes a problem anytime you want to use List inside a container with overflow: auto.

Out of curiosity, I removed the width rule altogether and tested Chrome, Safari, FF, and Edge. The Completed Training template and the list demos appear to work as expected without it. @mikemai2awesome, do you remember the specific FF issue the width rule was added for? Perhaps it was for an older version of FF? 🤞

Here's my branch, tests are passing.

Copy link
Collaborator

@mikemai2awesome mikemai2awesome left a comment

Choose a reason for hiding this comment

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

Out of curiosity, I removed the width rule altogether and tested Chrome, Safari, FF, and Edge. The Completed Training template and the list demos appear to work as expected without it. @mikemai2awesome, do you remember the specific FF issue the width rule was added for? Perhaps it was for an older version of FF?

@danielamorse Do the following and test again. If all good then let's roll with that.

// Spacing Prop
@each $spacing-value in $bolt-spacing-values {
  $spacing-value-name: nth($spacing-value, 1);

  .c-bolt-list--spacing-#{$spacing-value-name}:not(.c-bolt-list--inset) {
    margin-bottom: bolt-v-spacing(#{$spacing-value-name}) * -1;
    margin-left: bolt-spacing(#{$spacing-value-name}) * -1;
  }

  @-moz-document url-prefix() {
    .c-bolt-list--spacing-#{$spacing-value-name}.c-bolt-list--inset {
      // The inline here is talking about the items inside, the List component itself is still a block level element that would fill up the space of any container.
      &.c-bolt-list--display-inline,
      &.c-bolt-list--display-flex {
        width: 100%; // Width must be defined in order for the list to dislay correctly in Firefox.
      }

      @each $breakpoint in $bolt-breakpoints {
        $breakpoint-name: nth($breakpoint, 1);

        &.c-bolt-list--display-inline\@#{$breakpoint-name} {
          @include bolt-mq($breakpoint-name) {
            width: 100%; // Width must be defined in order for the list to dislay correctly in Firefox.
          }
        }
      }
    }
    .c-bolt-list--spacing-#{$spacing-value-name}:not(.c-bolt-list--inset) {
      // The inline here is talking about the items inside, the List component itself is still a block level element that would fill up the space of any container.
      &.c-bolt-list--display-inline,
      &.c-bolt-list--display-flex {
        width: calc(
          100% + #{bolt-spacing($spacing-value-name)}
        ); // Width must be defined in order for the list to dislay correctly in Firefox.
      }

      @each $breakpoint in $bolt-breakpoints {
        $breakpoint-name: nth($breakpoint, 1);

        &.c-bolt-list--display-inline\@#{$breakpoint-name} {
          @include bolt-mq($breakpoint-name) {
            width: calc(
              100% + #{bolt-spacing($spacing-value-name)}
            ); // Width must be defined in order for the list to dislay correctly in Firefox.
          }
        }
      }
    }
  }

  .c-bolt-list--spacing-#{$spacing-value-name}.c-bolt-list--inset {
    @include bolt-margin-bottom(0);
    @include bolt-margin-left(0);
  }
}

@danielamorse
Copy link
Collaborator

danielamorse commented Apr 2, 2020

@mikemai2awesome @sghoweri Mike's suggestion above appears to be working. I created PR #1806 with his change since I wanted to test it separately and because I don't think we're planning to release this as a hotfix.

If you guys approve #1806 we can close this one out.

@sghoweri
Copy link
Contributor Author

sghoweri commented Apr 8, 2020

Closing out in lieu of #1806 - thanks @danielamorse

@sghoweri sghoweri closed this Apr 8, 2020
@sghoweri sghoweri deleted the hotfix/safari-list-wrapping-fix branch April 8, 2020 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants