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

Fix unrecognisable buttons when changing colours #377

Merged
merged 1 commit into from Feb 8, 2017

Conversation

Projects
None yet
4 participants
@selfthinker
Copy link
Member

commented Jan 30, 2017

When overwriting colours via browser settings, Firefox always defaults buttons to a transparent background and IE defaults to a grey background, irrespective of said settings. When changing Windows system settings to a high contrast theme, Edge also defaults buttons to a transparent background.
That makes buttons appear just as normal text being a bit off kilter (due to the button padding).

To make them appear a bit more button-y again, this adds a transparent outline which is invisible in normal browser settings.

The only slightly negative side effect I found during testing is that the buttons in Safari have lost their 3px focus outline and have a 1px focus outline instead. That could be fixed by changing the transparent outline to 3px but that made some other things worse again.

Because Firefox always has black text on buttons, we actually have a completely invisible button on a dark background there. As far as I understand it, it's not fixable at all. I will file a bug report accordingly. Here is a reduced test case: http://jsbin.com/sibube

See also alphagov/govuk_elements#397.

Screenshots (form buttons)

This is the page I was testing, showing the default colours:
firefox-default

Before

Firefox:
firefox-light-before

IE 11:
ie-light-before

Edge via changes in system settings:
edge-dark-before

After

Firefox:
firefox-light-after

IE 11:
ie-light-after

Edge via changes in system settings:
edge-dark-after

Screenshots (link buttons)

The original start now button with default colours:
firefox-buttonlink-default

Before

Firefox:
firefox-buttonlink-light-before
firefox-buttonlink-dark-before

IE 11:
ie-buttonlink-light-before

After

Firefox:

firefox-buttonlink-light-after
firefox-buttonlink-dark-after

IE 11:
ie-buttonlink-light-after

@selfthinker selfthinker referenced this pull request Jan 30, 2017

Merged

Fix invisible input content when changing colours #397

1 of 6 tasks complete
@robinwhittleton

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2017

Great work 👍 It’d be good to see:

  • what the visual different in Safari is. I assume that’s on macOS and not iOS?
  • A bug number for Firefox attached to the commit so that it’s easily referenced in the future.
@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

The button looks like this in Safari when highlighted:
button-with-transparent-outline-in-safari

I tried to test in iOS but didn't know how to focus via BrowserStack. Any idea how to do that?

As not a designer I just saw a 1px border with a weird artefact. But when @edwardhorsford looked at it, he saw that it was 2px on the right and on the bottom, so it looked more like there was a 1px white outline on top of the original 3px outline.
What makes this weirder is that it's not 100% reproducible because I've seen the 3px outline in Safari as well at times. And when you tab further, the artefact on the left sometimes stays where it is although a completely different element has focus. And I've also seen it without the artefact.
All very weird...

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

I added another example of Edge with colour changes via system settings to the description.

Regarding adding the bug report to the commit, the bug I reported was for something the commit doesn't fix (because it cannot be fixed). As such, I don't think it belongs into the commit messages. Or should I add to the message that this doesn't fix buttons in Firefox, which then can be linked to the bug report?

@tvararu

tvararu approved these changes Feb 1, 2017

Copy link

left a comment

I think we should merge and ship this, it's a very simple fix with very good results from what I can see. Great work @selfthinker!

@robinwhittleton

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2017

Before this is merged I’d like to investigate what we could do for the focus outline in Safari. Could we drop outlines in browsers that support box-shadow and use that instead?

@robinwhittleton
Copy link
Contributor

left a comment

Would like to get Safari rendering issues fixed before going live with this.

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2017

I was just trying to find a way to focus on the button in iOS devices via BrowerStack. The only way I've found is by using the dev tools and toggling the element state to "focus". I couldn't reproduce it in the few iDevices I tested. I then thought I would double check in a few OS X Safaris other than mine and couldn't reproduce it either.

And then I remembered that most people wouldn't see the focus style at all on buttons because it needs to be enabled in the browser or system settings (it's two settings but either would enable focussing on buttons). I guess that's also why we wouldn't see it within BrowserStack.

When I then tried to investigate further within my local Safari, it only showed the 1px border and artefact when I tabbed to the button, but not when I inspected it (and toggled the state to "focus"). :( I can play around a bit more with a few other CSS rules. But other than that, I'm not sure what other options we have to fix it.

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2017

I can set the outline to 3px instead of 1px. That would fix it for Safari, but it makes the button look much bigger when changing the colours. That might make it look confusingly closer to input fields rather than buttons. See screenshots...

IE 11:
outline-3px-ie11-light

Firefox:
outline-3px-firefox-light

Do you think it's worth changing?

@36degrees

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2017

This has to be a Safari bug, right? The outline style should be overridden completely for focus, but it's taking the colour but using the width from the non-focussed variant?

http://codepen.io/36degrees/pen/KaoPrr

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Feb 1, 2017

It most probably is a Safari bug. My example is worse than yours, though, because of that artefact.

I played around a bit more with the CSS and found that the bug doesn't appear with default Safari buttons (which are used if you remove the border and background colour in your example; in our buttons that would also need removing of -webkit-appearance).

I personally wouldn't mind having just a 1px outline, if it wasn't for that ugly artefact which just makes it look unprofessional. But then again, it only happens for people who changed their browser or system settings who tab to the button.

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

@edwardhorsford and me tried a couple of more things...

We tried to only set the new transparent outline when the button doesn't have any focus. But that didn't solve the issue in Safari at all.

button:not(:focus) {
  outline: 1px solid transparent;
}

Then Ed remembered he had solved issues with outline-offset before and when we played around we found outline-offset: -1px fixes it (also in @36degrees's reduced test case). :)
That doesn't have any side effect in other browsers because other browsers all use the outline-offset: 0 when focussing on the button (which is set in govuk_template).

I will update the commit accordingly and re-test a few things.

Fix unrecognisable buttons when changing colours
When overwriting colours via browser settings,
Firefox always defaults buttons to a transparent background
and IE defaults to a grey background,
irrespective of said settings.
That makes buttons appear just as normal text
being a bit off kilter (due to the button padding).

To make them appear a bit more button-y again,
this adds a transparent outline
which is invisible in normal browser settings.

@selfthinker selfthinker force-pushed the fix-buttons-when-colours-change branch from 7ef813e to ff99977 Feb 2, 2017

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

@robinwhittleton, the Safari issue is fixed. And after a bit of testing I can confirm the fix doesn't seem to have any unwanted side effects in other browsers.

@robinwhittleton robinwhittleton merged commit 7daac5a into master Feb 8, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@robinwhittleton robinwhittleton deleted the fix-buttons-when-colours-change branch Feb 8, 2017

@robinwhittleton robinwhittleton referenced this pull request Mar 17, 2017

Merged

v5.1.2 #391

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.