Details

Reviewers

Commits

rMOZILLACENTRALb8a405d2e140: Bug 1731541 - Implement text-wrap: balance for nsBlockFrame reflow. r=emilio
rMOZILLACENTRALeb0b125d3214: Bug 1731541 - Implement text-wrap: balance for nsBlockFrame reflow. r=emilio
rMOZILLACENTRAL0fd4431f5279: Bug 1731541 - Implement text-wrap: balance for nsBlockFrame reflow. r=emilio

Bugzilla Bug ID

1731541

Summary

A simple form of balance for short blocks, implemented by incrementally reducing
the effective inline-size used during line-breaking, up to the point where an
extra line would be created.

This fails the test text-wrap-balance-line-clamp-001.html, but it's unclear to me
if that test is correct (see https://github.com/w3c/csswg-drafts/issues/9310).
If we do want the behavior expected by that test, an additional patch to handle
the interaction with line-clamp will be required.

Depends on D187543

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Build Status

Buildable 582660
Build 680605: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Herald added a project: secure-revision. · View Herald TranscriptSep 6 2023, 10:29 AM

Harbormaster completed remote builds in B574718: Diff 760543.Sep 6 2023, 10:29 AM

phab-bot published this revision for review.Sep 6 2023, 10:29 AM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

Herald added subscribers: zrhoffman, emilio. · View Herald TranscriptSep 6 2023, 10:29 AM

It seems tests are clearly lacking here, as anything with a float would have revealed the destructor issue right? Can we make sure various edge cases are tested?

layout/generic/nsBlockFrame.cpp
1850	This leaks (at best), as `BlockReflowState` has float arrays and what not. You need to destroy it first using `state.~BlockReflowState()`. But that's actually not enough because that would lose all the state that has been modified until now like `ReflowPushedFloats` etc, right? Also, what about the `OverflowTracker` etc?
layout/generic/nsLineLayout.cpp
199	This kinda feels like a text-indent right? I wonder if we could reuse more of the text-indent code somehow. But no big deal if not I guess.
modules/libpref/init/StaticPrefList.yaml
8783	How does this compare with Blink's / WebKit's limit out of curiosity?

This revision now requires changes to proceed.Sep 6 2023, 10:53 AM

Code analysis found 3 defects in diff 760543:

3 defects found by clang-tidy

WARNING: Found 3 defects (warning level) that can be dismissed.

You can run this analysis locally with:

./mach static-analysis check --outgoing (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 760543.

In D187544#6204257, @emilio wrote:

It seems tests are clearly lacking here, as anything with a float would have revealed the destructor issue right? Can we make sure various edge cases are tested?

Yeah, there are only a few very simple tests at the moment, we should add some more coverage.

layout/generic/nsBlockFrame.cpp
1850	Yeah, clearly this is too simplistic; will look into it.
layout/generic/nsLineLayout.cpp
199	Yes and no.... it's like an indent in that it'll affect how much more content can go on the line, but unlike an indent, it doesn't affect the space within which inline-alignment will happen. Because of that difference, I don't think we can merge them.
modules/libpref/init/StaticPrefList.yaml
8783	I'm seeing a limit of 4 in current Chrome release, increasing to 6 in Canary. The current WebKit PR at https://github.com/WebKit/WebKit/pull/16502 doesn't impose a limit, but that's an unreviewed PR so we'll see what happens...

Updated to work with floats (example: https://codepen.io/jfkthame/pen/MWZJZOE). I'll put together some WPT reftests in a following patch.

Harbormaster completed remote builds in B576007: Diff 762346.Sep 11 2023, 4:30 PM

Code analysis found 5 defects in diff 762346:

5 defects found by clang-tidy

3 defects unresolved compared to the previous diff 760543.

WARNING: Found 5 defects (warning level) that can be dismissed.

You can run this analysis locally with:

./mach static-analysis check --outgoing (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 762346.

A couple minor fixes incoming...

jfkthame requested review of this revision.Sep 13 2023, 12:45 PM

jfkthame updated this revision to Diff 763033.

Harbormaster completed remote builds in B576488: Diff 763033.Sep 13 2023, 12:45 PM

jfkthame planned changes to this revision.Sep 13 2023, 2:16 PM

jfkthame requested review of this revision.Sep 13 2023, 6:12 PM

jfkthame updated this revision to Diff 763225.

Harbormaster completed remote builds in B576627: Diff 763225.Sep 13 2023, 6:12 PM

jfkthame updated this revision to Diff 763332.Sep 13 2023, 10:08 PM

jfkthame edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B576707: Diff 763332.Sep 13 2023, 10:08 PM

jfkthame added a child revision: D188139: Bug 1731541 - Add a few WPT reftests using text-wrap:balance together with floats. r=#layout-reviewers.Sep 13 2023, 10:09 PM

Looks good with those comments fixed and the mutable reflowinput stuff cleaned up. I'll try to send a patch later today for that.

layout/generic/nsBlockFrame.cpp
1432	This is fine, but we should probably just use a single `ReflowInput` thing. Given some of this is D181857, what about just fixing that patch? I think we might find pagination regressions otherwise.
1968	Presumably it's intentional that we don't `PopState` here, right? Maybe worth a comment.
layout/generic/nsBlockFrame.h
542	I was very confused about this. The comment seems to be for `TrialReflow`, right? Also it seems to reference `aInset` which is not a thing. Mind fixing the doc?

This revision now requires changes to proceed.Sep 22 2023, 2:00 PM

jfkthame marked 6 inline comments as done.Sep 23 2023, 10:31 AM

jfkthame added inline comments.

layout/generic/nsBlockFrame.h
542	Yeah, sorry - the comment dates from a much earlier version, before the state record existed - will fix!

Rebase, update comments

Harbormaster completed remote builds in B580299: Diff 767141.Sep 23 2023, 10:40 AM

jfkthame updated this revision to Diff 767143.Sep 23 2023, 10:58 AM

jfkthame edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B580301: Diff 767143.Sep 23 2023, 10:58 AM

Code analysis found 5 defects in diff 767143:

5 defects found by clang-tidy

WARNING: Found 5 defects (warning level) that can be dismissed.

You can run this analysis locally with:

./mach static-analysis check --outgoing (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 767143.

emilio accepted this revision as: emilio.Sep 26 2023, 2:13 PM

emilio removed a reviewer: layout-reviewers.

emilio added inline comments.

layout/generic/nsBlockFrame.cpp
1531	clang-tidy is right: Just make the pref signed, or make `countLinesUpTo` work in terms of unsigned?
1811	Hmmm... Do you know why this is? Shouldn't it be enough to mark the first line as dirty and let the rest flow through?

This revision is now accepted and ready to land.Sep 26 2023, 2:13 PM

jfkthame added inline comments.Sep 27 2023, 4:54 PM

layout/generic/nsBlockFrame.cpp
1811	When we adjust the amount of inset being applied, this might not cause any change to the first line, so then we'd conclude that no more reflowing is needed. But there may be lines later in the block that do need to be affected by the changed inset.

jfkthame updated this revision to Diff 768777.Sep 27 2023, 7:25 PM

Harbormaster completed remote builds in B581639: Diff 768777.Sep 27 2023, 7:25 PM

Code analysis found 2 defects in diff 768777:

2 defects found by clang-tidy

2 defects unresolved and 3 defects closed compared to the previous diff 767143.

WARNING: Found 2 defects (warning level) that can be dismissed.

You can run this analysis locally with:

./mach static-analysis check --outgoing (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 768777.

jfkthame marked an inline comment as done.Sep 28 2023, 8:17 AM

Closed by commit rMOZILLACENTRAL0fd4431f5279: Bug 1731541 - Implement text-wrap: balance for nsBlockFrame reflow. r=emilio (authored by jfkthame). · Explain WhySep 28 2023, 8:38 AM

This revision was automatically updated to reflect the committed changes.

jfkthame added a commit: rMOZILLACENTRAL0fd4431f5279: Bug 1731541 - Implement text-wrap: balance for nsBlockFrame reflow. r=emilio.

• nataliaCs reopened this revision.Sep 28 2023, 9:13 AM

This revision is now accepted and ready to land.Sep 28 2023, 9:13 AM

• nataliaCs added a reverting change: rMOZILLACENTRAL2f46b4983d1d: Backed out 4 changesets (bug 1731541) for causing bustage on nsBlockFrame.cpp..Sep 28 2023, 9:13 AM

Rebase

Harbormaster completed remote builds in B581980: Diff 769154.Sep 28 2023, 3:19 PM

Code analysis found 2 defects in diff 769154:

2 defects found by clang-tidy

WARNING: Found 2 defects (warning level) that can be dismissed.

You can run this analysis locally with:

./mach static-analysis check --outgoing (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 769154.

Rebase

Harbormaster completed remote builds in B582032: Diff 769234.Sep 28 2023, 5:19 PM

Closed by commit rMOZILLACENTRALeb0b125d3214: Bug 1731541 - Implement text-wrap: balance for nsBlockFrame reflow. r=emilio (authored by jfkthame). · Explain WhySep 28 2023, 5:34 PM

This revision was automatically updated to reflect the committed changes.

jfkthame added a commit: rMOZILLACENTRALeb0b125d3214: Bug 1731541 - Implement text-wrap: balance for nsBlockFrame reflow. r=emilio.

smolnar reopened this revision.Sep 28 2023, 8:52 PM

This revision is now accepted and ready to land.Sep 28 2023, 8:52 PM

smolnar added a reverting change: rMOZILLACENTRAL2c0444cfb7d6: Backed out 5 changesets (bug 1731541) for causing multiple wpt failures. CLOSED….Sep 28 2023, 8:52 PM

jfkthame updated this revision to Diff 769511.Sep 29 2023, 8:02 AM

Harbormaster completed remote builds in B582257: Diff 769511.Sep 29 2023, 8:02 AM

Code analysis found 2 defects in diff 769511:

2 defects found by clang-tidy

WARNING: Found 2 defects (warning level) that can be dismissed.

You can run this analysis locally with:

./mach static-analysis check --outgoing (C/C++)

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 769511.

Closed by commit rMOZILLACENTRALb8a405d2e140: Bug 1731541 - Implement text-wrap: balance for nsBlockFrame reflow. r=emilio (authored by jfkthame). · Explain WhySep 30 2023, 3:59 PM

This revision was automatically updated to reflect the committed changes.

jfkthame added a commit: rMOZILLACENTRALb8a405d2e140: Bug 1731541 - Implement text-wrap: balance for nsBlockFrame reflow. r=emilio.

Harbormaster completed remote builds in B582660: Diff 769900.Nov 21 2023, 4:41 PM

Bug 1731541 - Implement text-wrap: balance for nsBlockFrame reflow. r=#layout-reviewers
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 769900

layout/generic/BlockReflowState.h

layout/generic/BlockReflowState.cpp

layout/generic/nsBlockFrame.h

layout/generic/nsBlockFrame.cpp

layout/generic/nsLineLayout.h

layout/generic/nsLineLayout.cpp

modules/libpref/init/StaticPrefList.yaml

testing/web-platform/meta/css/css-text/parsing/text-wrap-invalid.html.ini

testing/web-platform/meta/css/css-text/parsing/text-wrap-valid.html.ini

testing/web-platform/meta/css/css-text/parsing/white-space-shorthand-text-wrap.html.ini

testing/web-platform/meta/css/css-text/white-space/text-wrap-balance-002.html.ini

testing/web-platform/meta/css/css-text/white-space/text-wrap-balance-line-clamp-001.html.ini

testing/web-platform/meta/css/css-text/white-space/text-wrap-balance-text-indent-001.html.ini

testing/web-platform/meta/web-animations/animation-model/animation-types/accumulation-per-property-002.html.ini

testing/web-platform/meta/web-animations/animation-model/animation-types/addition-per-property-002.html.ini

testing/web-platform/meta/web-animations/animation-model/animation-types/interpolation-per-property-002.html.ini

Bug 1731541 - Implement text-wrap: balance for nsBlockFrame reflow. r=#layout-reviewersClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 769900

layout/generic/BlockReflowState.h

layout/generic/BlockReflowState.cpp

layout/generic/nsBlockFrame.h

layout/generic/nsBlockFrame.cpp

layout/generic/nsLineLayout.h

layout/generic/nsLineLayout.cpp

modules/libpref/init/StaticPrefList.yaml

testing/web-platform/meta/css/css-text/parsing/text-wrap-invalid.html.ini

testing/web-platform/meta/css/css-text/parsing/text-wrap-valid.html.ini

testing/web-platform/meta/css/css-text/parsing/white-space-shorthand-text-wrap.html.ini

testing/web-platform/meta/css/css-text/white-space/text-wrap-balance-002.html.ini

testing/web-platform/meta/css/css-text/white-space/text-wrap-balance-line-clamp-001.html.ini

testing/web-platform/meta/css/css-text/white-space/text-wrap-balance-text-indent-001.html.ini

testing/web-platform/meta/web-animations/animation-model/animation-types/accumulation-per-property-002.html.ini

testing/web-platform/meta/web-animations/animation-model/animation-types/addition-per-property-002.html.ini

testing/web-platform/meta/web-animations/animation-model/animation-types/interpolation-per-property-002.html.ini

Bug 1731541 - Implement text-wrap: balance for nsBlockFrame reflow. r=#layout-reviewers
ClosedPublic
Actions

Revision Contents
Changeset List