1464761 - Remove nsReflowStatus::mTruncated

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Cameron McCormack (:heycam)]

This macro is simple enough that we should just expand its uses and remove it.

Comment 1

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

I had tried to remove NS_FRAME_SET_TRUNCATION in bug 775624, but we end up keep it as a layer of abstraction until a better name comes up (See bug 775624 comment 100 and bug 775624 comment 106).

Comment 2

[Cameron McCormack (:heycam)]

OK, good to know!  (I didn't look into what it is used for so I don't have an opinion on a better name.)

Comment 3

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

I'd like to morph this bug into completely remove nsReflowStatus::mTruncated bit and its getter and setter [1] for the following reasons:

1. The purpose of this flag is for a non-top-of-page child frame to report that it cannot fit in the remaining available block-size [2], but only float [3] and table row frame [4] ever look at this bit. The callers can check the above condition themselves, and no need for every frame types to call NS_FRAME_SET_TRUNCATION at the end of Reflow().
2. The code checking orthogonal reflow in nsReflowStatus::UpdateTruncated [5] is not 100% correct because ReflowOutput might be initialized in child frame's writing-mode according to the document [6], and the ReflowOutput for flex item is an example [7].

[1] https://searchfox.org/mozilla-central/rev/1ce190047b9556c3c10ab4de70a0e61d893e2954/layout/generic/nsIFrame.h#276-283
[2] https://searchfox.org/mozilla-central/rev/1ce190047b9556c3c10ab4de70a0e61d893e2954/layout/generic/nsIFrame.cpp#257-260
[3] https://searchfox.org/mozilla-central/rev/1ce190047b9556c3c10ab4de70a0e61d893e2954/layout/generic/BlockReflowState.cpp#869
[4] https://searchfox.org/mozilla-central/rev/1ce190047b9556c3c10ab4de70a0e61d893e2954/layout/tables/nsTableRowFrame.cpp#1099
[5] https://searchfox.org/mozilla-central/rev/1ce190047b9556c3c10ab4de70a0e61d893e2954/layout/generic/nsIFrame.cpp#253-256
[6] https://searchfox.org/mozilla-central/rev/1ce190047b9556c3c10ab4de70a0e61d893e2954/layout/generic/ReflowOutput.h#171-175
[7] https://searchfox.org/mozilla-central/rev/1ce190047b9556c3c10ab4de70a0e61d893e2954/layout/generic/nsFlexContainerFrame.cpp#5692

Comment 4

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

Attachment: Bug 1464761 Part 1 - Remove IsTruncated() in FlowAndPlaceFloat(). r?jfkthame,dholbert

The complicated if-statement is checking the float with the
following conditions:

1. It is reflowed under a constrained available block-size.
2. Its position is not at the top of the page.
3. It has break-inside:avoid style.
4. Its reflow status is not fully complete or its block-size exceeds
   the available block-size.
5. It is a first-in-flow.

The condition 1, 2, and 4 are equivalent to reflowStatus.IsTruncated() in the
previous if-statement. Also, assuming the float frame implements break-avoid, it
should report break-before if ShouldAvoidBreakInside() is true.
FlowAndPlaceFloat() has already handled this via IsInlineBreakBefore() check,
which covers condition 3 and 5.

The code coverage confirms that the branch in never reached.
https://coverage.moz.tools/#revision=e6e2286d2ac25001127a1cf54a87a95fb435c734&path=layout%2Fgeneric%2FBlockReflowState.cpp&view=file&line=877

But, we are going to remove IsTruncated() bit in a later patch, so instead of
removing the unused if-statement, we use part of it to replace the
IsTruncated().

Depends on D151457

Comment 5

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

Attachment: Bug 1464761 Part 2 - Remove IsTruncated() in nsTableRowFrame::ReflowCellFrame(). r?jfkthame,dholbert

We call NS_FRAME_SET_TRUNCATION (via nsReflowStatus::UpdateTruncated) in the end
of every frame's Reflow() to update nsReflowStatus::mTruncated bit. In the
following patches, I'm going to rewrite all the callers of IsTruncated(), and
ultimately remove the mTruncated bit.

In this patch, I rewrite the callsite in ReflowCellFrame() by moving
nsReflowStatus::UpdateTruncated logic [1] into it. Note that we use an assertion
in ReflowCellFrame to make sure nsTableRowFrame and nsTableCellFrame have the
same writing-mode.

The only test covering the code path is layout/reftests/bugs/409084-1a.html.

[1] https://searchfox.org/mozilla-central/rev/2946e9b450cb35afaf8dad927a8d187975dcd74d/layout/generic/nsIFrame.cpp#256-258

Depends on D151458

Comment 6

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

Attachment: Bug 1464761 Part 3 - Remove IsTruncated() in nsColumnSetFrame::ReflowChildren(). r?jfkthame,dholbert

In nsReflowStatus::UpdateTruncated(), mTruncated can be true only when
mIsTopOfPage is false. However, when we reflow -moz-column-content, we always
set mIsTopOfPage to true. Therefore, IsTruncated() always returns false, i.e.
!IsTruncated() is always true.

Depends on D151459

Comment 7

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

Attachment: Bug 1464761 Part 4 - Remove nsReflowStatus::mTruncated bit. r?jfkthame,dholbert

In the description of the mTruncated bit, its purpose is the same as calling
SetInlineLineBreakBeforeAndReset(). We've removed all its usages in previous
patches, so the bit is no longer needed.

Depends on D151460

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: breaktest 1 (where we unintentionally change behavior with current "part 1" patch)

Here's a testcase whose behavior changes (for the worse, I think) with the current version of Part 1 here.

If this isn't a bug that's caught by our existing test suite, then let's be sure we include a version of this as a paged reftest (or a print WPT test) in some patch on this bug, to have automated coverage for this.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: screenshot showing the behavior-change on breaktest 1 (Nightly on left, local build w/ phab patch stack up through part 1 on right)

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: (crufty version of breaktest 2)

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: breaktest 2 (same as 1, but with "break-inside:avoid" on the float)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a344865b3232
Part 1 - Remove IsTruncated() in FlowAndPlaceFloat(). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/b40423a5defb
Part 2 - Remove IsTruncated() in nsTableRowFrame::ReflowCellFrame(). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/e006e49cadba
Part 3 - Remove IsTruncated() in nsColumnSetFrame::ReflowChildren(). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/fedad204cc81
Part 4 - Remove nsReflowStatus::mTruncated bit. r=dholbert

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/35251 for changes under testing/web-platform/tests

Comment 14

[bszekely]

https://hg.mozilla.org/mozilla-central/rev/a344865b3232
https://hg.mozilla.org/mozilla-central/rev/b40423a5defb
https://hg.mozilla.org/mozilla-central/rev/e006e49cadba
https://hg.mozilla.org/mozilla-central/rev/fedad204cc81

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot