alphagov / govuk-frontend

GOV.UK Frontend contains the code you need to start building a user interface for government platforms and services.
https://frontend.design-system.service.gov.uk/
MIT License
1.17k stars 320 forks source link

Fieldset legend text can be clipped when using a custom or fallback font #1239

Closed stevesims closed 5 years ago

stevesims commented 5 years ago

The SASS for .govuk-fieldset__legend has a line that sets overflow: hidden, as follows: https://github.com/alphagov/govuk-frontend/blob/b52474c4c1c98c9886b48caafa3746bf7782859d/src/components/fieldset/_fieldset.scss#L25

From what I can tell the webkit bug this relates to was this one which was fixed over 6 years ago.

Having this in there can mean that if the NTA font isn't present, the bottom of legends can get chopped off for larger font sizes, and the descenders in Arial end up overflowing the element and thus hidden.

NickColley commented 5 years ago

Hey @stevesims, thanks for letting us know about this.

I have reproduced this here:

https://govuk-frontend-issue-1239.glitch.me/

You can see the descenders from the 'g' in the title is clipped.

screen shot 2019-03-05 at 17 48 05 screen shot 2019-03-05 at 17 53 15

You can see that with the regular headings that they also clip, but because they do not have this 'overflow: hidden' you will not notice it.

screen shot 2019-03-05 at 17 54 30

I think this is likely because the font scale has been designed with NTA only in mind.

We have a new version of the font that has a different baseline which might avoid this issue.

@dashouse might have some insight into what is the best thing to do here.

NickColley commented 5 years ago

I've updated the title to make it focused on the problem as there may be a different solution to this issue, hope this is OK

dashouse commented 5 years ago

Good spot, we did a fair bit of checking between NTA and Arial but the legend specific styles were added a couple of months after that so didn't spot this.

The fact it's overflowing generally is ok, I believe it's because the line-height for the larger font sizes is actually smaller than the em-box of the font so they technically overlap even though the actual glyphs don't. This is to have a computed line height of 50px to lock it in the the baseline grid as well as feeling about right.

The newer cut of the font sits in the same position as Arial / Helvetica so we'll have to test this.

Do we know what versions of browsers would be affected by removing overflow: hidden?

36degrees commented 5 years ago

The 'hack' was introduced in https://github.com/alphagov/govuk_elements/pull/484 in June 2017, so I don't think it relates to the bug you reference given how old it is.

From reading that PR, I think it's a fix to allow hint text within legends to have margins.

As we don't put hints or error messages within the legends any more perhaps it's not be a problem any more, and the hack can be removed? We'd need to test it thoroughly in all of our supported browsers.

stevesims commented 5 years ago

I just read around a bit more on this. I can't find much by googling about that's "current" (within the past 3 years). The references on alphagov/govuk_elements#484 point towards a rejected PR alphagov/govuk_elements#76 which contains the comment:

In Webkit, the bottom margin of a legend and the top margin of the next element don't overlap, which they should. This addresses that by discarding the top margin of the next element

This may well be the underlying issue that was attempted to be addressed. (Again tho, I'm not sure whether this would still be an existant issue in WebKit/Blink based browsers - would need to experiment/test to find out.)

Given that spacing between elements in govuk-frontend primarily (with only a few exceptions) works through the use of margin-bottom it does feel that this would no longer be an issue.

andymantell commented 5 years ago

Just to say that I have observed this with the NTA font as well, it doesn't just seem to occur with Arial or other fallbacks. As alluded to above, I suspect this has now got worse with the new cut of the font. See demo here:

https://bush-monarch.glitch.me/

And screenshot of the above:

image

NickColley commented 5 years ago

Note that your example Andy has overriden font (since it's not on a gov.uk domain), so the demonstration Glitch apps use a custom font.

Here's what happens if you don't use a custom font:

font is not clipped