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 invisible input content when changing colours #397

Merged
merged 1 commit into from Feb 8, 2017

Conversation

Projects
None yet
3 participants
@selfthinker
Copy link
Member

commented Jan 30, 2017

When overwriting colours via browser settings, Firefox and IE always default to black text in form fields, irrespective of said settings.
When the background of the form fields is transparent and the general background is dark (e.g. black), it results in black text on a black background, i.e. making the text invisible and therefore completely inaccessible.

It seems counterintuitive but due to browser bugs (which I will report) it is necessary to remove the background-color: transparent. The only downside I can see is that anyone wanting to use input fields on a non-white background would need to give it a background colour (which would actually re-introduce this bug again in Firefox).

A reduced test case to play around with: http://jsbin.com/sibube
See also alphagov/govuk_frontend_toolkit#377.

What problem does the pull request solve?

It makes text within form fields accessible to partially sighted users who change their browser's colour settings in IE or Firefox.

What does it do?

It removes the transparent background from input fields.

How has this been tested?

I tested this in

  • Firefox and IE 11 and 8 with default colours, a dark and a light background via their browser settings
  • Chrome with default colours, a dark and a light background via the extensions "High Contrast" and "Change Colors"
  • Firefox, IE 11 and Edge with default colours, a dark and a light background via the Windows accessibility system settings
  • Various browser with different colours via Mac OS, iOS and Android accessibility system settings

Screenshots (if appropriate):

(Please ignore the buttons in those screenshots, I will open a different PR for that. Also ignore the wrong font.)

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

Before, i.e. showing the bug

Firefox:
firefox-dark-before

IE 11:
ie-dark-before

After, i.e. with fix

Firefox:
firefox-dark-after

IE 11:
ie-dark-after

What type of change is it?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Put an x in all the boxes that apply

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
@robinwhittleton

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2017

Yep, happy with this, with the addition of a link to the Firefox bug report in the commit message.

@nickcolley

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2017

In your example, if I set the background to black and the color to white, the text is not readable since it's the same color as the input. Is this a problem?

http://jsbin.com/bokikexizu/1/edit?html,css,js,output

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

That only happens because I've set the input's colour to inherit. In a real world scenario you would also change the input's background and text colour. But as that is what triggers the bug, I cannot incorporate that into the example.
But your change just made me re-test my example without the color: inherit, just to make sure that has nothing to do with the bug. And it doesn't, so good to know. :)

@nickcolley

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2017

I must be missing something but we do currently use color: inherit?

fdb36eb#diff-874ea218a8261b484fef9e13094e3b86R149

To be clear my suggestion is that if people have their own css style sheets they could be blocked by this?

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

Yes, we do. That's why I had it initially in the example. But that shouldn't have anything to do with the reduced test case. The test case shows the bug with or without color: inherit.

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

I just chatted to @nickcolley separately. His concern is either services using different colours or users using user stylesheets. In both cases the fact that we have color: inherit would mean they would need to change something. I think that's an edge case. But we could remove color: inherit again, just to be sure. That would only mean I would need to re-test all the things again.

The form control colour was set to inherit quite recently in 735955e. You could set the border colour via border-color: inherit as well instead.
The transparent background colour is quite recent as well (29f756b). I doubt it broke any colour settings from services or we would have heard from them? (Or maybe not, how often do we hear back in those cases?)

I personally don't think it's worth removing color: inherit but it probably wouldn't be harmful either. What do others think?

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

I just filed a bug report for the issue in Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1335476
It probably doesn't make sense to report it for Internet Explorer as Microsoft only really updates Edge and that doesn't allow colour changes.

@selfthinker selfthinker force-pushed the fix-input-when-colours-change branch from fdb36eb to 5d9c47f Jan 31, 2017

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Jan 31, 2017

I just updated the commit message to include the link to the bug report and I also extended the explanation a bit.

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2017

Prompted by another unrelated bug report, I will change this PR to remove the color: inherit as well (@nickcolley). We had a user report that this means he sees white text on a white background. That we use color: inherit is one half of the problem (the other half is that the rendering app in question gives input fields a background colour).

Here is an example with a white on dark background system setting but with unchanged browser settings (Firefox in this case):
firefox-dark-system-unchanged-browser-setting

Fix invisible input content when changing colours
When overwriting colours via browser settings,
Firefox and IE always default to black text in form fields,
irrespective of said settings.
When the background of the form fields is transparent
and the general background is dark (e.g. black),
it results in black text on a black background,
i.e. making the text invisible and therefore completely inaccessible.

The reason why that is a problem is because
both IE and Firefox hard-code the text colour
within input fields to be the system text colour.

Unfortunately the only solution is to not set any background.
And as foreground and background colours needs to be set together,
that means The text colour cannot be `inherit` either.

Bug report for Firefox:
https://bugzilla.mozilla.org/show_bug.cgi?id=1335476

@selfthinker selfthinker force-pushed the fix-input-when-colours-change branch from 5d9c47f to 13a9810 Feb 7, 2017

@selfthinker

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2017

I have just updated the commit again accordingly. I have also added a comment into the SCSS file instead of just removing the offending lines as it's something that can easily be overlooked and potentially introduced again.

@robinwhittleton

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2017

OK, I’m happy with this. Thanks for all the hard work in getting this sorted.

@robinwhittleton robinwhittleton merged commit 9bab8cd 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-input-when-colours-change branch Feb 8, 2017

robinwhittleton added a commit that referenced this pull request Mar 16, 2017

v3.0.0
- Breaking changes to form validation (#PR 405)

This adds two new classes: `.form-group-error` and `.form-control-error`, and removes the old `.error` class. This lets specific controls within a group be accurately marked as having errors. For example, this allows parts of a date entry to be marked rather than the whole section.

To upgrade, you’ll need to change any occurences of `.error` that were applied to form groups to be `.form-group-error`, and then add `.form-control-error` to the failing input inside.

- Radio buttons and checkboxes have new markup that doesn’t need JavaScript (PR #406)

Previously the radio buttons and checkboxes relied on selection-buttons.js (from govuk_frontend_toolkit) to work. By reworking the markup we’ve been able remove this requirement.

To upgrade, you’ll need to change your markup to match the new pattern. Once that has been done you’ll be able to stop including selection-buttons.js and remove the `GOVUK.SelectionButtons` constructor from your JavaScript. The look and feel remains the same barring some tweaks to the focus weight. Accessibility and device compatibility remains the same as the previous version.

- Add examples of phase tags used outside of phase banners (PR #366)
- Fix invisible input content when changing colours (PR #397)
- Recommend the use of `aria-current="page"` for the last breadcrumb (PR #418)

@robinwhittleton robinwhittleton referenced this pull request Mar 16, 2017

Merged

v3.0.0 #420

robinwhittleton added a commit that referenced this pull request Mar 16, 2017

v3.0.0
- Breaking changes to form validation (#PR 405)

This adds two new classes: `.form-group-error` and `.form-control-error`, and removes the old `.error` class. This lets specific controls within a group be accurately marked as having errors. For example, this allows parts of a date entry to be marked rather than the whole section.

To upgrade, you’ll need to change any occurences of `.error` that were applied to form groups to be `.form-group-error`, and then add `.form-control-error` to the failing input inside.

- Radio buttons and checkboxes have new markup that doesn’t need JavaScript (PR #406)

Previously the radio buttons and checkboxes relied on selection-buttons.js (from govuk_frontend_toolkit) to work. By reworking the markup we’ve been able remove this requirement.

To upgrade, you’ll need to change your markup to match the new pattern. Once that has been done you’ll be able to stop including selection-buttons.js and remove the `GOVUK.SelectionButtons` constructor from your JavaScript. The look and feel remains the same barring some tweaks to the focus weight. Accessibility and device compatibility remains the same as the previous version.

- Add examples of phase tags used outside of phase banners (PR #366)
- Fix invisible input content when changing colours (PR #397)
- Recommend the use of `aria-current="page"` for the last breadcrumb (PR #418)
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.