1504877 - Cache more than one BSize measurement for flex items

Status: ASSIGNED

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

Description

[Daniel Holbert [:dholbert]]

In bug 1376536, we've got a case where our flex item BSize measurement caching is failing because we oscillate between two different sets of inputs (one without a vertical scrollbar, and one with a vertical scrollbar) -- and for the flex item in question, we always "miss" our cached-value validity check when we see if we've got a usable cached measurement.

I've got some other ideas for how we can avoid the oscillation (see bug 1376536 comment 20), but for robustness, I think we should also just allow ourselves to cache two measurements rather than one.  This will let us gracefully handle cases where we're e.g. toggling a scrollbar on vs. off (or, somewhat-similarly, cases when we're jumping between a slightly different "measuring" vs. "final" reflow in an ancestor flex container).

There's an argument to be made for "why stop at 2".  For now, I suspect two values will be sufficient in most cases without using up too much memory, and it's an easy extension of our current implementation.  But if it turns out we need more for some extreme cases, we might want to use a more generalized cache, to allow a few flex items to cache multiple values while still only allocating memory for <1 value on most flex items.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1504877: Allow flex items to cache a second BSize measurement, if their first cached measurement isn't suitable.

Comment 2

[Daniel Holbert [:dholbert]]

Unfortunately, this doesn't help in bug 1376536 very much after all.

Really, we want to avoid reflowing the backscroll area *at all*.   And this patch does help us avoid doing a "measuring reflow" of that area -- but in bug 1376536's testcase (and on WhatsApp itself), we still do a "final reflow" of that area, which is still expensive, because we are still trying to reflow it in two different configurations (with & without the vertical scrollbar).

(The reason we reflow it in those two different configurations is in service of a measuring reflow on a more-distant flex-item ancestor, which contains both the text-entry area and the backscroll.  Unfortunately, the text entry mutation causes us to call MarkIntrinsicISizesDirty() all the way up the ancestor chain, and that causes cached measurement to be purged on this particular ancestor flex item, which means we end up having to measure its auto-height again. And its auto-height happens to be 0, since its sole child is abspos, so we end up inadvertently reflowing that abspos child with 0 height as well, which is why we end up struggling with the scrollbar being on vs. off as discussed in bug 1503660.)

Comment 3

[Daniel Holbert [:dholbert]]

So the place where we purge the cached value as we walk up the tree is this bit of PresShell::FrameNeedsReflow() (which gets called for the text frame when it changes, in nsTextFrame::CharacterDataChanged):

    if (aIntrinsicDirty != nsIPresShell::eResize) {
      // Mark argument and all ancestors dirty. (Unless we hit a reflow
      // root that should contain the reflow.  That root could be
      // subtreeRoot itself if it's not dirty, or it could be some
      // ancestor of subtreeRoot.)
      for (nsIFrame *a = subtreeRoot;
           a && !FRAME_IS_REFLOW_ROOT(a);
           a = a->GetParent())
        a->MarkIntrinsicISizesDirty();
    }

In this case, one of the frames in the chain is abspos. I wonder if it actually makes sense for us to be calling  MarkIntrinsicISizesDirty() up beyond that abspos frame?  If we could skip that, I think it'd help here.