Bug 239689 - Setting white-space to non-default value dynamically on a blank (whitespace/new line) does not work correctly
Summary: Setting white-space to non-default value dynamically on a blank (whitespace/n...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: Safari 15
Hardware: All All
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-04-23 00:57 PDT by Chijin
Modified: 2024-03-18 10:00 PDT (History)
12 users (show)

See Also:


Attachments
expected rendering result of chrome (189.66 KB, image/png)
2022-04-23 00:57 PDT, Chijin
no flags Details
Test reduction (210 bytes, text/html)
2022-04-24 20:01 PDT, zalan
no flags Details
Patch (3.36 KB, patch)
2024-03-17 13:55 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (3.75 KB, patch)
2024-03-17 20:28 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (4.69 KB, patch)
2024-03-18 05:32 PDT, zalan
no flags Details | Formatted Diff | Diff
[fast-cq]Patch (4.82 KB, patch)
2024-03-18 08:04 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chijin 2022-04-23 00:57:01 PDT
Created attachment 458204 [details]
expected rendering result of chrome

example url: https://codepen.io/chijinz/pen/eYyoLbz

Steps to reproduce the problem:

1. use Safari to visit the example url
2. the green light is on the top of the page even though white-space has been set to "pre-line". This is inconsistent with the document (https://developer.mozilla.org/en-US/docs/Web/CSS/white-space). Other browsers (e.g. chrome) works fine with this issue.
Comment 1 zalan 2022-04-24 20:01:17 PDT
Created attachment 458239 [details]
Test reduction

This bug only occurs when the pre-line is set dynamically on an "inline empty" content (no text or other inline level box).
Comment 2 zalan 2022-04-24 21:16:49 PDT
We don't create RenderText renderers for the whitespace/new line only content.

It looks like in RenderTreeUpdater::updateRenderTree
bool mayNeedUpdateWhitespaceOnlyRenderer = renderingParent().didCreateOrDestroyChildRenderer && text.data().isAllSpecialCharacters<isHTMLSpace>();
may need to be updated so that certain parent styles (e.g. white-space) are also taken into account when checking if the text node needs a renderer.
Comment 3 Radar WebKit Bug Importer 2022-04-30 00:57:14 PDT
<rdar://problem/92559818>
Comment 4 Chijin 2022-05-02 19:50:46 PDT
Hi, anyone take care of this issue?
Comment 5 Ahmad Saleem 2022-12-31 04:16:50 PST
I am still able to reproduce this in Safari 16.2 & STP160, is this something progressed with IFC on trunk? @Alan?
Comment 6 zalan 2022-12-31 07:16:20 PST
(In reply to zalan from comment #2)
> We don't create RenderText renderers for the whitespace/new line only
> content.
> 
> It looks like in RenderTreeUpdater::updateRenderTree
> bool mayNeedUpdateWhitespaceOnlyRenderer =
> renderingParent().didCreateOrDestroyChildRenderer &&
> text.data().isAllSpecialCharacters<isHTMLSpace>();
> may need to be updated so that certain parent styles (e.g. white-space) are
> also taken into account when checking if the text node needs a renderer.
Let's see what Antti thinks.
Comment 7 Chijin 2024-03-17 07:36:03 PDT
Hi zalan, one year has been passed, any update for this bug?
Comment 8 zalan 2024-03-17 13:55:51 PDT
Created attachment 470405 [details]
Patch
Comment 9 zalan 2024-03-17 13:56:40 PDT
(In reply to Chijin from comment #7)
> Hi zalan, one year has been passed, any update for this bug?
This is not really my area, but let's see if I got this right.
Comment 10 zalan 2024-03-17 20:28:37 PDT
Created attachment 470413 [details]
Patch
Comment 11 Chijin 2024-03-18 00:08:06 PDT
Thanks for the fixing!
Comment 12 zalan 2024-03-18 05:32:18 PDT
Created attachment 470415 [details]
Patch
Comment 13 Antti Koivisto 2024-03-18 07:15:52 PDT
Comment on attachment 470415 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=470415&action=review

> Source/WebCore/style/StyleTreeResolver.cpp:975
> +                if (!node.renderer() && containsOnlyASCIIWhitespace && parent.style.preserveNewline())
> +                    return true;

This could test !text->renderer() for consistency.
This really needs to be done only when parent.style.preserveNewline() changes value. We could set a bit in the parent struct when that happens and test for that.
Comment 14 zalan 2024-03-18 08:04:41 PDT
Created attachment 470417 [details]
[fast-cq]Patch
Comment 15 EWS 2024-03-18 09:45:36 PDT
Committed 276277@main (40f3c61c1d9d): <https://commits.webkit.org/276277@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 470417 [details].
Comment 16 zalan 2024-03-18 10:00:58 PDT
(In reply to Chijin from comment #11)
> Thanks for the fixing!
Thank you for filing it!