1700580 - padding-right calc() does not recalculate when resized

Status: RESOLVED FIXED

(Core :: Layout: Flexbox, defect, P3)

Description

[hqdrone]

Attachment: dist.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:87.0) Gecko/20100101 Firefox/87.0

Steps to reproduce:

I open index.html from dist.zip and I do horizontal resizing of window

Actual results:

Right block in section "About" doesn't recalculate padding-right: calc() on resize. I see horizontal scrollbar or clipped block

Expected results:

Right block in section "About" must recalculate padding-right: calc() on resize

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::CSS Parsing and Computation' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

This was a regression from bug 1492538: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a8b05894377919237c20e33dfe61bd9b103d500f&tochange=1aa53285261e17aa6a8b36e05c7208d616c24aa4

Daniel, can you take a look?

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: testcase 1 (reduced)

Daniel,

Do you recall where this claim is from? In testcase 1, after changing the flex container's width, it seems we don't clear the cache for a flex item with a percentage padding via MarkCachedFlexMeasurementsDirty nor mark the item dirty. The flex item 2's content-box size remains the same, so we just move it to the new position without reflow it.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #3)

Do you recall where this claim is from?

You're talking specifically about the second half, regarding percent sizes, right?

I don't recall where that came from, no. It's possible I misread the usage of nsChangeHint_ClearDescendantIntrinsics here and mistakenly thought it was being included rather than excluded:
https://searchfox.org/mozilla-central/rev/5a362eb7d054740dc9d7c82c79a2efbc5f3e4776/layout/base/nsChangeHint.h#373-380

Or maybe I had some testcase that suggested it worked this way, but which actually worked for an unrelated reason. In any case, looking at that nsChangeHint documentation, it seems that the code-comment you linked to is indeed wrong in its expectation of descendant intrinsic sizes being cleared. :-/

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Thanks for the reply!

You're talking specifically about the second half, regarding percent sizes, right?

Yes, I'm wondering the validity of this sentence:

Also: hypothetically if our padding changed via being a percent value and its percent-basis changing, then the percent-basis change should've cleared descendant intrinsic sizes which would purge cached values via MarkCachedFlexMeasurementsDirty and would make us fail the "if (cache...") check below.)

I'm planning to take a deeper look at the if block that mark the children's inline size dirty.
https://searchfox.org/mozilla-central/rev/5a362eb7d054740dc9d7c82c79a2efbc5f3e4776/layout/base/PresShell.cpp#2776-2813. It's possible that the caller doesn't pass correct aIntrinsicDirty or aBitToAdd so that we skip the if. Otherwise, we may consider adding the used padding to the flex item cache so that we can detect when the padding is changed.

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Otherwise, we may consider adding the used padding to the flex item cache so that we can detect when the padding is changed.

We update nsIFrame::UsedPaddingProperty() when creating a ReflowInput for a frame [1], so it's too late to compare the difference between the used border & padding on the frame and the FlexItem::BorderPadding in FlexItem::NeedsFinalReflow.

So we'll need to cache the padding anyway. The only benefit I can think of to cache border & padding as a standalone variable in nsFlexContainerFrame::CachedFlexItemData is to give us a more granular log about the reason to perform a final reflow. I believe changing mFinalReflowSize [2] to border-box size can capture the potential change of the percentage padding.

Daniel, does the above plan sound good to you?

[1] https://searchfox.org/mozilla-central/rev/55e8eba74b60b92d04b781f7928f54ef76b13fa9/layout/generic/ReflowInput.cpp#2532-2533
[2] https://searchfox.org/mozilla-central/rev/55e8eba74b60b92d04b781f7928f54ef76b13fa9/layout/generic/nsFlexContainerFrame.cpp#1919-1925

Comment 7

[Daniel Holbert [:dholbert]]

Yes, I think it makes sense to use the border-box size for mFinalReflowSize. Thanks!

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1700580 Part 1 - Unify CachedFlexItemData's APIs that update the existing cache.

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1700580 Part 2 - Add a helper class to cache flex item's metrics in its most recent reflow.

This patch introduces a class that caches metrics in flex item's final
reflow. With it, we have a better symmetry for caches, one for measuring
reflow and the other for final reflow. Also, we don't have to explain
that mLastReflowTreatedBSizeAsIndefinite is only meaningful if
mFinalReflowSize is set.

Depends on D124966

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1700580 Part 3 - Rename dynamic-isize-change.html to dynamic-isize-change-001.html.

We'll add more isize change test to wpt in the next part.

Depends on D124967

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1700580 Part 4 - Change the cached size in the final reflow from content-box size to border-box size.

Currently, we assume any flex item with percentage padding will be
marked as dirty if the percentage basis is changed. However, it is not
true.

To fix it, we change the content-box size cache in the flex item's final
reflow cache to border-box to detect any change to the padding.

Depends on D124968

Comment 12

[Daniel Holbert [:dholbert]]

So, I have one concern with the approach here -- sort of theoretical, but I suspect a testcase could be constructed that would demonstrate it...

• Suppose we're doing a reflow where the flex item's content-box size changes, in a way that we currently rely on this cache to detect. Let's say it gets 10px taller.
• Now, suppose it simultaneously loses 10px of vertical padding (due to its padding being percent-valued and its percent basis changing.
• So: its padding-box (and border-box) would remain the same size.

In this scenario, current mozilla-central would detect this and would reflow the flex item to account for the content-box size change. But this patch-stack (part 4 in particular) would prevent us from detecting that the flex item needs a new final reflow; we wouldn't see that its content-box changed size, and we'd skip the final reflow.

To avoid this problem, I suspect we may need to come at this a different way... (EDIT: perhaps more-correctly: we might just need one additional hack...) One option would be to store the padding (or BorderPadding) as its own LogicalMargin member-var in the cached metrics, and compare it / reset it / etc. as part of our regular cache usage.

With that approach, the flex-item-size that we're also storing could still be the content-box size, or it could be the border-box size -- whatever feels more ergonomic, I think. Regardless of which size we store: in my pathological scenario that I described above, we would now be able to detect that the cache was invalid and we need to reflow, because we'd see that the padding (or BorderPadding) has changed.

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #12)

• Suppose we're doing a reflow where the flex item's content-box size changes, in a way that we currently rely on this cache to detect. Let's say it gets 10px taller.
• Now, suppose it simultaneously loses 10px of vertical padding (due to its padding being percent-valued and its percent basis changing.

(er, I'd thought I recalled that this bug's testcase had involved vertical padding -- but in fact it's about horizontal padding. So in this theoretical example, I probably should have said "gets 10px wider" and "loses 10px of horizontal padding". Though I suspect it could be a problem in either axis, so taller/vertical-padding are worth considering as well.)

Comment 14

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Yes, comment 12 makes sense to me. Let me try to craft a testcase.

Comment 15

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1700580 Part 4 - Cache flex item's border and padding used in the final reflow.

Currently, we assume any flex item with percentage padding will be
marked as dirty if the percentage basis is changed. However, it is not
true.

To fix it, we cache the flex item's border and padding used in its most
recent final reflow to detect their changes.

dynamic-isize-change-004.html is designed to catch the concern in bug
1700580 comment 12.

Depends on D124968

Comment 16

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1700580 Part 5 - Mark a frame as IResize if its border-box is not changed but it has percentage padding.

The issue in dynamic-isize-change-005.html is that during the flex
item's incremental reflow, its CachedBAxisMeasurement does being
rejected due to its inline-size changed. However, it is not properly
marked as IResize in ReflowInput::InitResizeFlags(), so the flex
item (block frame) thinks its lines are clean and not bother reflowing
them.

To fix this, we can simply remove the condition that tests the frame's
box-sizing property because it has nothing to do with whether its
content-box size is changed or not. dynamic-isize-change-005.html is an
example where a flex item has the same border-box inline size, but
changes its content-box inline-size due to different flex container's
inline-size.

Depends on D125620

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e34694e8c9b
Part 1 - Unify CachedFlexItemData's APIs that update the existing cache. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/45519af7e8d2
Part 2 - Add a helper class to cache flex item's metrics in its most recent reflow. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c645fe62180e
Part 3 - Rename dynamic-isize-change.html to dynamic-isize-change-001.html. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/3da0171aa06e
Part 4 - Cache flex item's border and padding used in the final reflow. r=dholbert

Comment 18

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/9e34694e8c9b
https://hg.mozilla.org/mozilla-central/rev/45519af7e8d2
https://hg.mozilla.org/mozilla-central/rev/c645fe62180e
https://hg.mozilla.org/mozilla-central/rev/3da0171aa06e

Comment 19

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

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

Comment 20

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

Upstream PR merged by moz-wptsync-bot

Comment 21

[Phabricator Automation]

Comment on attachment 9241249 [details]
Bug 1700580 Part 5 - Mark a frame as IResize if its border-box is not changed but it has percentage padding.

Revision D125621 was moved to bug 1731653. Setting attachment 9241249 [details] to obsolete.