layout/base/nsCSSFrameConstructor.cpp
1555 ↗	(On Diff #787670)	Hmm... Are you sure this is how other browsers behave? I don't think it is: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/pseudo_element.cc;l=308;drc=42f0b9c4f60f67676d9096b3e94df9fd47f568f4 Do we behave the same if we just have an empty inline with an empty text-node? You can create it with JS quite easily...
testing/web-platform/tests/css/css-pseudo/before-after-empty-content-ref.html
15 ↗	(On Diff #787670)	`content` does nothing here, so maybe remove for clarity?

This revision now requires changes to proceed.Nov 16 2023, 3:35 PM

Ok, so I think I changed my mind about this, but the spec isn't super-clear. This seems sensible and should be reasonably close to what other browsers do.

I filed https://github.com/w3c/csswg-drafts/issues/9606 on this. Mind pointing to that issue in the test-case?

Also, probably wait until after the soft-freeze. It is probably also worth looking into making empty text-nodes not export a baseline more generally. I think your test-case should ideally behave the same for content: "" and content: attr(non-existent). Mind filing a follow-up for that at least?

testing/web-platform/tests/css/css-pseudo/before-after-empty-content.html
7 ↗	(On Diff #787670)	Can you also point to https://github.com/w3c/csswg-drafts/issues/9606?

This revision is now accepted and ready to land.Nov 16 2023, 4:02 PM

dshin updated this revision to Diff 788654.Nov 20 2023, 3:42 PM

dshin retitled this revision from Bug 1855583: Do not generate text run for empty pseudoelement content. r=#layout-reviewers to Bug 1855583: Empty text line should not give line height even with `white-space: pre`. r=#layout-reviewers.

dshin marked 2 inline comments as done.

Harbormaster completed remote builds in B598359: Diff 788654.Nov 20 2023, 3:42 PM

dshin added inline comments.Nov 20 2023, 3:42 PM

testing/web-platform/tests/css/css-pseudo/before-after-empty-content-ref.html
15 ↗	(On Diff #787670)	That's actually not true, at least for `::before` & `::after`: "When their computed content value is not none, these pseudo-elements generate boxes [...]"

emilio added inline comments.Nov 20 2023, 3:45 PM

testing/web-platform/tests/css/css-pseudo/before-after-empty-content-ref.html
15 ↗	(On Diff #787670)	Right, my point is that in this rule there are no pseudo-elements, right?

This is fine, but we should probably also do your original frame constructor patch while at it. content: "" is common enough to be worth optimizing some frames around.

dshin added inline comments.Nov 20 2023, 3:51 PM

testing/web-platform/tests/css/css-pseudo/before-after-empty-content-ref.html
15 ↗	(On Diff #787670)	facepalm right.

In D193807#6431592, @emilio wrote:

This is fine, but we should probably also do your original frame constructor patch while at it. content: "" is common enough to be worth optimizing some frames around.

I'll do that as part of bug 1865668

rebase, fix double space in comment

Harbormaster completed remote builds in B598362: Diff 788658.Nov 20 2023, 3:56 PM

Closed by commit rMOZILLACENTRALf78453d75438: Bug 1855583: Empty text line should not give line height even with `white-space… (authored by dshin). · Explain WhyNov 20 2023, 4:17 PM

This revision was automatically updated to reflect the committed changes.

dshin added a commit: rMOZILLACENTRALf78453d75438: Bug 1855583: Empty text line should not give line height even with `white-space….

• NarcisB reopened this revision.Nov 20 2023, 8:51 PM

This revision is now accepted and ready to land.Nov 20 2023, 8:51 PM

• NarcisB added a reverting change: rMOZILLACENTRALb24ebcca491e: Backed out changeset f78453d75438 (bug 1855583) for wpt failure on empty-text….Nov 20 2023, 8:51 PM

dshin updated this revision to Diff 788973.Nov 20 2023, 11:24 PM

Harbormaster completed remote builds in B598617: Diff 788973.Nov 20 2023, 11:24 PM

Closed by commit rMOZILLACENTRAL8272ad0d7c2d: Bug 1855583: Empty text line should not give line height even with `white-space… (authored by dshin). · Explain WhyNov 20 2023, 11:37 PM