851012 - Investigate the usage of the NS_FRAME_CONTAINS_RELATIVE_HEIGHT bit & resize flags on flex containers

Status: ASSIGNED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Filing this bug as a followup to cover dbaron's question from bug 841827 comment #6:
> But I'm also a little concerned about whether this is setting the bit on the
> right set of frames [...] (Does the parent, given that
> it's the containing block, also need to have the bit set?  And does the
> quirks mode percentage height code require setting it in more cases in
> quirks mode?  I'm comparing to the code that sets this bit in
> nsHTMLReflowState::InitResizeFlags, which is the primary case for setting
> this bit.)  Would you mind looking into that?

Comment 1

[Daniel Holbert [:dholbert]]

So, to recap on what bug 841827 did:
 - It makes us set NS_FRAME_CONTAINS_RELATIVE_HEIGHT on flex container frames that are absolutely positioned, height:auto, and have both "top" and "bottom" set.

The documentation for NS_FRAME_CONTAINS_RELATIVE_HEIGHT says:
> 149 // If this bit is set, this frame or one of its descendants has a
> 150 // percentage height that depends on an ancestor of this frame.
https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIFrame.h#149

So this is definitely true of the flex container frame itself.

It could be true of the flex container frame's parent / grandparent / etc., up to (but not including) its containing block.

So I think you're right -- we should really be setting this bit on the parents, in something similar to how nsHTMLReflowState::InitResizeFlags does.

Perhaps nsHTMLReflowState::InitResizeFlags should just include this situation (positioned w/ auto-height & top/bottom set) in its "dependsOnCBHeight" conditions?

Comment 2

[Daniel Holbert [:dholbert]]

(er s/on the parents/on the ancestor chain/)

Comment 3

[Daniel Holbert [:dholbert]]

Actually, it looks like nsHTMLReflowState::InitResizeFlags should already be checking that (though it apparently wasn't sufficient to bug 841827's situation)... investigating.

Comment 4

[Daniel Holbert [:dholbert]]

Ah -- so it looks like InitResizeFlags *only* sets NS_FRAME_CONTAINS_RELATIVE_HEIGHT on the ancestor chain -- not on |frame| itself.  And from an IRL chat with dbaron right now, it sounds like that's really the intended usage of CONTAINS_RELATIVE_HEIGHT -- it's generally not supposed to be set on the frame that *has* a relative height. (despite the documentation quoted in comment 1)

SO: To answer the question posed in comment 1:
 - We are correctly setting the bit on the ancestor chain. InitResizeFlags does that for us.
 - BUT it might be slightly-incorrect that nsFlexContainerFrame is bothering to set that flag on itself, and then rely on it. I'm setting it so that nsHTMLReflowState::ShouldReflowAllChildren() will return true in cases where we're being vertically resized, but dbaron says we probably should just be checking "ShouldReflowAllChildren() || mVResize" instead of ShouldReflowAllChildren().

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> ... but dbaron says we probably should just be
> checking "ShouldReflowAllChildren() || mVResize" instead of
> ShouldReflowAllChildren().

Also, we might need to rethink the appropriateness of the mHResize / mVResize-setting code for flexbox. It's basically targeted at blocks right now, and we might need to make flexbox take a different path through that code: 
https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#523

Comment 6

[Daniel Holbert [:dholbert]]

Alternately, we could just get rid of nsFlexContainerFrame's ShouldReflowAllChildren() check entirely and assume that, if someone tells a flexbox to reflow, it should always reflow all its children. (And then let the children optimize their own reflows as appropriate.)

Comment 7

[Daniel Holbert [:dholbert]]

I filed bug 851607 on getting rid of nsFlexContainerFrame's ShouldReflowAllChildren() check.

Also, RE "we might need to rethink the appropriateness of the mHResize / mVResize-setting code for flexbox" in comment 5 -- I just found the place where I ran up against that code before. It was in bug 821775 comment 6, with nested flex containers. Quoting that comment:
> Right now, the testcase's inner flex container ends up visiting the
> "!nsLayoutUtils::IsNonWrapperBlock(frame)" clause on its "real" reflow after
> a window-resize.
> 
> That clause ends up copying mVResize = false from the outer flex container. 
> I don't think we want that to happen