Fixing high contrast bugs from issue #3102 #3301

kristoferbrown · 2017-11-02T21:37:20Z

Pull request checklist

Addresses an existing issue: Fixes High contrast issues in components #3102
Include a change request file using $ npm run change

Description of changes

Resolves high contrast bugs in

ChoiceGroup
PeoplePicker
SwatchColorPicker
Dropdown
Persona
ColorPicker
PrimaryButton
MessageBar
Detailslist Check
Toggle
ProgressIndicator
Spinner

…ample

… updates

kristoferbrown · 2017-11-02T21:44:48Z

@micahgodbolt, I'd love for you to take a look at what I did in Button to make sure I'm handling the ccs in js/theming stuff right.

@lynamemi, I think you mentioned wanting to see how high contrast stuff works, so here's a PR with several changes. 99% of it is using @include high-contrast to target HC mode in Edge, changing some colors to system color names, and then maybe setting -ms-high-contrast-adjust: none if text has weird unexpected backgrounds.

lynamemi · 2017-11-02T22:20:21Z

packages/office-ui-fabric-react/src/components/Button/examples/Button.Anchor.Example.tsx

@@ -24,7 +24,6 @@ export class ButtonAnchorExample extends React.Component<IButtonProps, {}> {
          href='http://bing.com'
          target='_blank'
          title='Let us bing!'
-          style={ { color: '#ffffff' } }


I know this looks totally out of place, but I added this in because UHF in the website overrides our link styles. Maybe there is just a better way to go about it? Is it causing a HC issue?

I removed it because we're changing the HC primary button style to black on white and this override made it white on white. I'm not seeing any issue with it in non-HC mode after removing this line, so I think it should be OK to remove now.

Unfortunately the non-HC mode issue only shows up when we deploy the website. The local build doesn't have the UHF. Maybe you can test the live site in the browser to double check it's ok to remove it?

Ah ok, that makes sense. Seems like it's still in issue. I'll come up with another way to fix this HC issue without regressing on the issue you fixed and then let you know.

Awesome, thank you! I'll stay tuned.

@lynamemi, I think we can sidestep this whole issue by just making this example button not primary. Let me know if you see any issue with that approach.

Also, the Persona/ActivityItem issue we saw this morning should be fixed in this PR now.

Very good point! That change to standard and the updates for activity item look good to me!

phkuo · 2017-11-06T19:59:44Z

@kristoferbrown did you investigate fixing the HTML first to be semantically correct for each of these? This only fixes these for IE/Edge, and I understand we can't get it perfect on FireFox since we lack these selectors, but we should still encourage semantically correct HTML over messy media queries.

kristoferbrown · 2017-11-06T21:56:48Z

@phkuo, I'll do another pass over all of the changes in this PR and see if any of these fixes can be replaced with HTML changes.

kristoferbrown · 2017-11-07T00:46:23Z

@phkuo, I skimmed over all the files changed in this PR and didn't find any of the fixes that could be accomplished just by changing the HTML. Everything in this PR is handling a weird edge case to begin with, so all these media queries are needed to fix them.

kristoferbrown · 2017-11-16T00:37:19Z

@phkuo, any chance you could take another look at this PR so we can get it out the door?

phkuo · 2017-11-16T01:00:02Z

packages/office-ui-fabric-react/src/components/ActivityItem/ActivityItem.styles.ts

+      selectors: {
+        '.ms-Persona-imageArea': {
+          marginTop: '-2px',
+          border: '2px solid #fff',


this needs to use a themed color

This has been changed to theme.palette.white

phkuo · 2017-11-16T01:00:38Z

packages/office-ui-fabric-react/src/components/Button/ButtonThemes.ts

@@ -1,6 +1,8 @@
 import { IButtonStyles } from './Button.Props';
 import { ITheme } from '../../Styling';

+const MS_HIGHCONTRAST_ACTIVE = '@media screen and (-ms-high-contrast: active)';


this const should be in a shared file somewhere, no?

Good timing on this, David just added this to CommonStyles. I've changed over every other instance of this to use the one defined there.

phkuo · 2017-11-16T01:02:43Z

packages/office-ui-fabric-react/src/components/Button/ButtonThemes.ts

+        [MS_HIGHCONTRAST_ACTIVE]: {
+          color: 'Window',
+          backgroundColor: 'WindowText',
+          MsHighContrastAdjust: 'none'


if we actually need this, it's really bad practice, we shouldn't be having usage-necessary images being background images. this is something we can definitely do by tweaking the HTML, or using a :before

@phkuo, I'm not sure what the issue is here, there is no background image. The idea is just that primary buttons should use a background color instead of a border like normal buttons. Can you clarify what you'd like to see changed here?

Isn't -ms-high-contrast-adjust:none used to show the background image? If yes, see previous comment.

If it's only used so that the background color shows, then shouldn't setting background color directly (as is already done here) be sufficient?

-ms-high-contrast-adjust affects a few different things as well. Namely, it turns off the Window colored highlight that Edge (but not IE) gives all text. It's invisible most of the time, but any time there is an intentional background color or the text is close to other elements it covers things up.

Without setting it here, things like this happen.

* High contrast fixes: choiceGroup, persona, suggestions, commandbar example * Increasing visibilty of focus border on choice group elements. * picker selected item high contrast improvements * Removing swatchColorPicker high contrast borders * Primary button, dropdown, and colorPicker contrast adjustments * Details check, message bar x button, progress, spinner, and toggle HC updates * rush change * Build fixes * Fixing persona/activityItem border issues * Changing achor button example to a non-primary button * Using non-deprecated defaultButton * changing compact border to palette color * Removing high-contrast const definitions from all style ts files

Kris Brown added 8 commits October 19, 2017 12:42

High contrast fixes: choiceGroup, persona, suggestions, commandbar ex…

baa55a5

…ample

Increasing visibilty of focus border on choice group elements.

c80ff52

Merge branch 'master' into v-krbrow/oct-contrast-bugs

69e635e

picker selected item high contrast improvements

130ce7e

Removing swatchColorPicker high contrast borders

f0f532a

Primary button, dropdown, and colorPicker contrast adjustments

645f7f9

Details check, message bar x button, progress, spinner, and toggle HC…

94491ee

… updates

rush change

fedb23d

kristoferbrown requested a review from phkuo as a code owner November 2, 2017 21:37

kristoferbrown requested a review from micahgodbolt November 2, 2017 21:38

kristoferbrown assigned gokunymbus and unassigned gokunymbus Nov 2, 2017

kristoferbrown requested a review from gokunymbus November 2, 2017 21:38

Build fixes

32371a2

lynamemi reviewed Nov 2, 2017

View reviewed changes

Kris Brown added 2 commits November 6, 2017 13:26

Fixing persona/activityItem border issues

29b112c

Changing achor button example to a non-primary button

b8e81ef

Using non-deprecated defaultButton

2178c3f

phkuo requested changes Nov 16, 2017

View reviewed changes

Kris Brown added 3 commits November 16, 2017 12:52

changing compact border to palette color

58f7897

Merge branch 'master' into v-krbrow/oct-contrast-bugs

2f2aca3

Removing high-contrast const definitions from all style ts files

76a6408

phkuo approved these changes Nov 17, 2017

View reviewed changes

kristoferbrown merged commit 067363f into microsoft:master Nov 17, 2017

lynamemi mentioned this pull request Dec 15, 2017

Website: UHF link color override for new button on GetStarted page #3585

Merged

2 tasks

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing high contrast bugs from issue #3102 #3301

Fixing high contrast bugs from issue #3102 #3301

kristoferbrown commented Nov 2, 2017

kristoferbrown commented Nov 2, 2017

lynamemi Nov 2, 2017

kristoferbrown Nov 2, 2017

lynamemi Nov 2, 2017

kristoferbrown Nov 2, 2017

lynamemi Nov 3, 2017

kristoferbrown Nov 6, 2017

lynamemi Nov 7, 2017 •

edited

Loading

phkuo commented Nov 6, 2017

kristoferbrown commented Nov 6, 2017

kristoferbrown commented Nov 7, 2017

kristoferbrown commented Nov 16, 2017

phkuo Nov 16, 2017

kristoferbrown Nov 16, 2017

phkuo Nov 16, 2017

kristoferbrown Nov 16, 2017

phkuo Nov 16, 2017

kristoferbrown Nov 16, 2017

phkuo Nov 16, 2017

kristoferbrown Nov 16, 2017

Fixing high contrast bugs from issue #3102 #3301

Fixing high contrast bugs from issue #3102 #3301

Conversation

kristoferbrown commented Nov 2, 2017

Pull request checklist

Description of changes

kristoferbrown commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lynamemi Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

phkuo commented Nov 6, 2017

kristoferbrown commented Nov 6, 2017

kristoferbrown commented Nov 7, 2017

kristoferbrown commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lynamemi Nov 7, 2017 •

edited

Loading