WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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
<
rdar://problem/85254819
>
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
Created
attachment 447441
[details]
Depends on
bug 234428
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
Created
attachment 447491
[details]
Patch
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
Created
attachment 448463
[details]
Patch
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
Committed
r287724
(
245805@trunk
): <
https://commits.webkit.org/245805@trunk
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug