Bug 232939 - 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
Summary: REGRESSION(r281389): Text wraps unnecessarily within intrinsically-sized elem...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari Technology Preview
Hardware: Mac (Intel) macOS 11
: P1 Major
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on: 234428
Blocks:
  Show dependency treegraph
 
Reported: 2021-11-10 04:11 PST by Oliver Byford
Modified: 2022-01-06 15:50 PST (History)
7 users (show)

See Also:


Attachments
Safari 15 (245.95 KB, image/png)
2021-11-10 04:11 PST, Oliver Byford
no flags Details
Safari Tech Preview r132 (241.80 KB, image/png)
2021-11-10 04:11 PST, Oliver Byford
no flags Details
Safari Tech Preview r134 (791.31 KB, image/jpeg)
2021-11-10 04:12 PST, Oliver Byford
no flags Details
Safari Tech Preview r132 – using Arial (262.17 KB, image/png)
2021-11-10 04:12 PST, Oliver Byford
no flags Details
Depends on bug 234428 (5.07 KB, patch)
2021-12-17 01:42 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch for EWS (11.78 KB, patch)
2021-12-17 01:45 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (5.80 KB, patch)
2021-12-17 16:22 PST, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (5.97 KB, patch)
2022-01-05 19:49 PST, Myles C. Maxfield
zalan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Byford 2021-11-10 04:11:34 PST
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.
Comment 1 Oliver Byford 2021-11-10 04:11:56 PST
Created attachment 443799 [details]
Safari Tech Preview r132
Comment 2 Oliver Byford 2021-11-10 04:12:12 PST
Created attachment 443800 [details]
Safari Tech Preview r134
Comment 3 Oliver Byford 2021-11-10 04:12:32 PST
Created attachment 443801 [details]
Safari Tech Preview r132 – using Arial
Comment 4 Oliver Byford 2021-11-10 06:24:00 PST
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.
Comment 5 zalan 2021-11-10 06:53:41 PST
Thank you for reporting it. I can reproduce it. Will look into it shortly.
Comment 6 Radar WebKit Bug Importer 2021-11-10 08:18:30 PST
<rdar://problem/85254819>
Comment 7 zalan 2021-11-10 08:53:00 PST
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.
Comment 8 Myles C. Maxfield 2021-11-15 02:29:33 PST
This is a really bad bug and we should fix it immediately.
Comment 9 Myles C. Maxfield 2021-11-15 02:32:29 PST
Also, this is a fantastic bug report. I don't know if I've ever seen a bug report this detailed before.
Comment 10 Oliver Byford 2021-11-29 08:30:33 PST
@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.
Comment 11 Myles C. Maxfield 2021-11-29 10:16:03 PST
I can’t talk about future products or releases but I can say I plan to start fixing this bug this week.
Comment 12 Myles C. Maxfield 2021-12-03 02:43:41 PST
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.
Comment 13 Myles C. Maxfield 2021-12-03 02:52:30 PST
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.
Comment 14 Myles C. Maxfield 2021-12-03 02:56:24 PST
Oh, the same thing happens for the trailing newline using stripTrailingSpace().
Comment 15 Myles C. Maxfield 2021-12-03 03:01:15 PST
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.
Comment 16 Myles C. Maxfield 2021-12-14 12:52:29 PST
I'll try to fix this today.
Comment 17 Myles C. Maxfield 2021-12-16 19:57:57 PST
Oh, I guess there's another requirement to trigger this bug: We're not using IFC to lay out the text.
Comment 18 Myles C. Maxfield 2021-12-17 00:54:50 PST
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.
Comment 19 Myles C. Maxfield 2021-12-17 01:42:17 PST
Created attachment 447441 [details]
Depends on bug 234428
Comment 20 Myles C. Maxfield 2021-12-17 01:45:52 PST
Created attachment 447442 [details]
Patch for EWS
Comment 21 Myles C. Maxfield 2021-12-17 01:55:59 PST
After applying this patch, https://design-system.service.gov.uk/ looks right.
Comment 22 Myles C. Maxfield 2021-12-17 10:41:14 PST
Uh oh, test failures!
Comment 23 Myles C. Maxfield 2021-12-17 16:22:46 PST
Created attachment 447491 [details]
Patch
Comment 24 zalan 2021-12-17 21:48:25 PST
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?
Comment 25 Myles C. Maxfield 2021-12-17 23:51:45 PST
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.
Comment 26 Oliver Byford 2021-12-22 01:44:56 PST
(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.
Comment 27 Myles C. Maxfield 2022-01-05 19:49:26 PST
Created attachment 448463 [details]
Patch
Comment 28 zalan 2022-01-05 19:54:19 PST
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 ] -->
Comment 29 Myles C. Maxfield 2022-01-06 08:34:43 PST
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.
Comment 30 Myles C. Maxfield 2022-01-06 15:50:36 PST
Committed r287724 (245805@trunk): <https://commits.webkit.org/245805@trunk>