Bug 263698

Summary:

A floating element can cause the following element’s text-indent to fail

Product:

WebKit

Reporter:

Brad Andalman <bya>

Component:

Layout and Rendering

Assignee:

alan <zalan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, bfulgham, bw, commit-queue, koivisto, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

Safari 17

Hardware:

Mac (Apple Silicon)

OS:

macOS 14

Bug Depends on:

263773

Bug Blocks:

Attachments:

Description	Flags
HTML with paragraphs that should all display a text-indent	none
Image showing rendered HTML in Safari, Chrome, Firefox	none
Test reduction	none
[fast-cq]Patch	none
Additional test case	none
Patch	none
Patch	none
[fast-cq]Patch	none

Brad Andalman

Reported 2023-10-25 17:23:24 PDT

Created attachment 468341 [details] HTML with paragraphs that should all display a text-indent In the attached HTML, each paragraph should have a text-indent of 1.5em. However, in Safari 17.1 on Sonoma, the paragraph that follows the floating element (seen in lightgray) does not actually indent. Even when you change the floating element’s width to 89%, which pulls the first word of the following paragraph up, there is no text indent. You have to get to a width of 81% before the text-indent has any effect. This is not the case in Chrome and Firefox, where the text-indent always has an effect, regardless of the presence of a floating element immediately before the paragraph with the indent. I can maybe understand why the text-indent doesn’t work for widths between 82-89%: there isn’t enough room for both the first word and the text indent. In that case, though, I’d argue that the word shouldn’t be brought up. In fact, that’s also what Chrome and Firefox do.

Attachments
HTML with paragraphs that should all display a text-indent (1.96 KB, text/html) 2023-10-25 17:23 PDT, Brad Andalman	no flags	Details
Image showing rendered HTML in Safari, Chrome, Firefox (705.68 KB, image/png) 2023-10-25 17:25 PDT, Brad Andalman	no flags	Details
Test reduction (312 bytes, text/html) 2023-10-26 09:44 PDT, alan	no flags	Details
[fast-cq]Patch (30.66 KB, patch) 2023-10-26 11:37 PDT, alan	no flags	Details Formatted Diff Diff
Additional test case (338 bytes, text/html) 2023-10-27 07:24 PDT, Brad Andalman	no flags	Details
Patch (10.00 KB, patch) 2023-11-02 10:44 PDT, alan	no flags	Details Formatted Diff Diff
Patch (10.28 KB, patch) 2023-11-04 09:23 PDT, alan	no flags	Details Formatted Diff Diff
[fast-cq]Patch (10.28 KB, patch) 2023-11-04 15:29 PDT, alan	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Brad Andalman

Comment 1 2023-10-25 17:25:35 PDT

Created attachment 468342 [details] Image showing rendered HTML in Safari, Chrome, Firefox

Radar WebKit Bug Importer

Comment 2 2023-10-26 09:38:34 PDT

<rdar://problem/117538081>

alan

Comment 3 2023-10-26 09:44:26 PDT

Created attachment 468352 [details] Test reduction

alan

Comment 4 2023-10-26 09:44:49 PDT

Thank you for reporting this issue and for the great test case!

alan

Comment 5 2023-10-26 11:37:03 PDT

Created attachment 468354 [details] [fast-cq]Patch

EWS

Comment 6 2023-10-26 14:37:07 PDT

Committed 269827@main (7137ef54ac3f): <https://commits.webkit.org/269827@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 468354 [details].

WebKit Commit Bot

Comment 7 2023-10-27 05:15:46 PDT

Re-opened since this is blocked by bug 263773

Brad Andalman

Comment 8 2023-10-27 07:24:17 PDT

Created attachment 468368 [details] Additional test case As long as this bug was reopened, I wanted to add an additional test case. Some background: I originally discovered this indentation bug in our application, which exhibits this problem with text indentation on Sonoma only. When coming up with a smaller test case, I discovered the test case I reported, which fails on all macOS versions I tried. However, simply defining a column-width changes the behavior to match what I see in our application. Specifically, this new test case succeeds when you render it in Safari 16 (and earlier), but fails in Safari 17 (i.e. on Sonoma). I’m hoping it’s the same underlying bug, so I’m attaching this new test case here. Please let me know if I should split it out into a new bug instead.

alan

Comment 9 2023-10-27 07:33:44 PDT

(In reply to Brad Andalman from comment #8) > Created attachment 468368 [details] > Additional test case > > As long as this bug was reopened, I wanted to add an additional test case. > > Some background: I originally discovered this indentation bug in our > application, which exhibits this problem with text indentation on Sonoma > only. When coming up with a smaller test case, I discovered the test case I > reported, which fails on all macOS versions I tried. > > However, simply defining a column-width changes the behavior to match what I > see in our application. Specifically, this new test case succeeds when you > render it in Safari 16 (and earlier), but fails in Safari 17 (i.e. on > Sonoma). > > I’m hoping it’s the same underlying bug, so I’m attaching this new test case > here. Please let me know if I should split it out into a new bug instead. Thank you! This additional test case seems to be working on my local build (with the fix applied). I am going to re-test it once I resolved the perf regression but I encourage you to do the same once this re-lands on whatever type of build you have access to (local, STP, platform seed etc). Thank you again!

Brad Andalman

Comment 10 2023-10-27 07:40:42 PDT

That’s great news! Thanks, Zalan. I am new to this, so I don’t know the best way to test. I figured I could download the appropriate build from here: https://webkit.org/build-archives/#mac-sonoma-x86_64%20arm64 But when I downloaded the build for 269827@main and ran run-webkit-archive, I didn’t see the fix when I loaded the test case in the Safari that launched. Do you have any advice for the best way to test? (Apologies for using this bug for this info…)

alan

Comment 11 2023-10-27 07:59:01 PDT

(In reply to Brad Andalman from comment #10) > That’s great news! Thanks, Zalan. > > I am new to this, so I don’t know the best way to test. I figured I could > download the appropriate build from here: > https://webkit.org/build-archives/#mac-sonoma-x86_64%20arm64 > > But when I downloaded the build for 269827@main and ran run-webkit-archive, > I didn’t see the fix when I loaded the test case in the Safari that > launched. Do you have any advice for the best way to test? (Apologies for > using this bug for this info…) I tried run-webkit-archive and it failed to load the correct frameworks for Safari. Not sure what's going on here, maybe @ap has some insights?

Alexey Proskuryakov

Comment 12 2023-10-28 15:48:51 PDT

I cannot reproduce this on a freshly installed macOS 14 (23A344) with SIP enabled, WebContent loads with downloaded frameworks as expected. Brad, could you please file a bug at https://feedbackassistant.apple.com? - Reproduce the issue, and note the time. - File a report with a sysdiagnose attached, and with your steps to reproduce. - Post the feedback number here. Alan, since your situation may be different, please file a separate report, or I could take a look at your machine in person.

Brad Andalman

Comment 13 2023-10-31 08:57:46 PDT

I’ve filed a bug using Feedback Assistant (FB13318964) as suggested in comment #12

Alexey Proskuryakov

Comment 14 2023-10-31 14:20:43 PDT

Please do attach a sysdiagnose to FB13318964 (probably with a new timestamp, as original logs may be unavailable if you take the sysdiagnose now).

Brad Andalman

Comment 15 2023-10-31 14:33:10 PDT

Sorry - I thought that was automatic. I’ve attached a sysdiagnose to that feedback.

Alexey Proskuryakov

Comment 16 2023-10-31 17:33:29 PDT

I figured out why I couldn't reproduce. Will look into what can be done to make the archives easily usable, and in the meanwhile, I think that disabling SIP should help (https://developer.apple.com/documentation/security/disabling_and_enabling_system_integrity_protection). This is probably enough of a digression, and the discussion can be turned back to the original layout issue. Please feel free to reach out to me in Slack or email if you'd like to discuss the problem with build archives.

alan

Comment 17 2023-11-02 10:44:20 PDT

Created attachment 468461 [details] Patch

alan

Comment 18 2023-11-02 10:45:18 PDT

landing is pending on A/B perf test result.

alan

Comment 19 2023-11-02 10:46:16 PDT

(In reply to zalan from comment #18) > landing is pending on A/B perf test result. (which was supposedly fixed by https://commits.webkit.org/270075@main)

alan

Comment 20 2023-11-04 09:23:25 PDT

Created attachment 468485 [details] Patch

alan

Comment 21 2023-11-04 15:29:31 PDT

Created attachment 468489 [details] [fast-cq]Patch

EWS

Comment 22 2023-11-04 19:33:18 PDT

Committed 270238@main (e918cedab35b): <https://commits.webkit.org/270238@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 468489 [details].

Note You need to log in before you can comment on or make changes to this bug.