Bug 265271 - REGRESSION (Safari 17.1) line-height is computed differently
Summary: REGRESSION (Safari 17.1) line-height is computed differently
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari 17
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-11-22 16:59 PST by Brad Andalman
Modified: 2023-12-22 07:21 PST (History)
7 users (show)

See Also:


Attachments
A simple HTML file that should be used to inspect a paragraph’s height (288 bytes, text/html)
2023-11-22 16:59 PST, Brad Andalman
no flags Details
A paragraph rendered in Safari 16.6 showing a line-height of 22px (734.87 KB, image/png)
2023-11-22 17:00 PST, Brad Andalman
no flags Details
A paragraph rendered in Safari 17.1 showing a line-height of 21px (774.52 KB, image/png)
2023-11-22 17:01 PST, Brad Andalman
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brad Andalman 2023-11-22 16:59:23 PST
Created attachment 468734 [details]
A simple HTML file that should be used to inspect a paragraph’s height

In Safari 17.1, the line-height calculation can return a different value than what’s returned in Safari 16.6. I’m not sure whether this is a bug or an intentional change, so I’m filing it in case it’s the former.

In Safari 16.6, I believe the calculation for line-height is: floor(round(fontSizeInPixels) * lineHeightInEms)
In Safari 17.1, it appears to be: floor(fontSizeInPixels * lineHeightInEms)

You can see this in the attached HTML (of a simple paragraph with font-size of 16.8px and line-height of 1.3): Safari 17.1 reports the height of the paragraph as 21px, while Safari 16.6 reports it as 22px.
Comment 1 Brad Andalman 2023-11-22 17:00:53 PST
Created attachment 468735 [details]
A paragraph rendered in Safari 16.6 showing a line-height of 22px
Comment 2 Brad Andalman 2023-11-22 17:01:13 PST
Created attachment 468736 [details]
A paragraph rendered in Safari 17.1 showing a line-height of 21px
Comment 3 zalan 2023-11-22 21:08:32 PST
Thank you for filing this issue!
265657@main (https://commits.webkit.org/265657@main) is the regression point but it looks like this change was intentional. 
we went from
  unsigned computedPixelSize() const { return unsigned(m_computedSize + 0.5f); }
to
  float computedSize() const { return m_computedSize; }
and the final line box height is computed as follows:
  floorf(minimumValueForLength(m_style.lineHeight, fontSize()) <- font size here is computed(Pixel)Size().

(just confirming your observation on how computation changed between 16.6 and 17.1)
Comment 4 Brad Andalman 2023-11-22 22:40:54 PST
Thanks so much for looking into this! 

That commit certainly does make it look like this change is intentional. Given that, should I close out this bug?

Alternatively, I could turn this bug into a request for API that would allow clients to query the “computedLineHeight” for an element. That way, if this calculation changes (again) in the future — perhaps for bug #261463? - pages that use this proposed function wouldn’t need to be updated.

As it is now, I’m re-implementing this WebKit line-height calculation, which is quite fragile (as I’ve just learned). I guess I could use a hidden single-line paragraph to query the height, but real API would be much better. Is that a realistic suggestion?
Comment 5 zalan 2023-11-28 10:07:51 PST
> As it is now, I’m re-implementing this WebKit line-height calculation, which
> is quite fragile (as I’ve just learned)
FWIW I am planning on removing all these seemingly random floor/ceil calls on line box sizing soonish (see bug 261212).
Comment 6 Radar WebKit Bug Importer 2023-11-29 17:00:14 PST
<rdar://problem/118957902>
Comment 7 Vitor Roriz 2023-12-22 05:33:43 PST
I agree with @zalan that this change seems to be intentional. As already mentioned, https://commits.webkit.org/265657@main replaced all calls to "computedPixelSize()" to "computedSize()" which doesn't round up the flat size value.

Therefore, I believe we can close this ticket if you agree with it Brad.
Comment 8 Brad Andalman 2023-12-22 07:21:20 PST
Marking this bug as resolved – as @zalan pointed out, it looks intentional.

Thanks for looking into this!