Skip to content

Addressing Issue #3691 - SplitButton: In Safari, the SplitButton has mismatched heights#4797

Merged
oengusmacinog-zz merged 9 commits intomicrosoft:masterfrom
oengusmacinog-zz:safari-button-reset-3691
May 21, 2018
Merged

Addressing Issue #3691 - SplitButton: In Safari, the SplitButton has mismatched heights#4797
oengusmacinog-zz merged 9 commits intomicrosoft:masterfrom
oengusmacinog-zz:safari-button-reset-3691

Conversation

@oengusmacinog-zz
Copy link
Copy Markdown
Collaborator

@oengusmacinog-zz oengusmacinog-zz commented May 7, 2018

Pull request checklist

Description of changes

Added a margin: 0 reset to ms-Button root, as well as the split button. This is a fix for Safari, where the browser default seems to set the margin to 0em which for some reason translates to a margin left and right of 2px throwing off the button layout.

I will not this bug does not show up on the live demo site (https://developer.microsoft.com/en-us/fabric#/components/button)
because the UHF applies its own custom file with resets (looks, like normalize, is part of it) with this file: mwf-west-european-default.min.css.

I will follow up on general resets for Fabric as well, but for now, this single reset will solve the problem with the button.

@micahgodbolt
Copy link
Copy Markdown
Member

i'd like to understand this one a bit better. it seems really odd behavior. The problem with adding zero margins is that other's might have set margins that will now see them overridden. Is this a safari bug that will be resolved later?

@oengusmacinog-zz
Copy link
Copy Markdown
Collaborator Author

@micahgodbolt My original intention was to follow up on a set of browser reset styles, as the Fabric website is already utilizing mwf-west-european-default.min.css due to the UHF, it would be consistent for Fabric to use this on its own - since all our live examples do. However, that seemed like it might take a while and the split button has looked broken in Safari for months now (if the app using fabric doesn't load its own browser reset), so I was submitting this PR as a quick fix for the specific error.

@micahgodbolt
Copy link
Copy Markdown
Member

I looked into this bug a bit more and wow, thats annoying! This happened with overflow as well (different default in IE11) and the solution was to add it to the Fabric component. My problem with this solution is that it has a chance of breaking any margins applied with a single classname, but if we add it to the button tag itself, any class won't be affected.

image

Copy link
Copy Markdown
Member

@micahgodbolt micahgodbolt left a comment

Choose a reason for hiding this comment

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

move style to Fabric component to keep specificity low

@oengusmacinog-zz
Copy link
Copy Markdown
Collaborator Author

I like it! :)

@micahgodbolt
Copy link
Copy Markdown
Member

well that sure cleans up the PR a bit :D

font-size: 14px;
font-weight: 400;
height: 100%;
margin-bottom: 0px;
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.

why are these still here?

@JasonGore
Copy link
Copy Markdown
Member

It doesn't look like the experiments change file is needed anymore?

@oengusmacinog-zz
Copy link
Copy Markdown
Collaborator Author

oengusmacinog-zz commented May 21, 2018

Currently running rush rebuild to clean up that experiments snapshot as well, but yessss way cleaner :) (will nix the experiments change file too)

@oengusmacinog-zz oengusmacinog-zz merged commit 99145f5 into microsoft:master May 21, 2018
@oengusmacinog-zz oengusmacinog-zz deleted the safari-button-reset-3691 branch May 21, 2018 18:34
Markionium added a commit to Markionium/office-ui-fabric-react that referenced this pull request May 23, 2018
* master: (67 commits)
  Applying package updates.
  Revert ChoiceGroup change in 5.0 to minimize potential partner impact. (microsoft#4962)
  Applying package updates.
  ChoiceGroup: getStyles conversion (microsoft#4852)
  Export SASS variables and mixins (microsoft#4959)
  Variants: update algorithm (microsoft#4949)
  Allow for customization of keycodes that cause the focus rect to appear (microsoft#4948)
  Mergestyles facepile (microsoft#4950)
  Fixing circular dependency and non-AMD references in ContextualMenu (microsoft#4946)
  Clean up semantic slots (microsoft#4932)
  Added missing merge-styles background-size typing (microsoft#4935)
  Applying package updates.
  Split menu button styles (microsoft#4922)
  Experimental Chiclet Component (microsoft#4678)
  Remove hover and pressed background colors (microsoft#4908)
  Remove @cschleiden as Rating code owner (microsoft#4929)
  Addressing Issue microsoft#3691 - SplitButton: In Safari, the SplitButton has mismatched heights (microsoft#4797)
  Applying package updates.
  Adding option for focus on tags in disabled picker (microsoft#4833)
  Jest merge-styles serialization fix for animation-name (microsoft#4927)
  ...
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SplitButton: In Safari, the SplitButton has mismatched heights

3 participants