1792134 - Investigate if it is possible to remove StyleClear::Line

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, task)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

As far as I can tell, StyleClear::Line [1] is abused by BRFrame to report a inline break-after status when it has clear:none [2]. The status is later carrying to nsLineBox [3].

I'd like to investigate if it is possible to remove StyleClear::Line, and use something else in nsLineBox to represent the "inline break-after but no float clearance" idea. I can imaging by doing so, we can

1. simplify the clear property implementation in style system because StyleClear enum values and the clear property values are the same.
2. make the inline break-before, break-after, and float clearance clearer. Currently, these idea are mix together already, so it's super hard to improve break-before and break-after for block/flex/grid [4].

[1] https://searchfox.org/mozilla-central/rev/534b95e2e21caaea3e5932fd5a99e4961c942f89/layout/style/nsStyleConsts.h#156-163
[2] https://searchfox.org/mozilla-central/rev/534b95e2e21caaea3e5932fd5a99e4961c942f89/layout/generic/BRFrame.cpp#154-160
[3] https://searchfox.org/mozilla-central/rev/534b95e2e21caaea3e5932fd5a99e4961c942f89/layout/generic/nsBlockFrame.cpp#4786-4800
[4] https://drafts.csswg.org/css-break-3/#break-between

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1792134 Part 1 - Clean up around BR clearance handling. r?#layout

This patch shouldn't change the behavior.

• Rename BlockReflowState's mFloatBreakType to mTrailingClearFromPIF since
  it's the cached result of FindTrailingClear() from the block's prev-in-flow.
  Note that mTrailingClearFromPIF can have StyleClear::Line, which is
  not a float clear value. We'll remove StyleClear::Line in a later part to
  make this 100% correct.
• Rename nsLayoutUtils::CombineBreakType to CombineClearType as well as its
  arguments.
• Simplify FindTrailingClear()'s implementation.

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1792134 Part 2 - Simplify two callsites with nsLineBox::HasBreakBefore. r?#layout

Depends on D158221

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1792134 Part 3 - Remove StyleClear::Line. r?#layout

Before this patch, StyleClear::Line indicates that the nsLineBox has break-after
but no float clearance (note we don't allow line break-before with clear:none);
StyleClear::None indicates the line has no break-before nor break-after.

In this patch, I added mHasBreak bit in nsLineBox to indicate the line has a
break so that StyleClear can serve its original meaning -- the float clearance.

Now, instead of using StyleClear::None to clear the line break status, the caller
should use ClearBreak(); Similar to set SetInlineLineBreakBeforeAndReset() and
SetInlineLineBreakAfter on nsReflowStatus, SetBreakTypeBefore/SetBreakTypeAfter
on nsLineBox always sets break status with an optional float clearance
parameter.

This patch shouldn't change the behavior.

Depends on D158222

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1792134 Part 4 - Move clear property out of gecko.mako.rs. r?#layout,#style

Depends on D158224

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1792134 Part 5 - Improve nsLineBox log. r?#layout

• Print StyleClear value verbatim.
• Remove mAllFlags since I doubt anyone can decipher FlagBits from it.

Depends on D158225

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Patches uploaded for review.

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1792134 Part 6 - More rename and clean up related to StyleClear. r?#layout,emilio

• Rename mBreakType to mFloatClearType in nsLineBox and nsReflowStatus and
  the methods around it.
• Rename mBreakType to mFloatClear in nsStyleDisplay.
• Many other method parameters or local variables rename such as from
  aBreakType to aClearType.

Depends on D158226

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4344f8070f22
Part 1 - Clean up around BR clearance handling. r=emilio
https://hg.mozilla.org/integration/autoland/rev/389cb54bc4b9
Part 2 - Simplify two callsites with nsLineBox::HasBreakBefore. r=emilio
https://hg.mozilla.org/integration/autoland/rev/1447d36bc677
Part 3 - Remove StyleClear::Line. r=emilio
https://hg.mozilla.org/integration/autoland/rev/7439d7af7545
Part 4 - Move clear property out of gecko.mako.rs. r=emilio
https://hg.mozilla.org/integration/autoland/rev/3000987ae9a1
Part 5 - Improve nsLineBox log. r=emilio
https://hg.mozilla.org/integration/autoland/rev/cbbc4c7145a9
Part 6 - More rename and clean up related to StyleClear. r=emilio

Comment 9

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/4344f8070f22
https://hg.mozilla.org/mozilla-central/rev/389cb54bc4b9
https://hg.mozilla.org/mozilla-central/rev/1447d36bc677
https://hg.mozilla.org/mozilla-central/rev/7439d7af7545
https://hg.mozilla.org/mozilla-central/rev/3000987ae9a1
https://hg.mozilla.org/mozilla-central/rev/cbbc4c7145a9