RESOLVED FIXED 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
https://bugs.webkit.org/show_bug.cgi?id=232939
Summary REGRESSION(r281389): Text wraps unnecessarily within intrinsically-sized elem...
Oliver Byford
Reported 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.
Attachments
Safari 15 (245.95 KB, image/png)
2021-11-10 04:11 PST, Oliver Byford
no flags
Safari Tech Preview r132 (241.80 KB, image/png)
2021-11-10 04:11 PST, Oliver Byford
no flags
Safari Tech Preview r134 (791.31 KB, image/jpeg)
2021-11-10 04:12 PST, Oliver Byford
no flags
Safari Tech Preview r132 – using Arial (262.17 KB, image/png)
2021-11-10 04:12 PST, Oliver Byford
no flags
Depends on bug 234428 (5.07 KB, patch)
2021-12-17 01:42 PST, Myles C. Maxfield
no flags
Patch for EWS (11.78 KB, patch)
2021-12-17 01:45 PST, Myles C. Maxfield
no flags
Patch (5.80 KB, patch)
2021-12-17 16:22 PST, Myles C. Maxfield
no flags
Patch (5.97 KB, patch)
2022-01-05 19:49 PST, Myles C. Maxfield
zalan: review+
Oliver Byford
Comment 1 2021-11-10 04:11:56 PST
Created attachment 443799 [details] Safari Tech Preview r132
Oliver Byford
Comment 2 2021-11-10 04:12:12 PST
Created attachment 443800 [details] Safari Tech Preview r134
Oliver Byford
Comment 3 2021-11-10 04:12:32 PST
Created attachment 443801 [details] Safari Tech Preview r132 – using Arial
Oliver Byford
Comment 4 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.
zalan
Comment 5 2021-11-10 06:53:41 PST
Thank you for reporting it. I can reproduce it. Will look into it shortly.
Radar WebKit Bug Importer
Comment 6 2021-11-10 08:18:30 PST
zalan
Comment 7 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.
Myles C. Maxfield
Comment 8 2021-11-15 02:29:33 PST
This is a really bad bug and we should fix it immediately.
Myles C. Maxfield
Comment 9 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.
Oliver Byford
Comment 10 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.
Myles C. Maxfield
Comment 11 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.
Myles C. Maxfield
Comment 12 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.
Myles C. Maxfield
Comment 13 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.
Myles C. Maxfield
Comment 14 2021-12-03 02:56:24 PST
Oh, the same thing happens for the trailing newline using stripTrailingSpace().
Myles C. Maxfield
Comment 15 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.
Myles C. Maxfield
Comment 16 2021-12-14 12:52:29 PST
I'll try to fix this today.
Myles C. Maxfield
Comment 17 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.
Myles C. Maxfield
Comment 18 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.
Myles C. Maxfield
Comment 19 2021-12-17 01:42:17 PST
Myles C. Maxfield
Comment 20 2021-12-17 01:45:52 PST
Created attachment 447442 [details] Patch for EWS
Myles C. Maxfield
Comment 21 2021-12-17 01:55:59 PST
After applying this patch, https://design-system.service.gov.uk/ looks right.
Myles C. Maxfield
Comment 22 2021-12-17 10:41:14 PST
Uh oh, test failures!
Myles C. Maxfield
Comment 23 2021-12-17 16:22:46 PST
zalan
Comment 24 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?
Myles C. Maxfield
Comment 25 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.
Oliver Byford
Comment 26 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.
Myles C. Maxfield
Comment 27 2022-01-05 19:49:26 PST
zalan
Comment 28 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 ] -->
Myles C. Maxfield
Comment 29 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.
Myles C. Maxfield
Comment 30 2022-01-06 15:50:36 PST
Note You need to log in before you can comment on or make changes to this bug.