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 for select height zoom problem #1018

Merged
merged 1 commit into from
Aug 5, 2019
Merged

Conversation

andysellick
Copy link
Contributor

@andysellick andysellick commented Aug 2, 2019

What

This is a fix for a problem noticed during user research where a participant had their phone text size set very high. This resulted in the text in the select component being cropped slightly. This can be reproduced using Firefox's 'zoom text only' option, and it looks like this:

Screen Shot 2019-08-02 at 14 18 00

I've raised an issue in govuk-frontend but it's likely that their fix will not help on GOV.UK as we're using compatibility mode for elements and toolkit, and this issue apparently relates to the base font size.

The problem is caused by the height of selects being set at 40px. The best solution seems to be to set the height using em, so that it scales with the text size of the select. An alternative is to set it using rem but since the component guide doesn't set a base font size this results in the height appearing differently there and in finder-frontend.

I think (correct me if I'm wrong) that as long as there's an explicit font size on the select (which there is) even if it's specified using em (which it is) as long as the height is greater than 1em (which it is now) then the height of the select will always exceed the height of the text, so this problem shouldn't occur.

Screen Shot 2019-08-02 at 14 26 32

Why

Well, it's broken.

Visual Changes

I've overridden the height of 40px with a height of 2.14em, which appears to be the same, so there should be no visual changes. I'm pretty sure we're only using this component in finder-frontend, so we only need to test the search and finders pages.

View Changes

https://govuk-publishing-compo-pr-1018.herokuapp.com/component-guide/select

Trello card: https://trello.com/c/VWJHWlBC/922-fix-facet-text-zoom-problem

@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1018 August 2, 2019 13:29 Inactive
@andysellick andysellick force-pushed the fix-select-height-problem branch from 9e69ef1 to 04eaa63 Compare August 2, 2019 13:31
@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1018 August 2, 2019 13:31 Inactive
@andysellick andysellick force-pushed the fix-select-height-problem branch from 04eaa63 to cf41edd Compare August 2, 2019 13:35
@bevanloon bevanloon temporarily deployed to govuk-publishing-compo-pr-1018 August 2, 2019 13:35 Inactive
// Solution to text inside selects becoming unreadable if font size in
// the browser is increased. This is currently a problem in govuk-frontend
.gem-c-select {
.govuk-select {
Copy link
Contributor

@NickColley NickColley Aug 2, 2019

Choose a reason for hiding this comment

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

It'd be good to catchup with @alex-ju I think you may need to cater for both cases:

  1. compatibility mode turned on
  2. compatibility mode turned off

I still think we could consider a fix upstream in govuk-frontend first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still think we could consider a fix upstream in govuk-frontend first?

Yes, if that's an option. Sorry, the impression I had was that it wasn't. Happy to have that happen, although we need a fix quickly and I didn't want to put pressure on anyone.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've raised an issue in govuk-frontend but it's likely that their fix will not help on GOV.UK as we're using compatibility mode for elements and toolkit, and this issue apparently relates to the base font size.

I think I may have miscommunicated, we already output different units for the font scale depending on if the compatibility mode is set, we may be able to use similar logic to solve this issue too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Spoke with Andy, and we'll try for a long term fix on govuk-frontend but that shouldnt block quicker improvements here.

Copy link
Contributor

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

I would recommend using a height: auto on <select> elements (I'm doing the same in Content Publisher). This also helps with making the <select multiple> version look like a list of things you can choose from without having to scroll the list with only one item visible at a time.
By doing it this way, a basic select will still render at 40px height by default.

Update: the 40px height will only be preserved when the compatibility mode is not enabled. Another good reason to work towards dropping legacy stuff.

@andysellick
Copy link
Contributor Author

@alex-ju this change shouldn't affect select multiples, should it? It's specific to the select component, which doesn't have a multiple option.

I just want to check this won't break anything.

@alex-ju
Copy link
Contributor

alex-ju commented Aug 2, 2019

@andysellick yep, sorry if I caused confusion, the select in govuk_publishing_components doesn't allow a multiple attribute at the moment, but the one in govuk-frontend does.

@andysellick
Copy link
Contributor Author

@alex-ju have tested this with compatibility mode on and off, doesn't seem to make a difference, works fine in both.

@alex-ju alex-ju mentioned this pull request Aug 5, 2019
@alex-ju
Copy link
Contributor

alex-ju commented Aug 5, 2019

Needs a rebase so we can get it into 17.21.0

@andysellick andysellick force-pushed the fix-select-height-problem branch from cf41edd to 047b0df Compare August 5, 2019 09:52
@andysellick andysellick merged commit 69211a8 into master Aug 5, 2019
@andysellick andysellick deleted the fix-select-height-problem branch August 5, 2019 10:03
36degrees added a commit that referenced this pull request Jun 1, 2023
This was introduced in August 2019 [1] to mitigate an issue in GOV.UK Frontend where the height of a select component was set in pixels, which meant it did not scale if users changed the text size in their browser [2].

That issue was fixed in GOV.UK Frontend [3] and released as part of v3.3.0 [4] in October 2019. Since then the height of a select component has been set in rem (as long as compatibility mode is not enabled) which means this adjustment can be removed.

[1]: #1018
[2]: alphagov/govuk-frontend#1519
[3]: alphagov/govuk-frontend#1574
[4]: https://github.com/alphagov/govuk-frontend/releases/tag/v3.3.0#:~:text=Pull%20request%20%231574%3A%20Make%20form%20elements%20scale%20correctly%20when%20text%20resized%20by%20user.
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.

4 participants