REGRESSION(r281389): Text wraps unnecessarily within intrinsically-sized elements when using certain fonts and the inner HTML of the element contains a new line that is not preceded by a space

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Oliver Byford]

Created attachment 443798 [details]
Safari 15

We're seeing an issue with text being aggressively wrapped when it doesn't need to be in Safari Technology Preview r132 and above – we've also tested with r131 and the problem does not occur there, so are fairly confident this is a regression that was introduced in r132.

This seems to be a compatibility issue with our particular font, as the problem does not occur if remove the font from our font stack and fall back to Arial.

## Steps to reproduce

1. Open https://design-system.service.gov.uk in both Safari Technology Preview (r132 or later) and a current stable version of Safari (I've checked in both v15.0 (16612.1.29.41.4) and v15.1 (17612.2.91.20) and they display correctly)
2. Compare the layout in the two browsers, paying attention to how text is being wrapped

## Expected results

The layout and wrapping of text should be consistent between the current stable version of Safari and Safari Technology Preview.

## Actual results

Text is aggressively wrapped when it doesn't need to be in Safari Technology Preview.

So far we've observed the problem affecting:

- the logo in the header
- the product name in the header ('Design System')
- service names in the header component (https://design-system.service.gov.uk/components/header/with-service-name/index.html)
- text in the button component (https://design-system.service.gov.uk/components/button/default/index.html)
- text in the phase banner component (https://design-system.service.gov.uk/components/phase-banner/default/index.html)
- section headings in the accordion component (https://design-system.service.gov.uk/components/accordion/default/index.html)
- summary text in the details component (https://design-system.service.gov.uk/components/details/default/index.html)
- legends in fieldsets (https://design-system.service.gov.uk/components/date-input/date-of-birth/index.html)
- labels for the checkboxes and radios components (https://design-system.service.gov.uk/components/radios/hint/index.html)
- tab labels in the tabs component (https://design-system.service.gov.uk/components/tabs/default/index.html)
- tags (https://design-system.service.gov.uk/components/tag/coloured-tags/index.html)

I've marked this issue as major severity as we're not seeing this happening in other browsers and, if this regression hit a stable release of Safari it would affect the display of elements across GOV.UK – the website for the UK government. I've attached screenshots for comparison, as well as a version where we fall back to Arial and the problems no longer occur.

We're tracking this internally at https://github.com/alphagov/govuk-frontend/issues/2429.

[Oliver Byford]

Created attachment 443799 [details]
Safari Tech Preview r132

[Oliver Byford]

Created attachment 443800 [details]
Safari Tech Preview r134

[Oliver Byford]

Created attachment 443801 [details]
Safari Tech Preview r132 – using Arial

[Oliver Byford]

From further testing, this appears to be an issue when:

- using certain fonts – it happens with our font ('GDS Transport'), but not Roboto
- an element is intrinsically sized (e.g. `display: inline-block`)
- there is at least one new line anywhere within the inner HTML of the element – unless there is a space immediately before the new line

See https://codepen.io/36degrees/pen/abyjzNr for a minimum viable example of the issue.

[zalan]

Thank you for reporting it. I can reproduce it. Will look into it shortly.

[Radar WebKit Bug Importer]

<rdar://problem/85254819>

[zalan]

This is caused by the
-#if !USE(FREETYPE)
+#if PLATFORM(WIN)
changes in r281389. Maybe it made an existing bug in our layout/text code visible.

[Myles C. Maxfield]

This is a really bad bug and we should fix it immediately.

[Myles C. Maxfield]

Also, this is a fantastic bug report. I don't know if I've ever seen a bug report this detailed before.

[Oliver Byford]

@Myles – thanks, that's kind of you. I'm glad the detail in the bug report was useful.

I just wanted to check in and see if there'd been any progress on this. We're a little nervous that this bug could still end up in a stable release of Safari – can we be reasonably confident that won't happen?

I'd also be really curious to know if there's something specific to the font that we're using that's causing the problem to occur, as I've yet to see it happening on other sites.

[Myles C. Maxfield]

I can’t talk about future products or releases but I can say I plan to start fixing this bug this week.

[Myles C. Maxfield]

Forcing the newline character to have the same width as the space glyph in WidthIterator::applyCSSVisibilityRules() fixes this, but I don't understand why. I need to continue investigating.

[Myles C. Maxfield]

I think I see. There's a block at the bottom of RenderText::computePreferredLogicalWidths() which is only hit if wordLen == 0 and !isNewline (which is the case here) which measures the width of the one character at index i and adds it to currMaxWidth. In our situation, that's the leading newline character.

Then, later, we subtract it out like this: "widths.max -= font.width(RenderBlock::constructTextRun(&space, 1, style));". This expects that the width of this initial character is equal to the width of a space.

[Myles C. Maxfield]

Oh, the same thing happens for the trailing newline using stripTrailingSpace().

[Myles C. Maxfield]

This bug appears to occur:

1. If the font's newline character has a width that's different than the space character's width
2. If an element is shrink-wrapped
3. And the element starts or ends with a newline character

That's a pretty common set of conditions. It's surprising we haven't seen this bug earlier.

[Myles C. Maxfield]

I'll try to fix this today.

[Myles C. Maxfield]

Oh, I guess there's another requirement to trigger this bug: We're not using IFC to lay out the text.

[Myles C. Maxfield]

So I think the old code was actually correct. If you make an element with some newlines in it, and then you make a duplicate element except with spaces instead of newlines, all the other browsers display these identically. I think it would probably be a mistake to expose the widths of the newline characters to the web, since the width of those characters is probably bogus in most fonts, since that value isn't actually observable anywhere.

So I think this is a pretty straightforward fix, then.

[Myles C. Maxfield]

Created attachment 447441 [details]
Depends on bug 234428

[Myles C. Maxfield]

Created attachment 447442 [details]
Patch for EWS

[Myles C. Maxfield]

After applying this patch, https://design-system.service.gov.uk/ looks right.

[Myles C. Maxfield]

Uh oh, test failures!

[Myles C. Maxfield]

Created attachment 447491 [details]
Patch

[zalan]

Comment on attachment 447491 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447491&action=review

> LayoutTests/fast/text/newline-width-expected.html:8
> +    white-space: pre-wrap;
> +    white-space: nowrap;

pre-nowrap :)

> LayoutTests/fast/text/newline-width-expected.html:16
> +div:before {
> +    content: "";
> +    position: absolute;
> +}

why is before content required to trigger this behavior? A simple shrink-fit content is not sufficient?

[Myles C. Maxfield]

Comment on attachment 447491 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=447491&action=review

>> LayoutTests/fast/text/newline-width-expected.html:8
>> +    white-space: nowrap;
> 
> pre-nowrap :)

whoops

>> LayoutTests/fast/text/newline-width-expected.html:16
>> +}
> 
> why is before content required to trigger this behavior? A simple shrink-fit content is not sufficient?

Correct - a simple shrink-fit content is not sufficient. I didn't investigate that part; I assumed this was a way to opt out of IFC.

[Oliver Byford]

(In reply to Myles C. Maxfield from comment #25)

> Correct - a simple shrink-fit content is not sufficient. I didn't
> investigate that part; I assumed this was a way to opt out of IFC.

It looks like this might has changed since I reported the bug – from testing quickly in r137 the bug is no longer occurring in the simplified CodePen examples (https://codepen.io/36degrees/pen/abyjzNr) or on a small number of the components/pages that I listed in my initial bug report.

[Myles C. Maxfield]

Created attachment 448463 [details]
Patch

[zalan]

Comment on attachment 448463 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448463&action=review

> LayoutTests/ChangeLog:10
> +        The div:before { position: absolute; } is necessary to trigger this bug; I assume it's necessary to
> +        opt-out of IFC.

maybe use <!DOCTYPE html> <!-- webkit-test-runner [ LayoutFormattingContextIntegrationEnabled=false ] -->

[Myles C. Maxfield]

Comment on attachment 448463 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=448463&action=review

>> LayoutTests/ChangeLog:10
>> +        opt-out of IFC.
> 
> maybe use <!DOCTYPE html> <!-- webkit-test-runner [ LayoutFormattingContextIntegrationEnabled=false ] -->

Oh, I didn't know about that. Good idea.

[Myles C. Maxfield]

Committed r287724 (245805@trunk): <https://commits.webkit.org/245805@trunk>