1496833 - Add a reftest that depends on ancestor intrinsics being cleared on bsize changes.

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

It was added to handle the flex container frame clearing up its cached state on the flex item. But that should no longer be needed since recent changes by dholbert made us not clear the whole set of cached reflows, but just the particular flex child item whose intrinsic size changed.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Add a reftest that depends on ancestor intrinsics being cleared on bsize changes.

Bug 1490890 made the flex container not responsible for clearing the
cached reflows, and further fixes to the cached stuff have landed, so this
should be possible now.

Comment 2

[Daniel Holbert [:dholbert]]

Hmm... so suppose we have the following DOM:

 <div style="display:flex; flex-direction:column">
   <div id="flexItem" style="height:auto">
     <div="myHeightChanges">...</div>
   </div>
 </div>

...and suppose #myHeightChanges undergoes a height change.  Will we still clear the cached measurement for #flexItem, after your patch here?

In current mozilla-central, I imagine we do clear the cached measurement, because the height change (on the innermost div) will trigger nsChangeHint_ClearAncestorIntrinsic, which makes us call nsFrame::MarkIntrinsicISizesDirty up the parent chain, which makes #flexItem delete its cached size.

But with the attached patch, it seems like that won't happen anymore, so I don't immediately see how that measurement would get purged...

(I might be forgetting a piece of how this works, though.)

Comment 3

[Emilio Cobos Álvarez (:emilio)]

We use the computed size as part of the cache key right? I think that should avoid us using the cached reflow in that case.

Comment 4

[Daniel Holbert [:dholbert]]

In the scenario that I laid out, the ReflowInput::ComputedBSize() would be nscoord_MAX.

(The actual measured bsize -- based on the child's size -- is the thing we're trying to measure + cache.)

Comment 5

[Daniel Holbert [:dholbert]]

(in other words: the BSize that's part of the cache key would be nscoord_MAX, and the bsize that undergoes a change would be the cache *value*, but we only find out that it's changed if we purge the cache, e.g. due to noticing that one of our descendants underwent a bsize change via this nsChangeHint.)

Comment 6

[Emilio Cobos Álvarez (:emilio)]

You're right! I repurposed this to add a reftest instead.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f34806131069
Add a reftest that depends on ancestor intrinsics being cleared on bsize changes. r=dholbert

Comment 8

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

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

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/f34806131069

Comment 10

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

Upstream PR merged

Comment 11

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

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/13439
* Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/KxESzcxbQ2m7jgWSLE7LFw)