841827 - {inc} abspos flex container w/ "top: 0; bottom: 0" doesn't resize when window is resized

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR: Load attached testcase; resize window vertically

EXPECTED RESULTS: The black-bordered flex container should resize vertically w/ the container.

ACTUAL RESULTS: The flex container remains at its initial height (the window-height when the page loaded)

Comment 1

[Daniel Holbert [:dholbert]]

If I use gdb to forcibly set "shouldReflowChildren" to true, in nsFlexContainerFrame::Reflow,  after vertically resizing the window, then I get EXPECTED RESULTS. (the flex container resizes)

I think we should consider this a case where we've got a relative height (since our height depends on our containing block), so I think we should add this as a condition that makes us add the NS_FRAME_CONTAINS_RELATIVE_HEIGHT bit.

With that bit set, "aReflowState.ShouldReflowAllKids()" will end up returning true (from its mVResize && CONTAINS_RELATIVE_HEIGHT check), and we'll proceed with a full reflow.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1: treat abspos w/ explicit "top" & "bottom" values as being a relative height

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: reftest patch

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 718808 [details] [diff] [review]
reftest patch

(The reftest patch uses 'hg cp', so it looks like it's deleting a bunch of stuff, but really it's just co-opting the reusable parts.)

Comment 5

[Daniel Holbert [:dholbert]]

Green try run: https://tbpl.mozilla.org/?tree=Try&rev=17d6fa1dca72

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 718791 [details] [diff] [review]
fix v1: treat abspos w/ explicit "top" & "bottom" values as being a relative height

So the condition for abs-pos should *also* check && eStyleUnit_Auto == stylePos->mHeight.GetUnit() since the height is only a function of 'top' and 'bottom' when the 'height' is 'auto':
http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-height

r=dbaron with that fixed


But I'm also a little concerned about whether this is setting the bit on the right set of frames, but this patch is fine.  (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 7

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #6)
> So the condition for abs-pos should *also* check && eStyleUnit_Auto ==
> stylePos->mHeight.GetUnit() since the height is only a function of 'top' and
> 'bottom' when the 'height' is 'auto':
> http://www.w3.org/TR/CSS21/visudet.html#abs-non-replaced-height

Ah, right. Thanks!

> r=dbaron with that fixed
> 
> But I'm also a little concerned about whether this is setting the bit on the
> right set of frames, but this patch is fine.

It sounds like you're OK w/ this patch landing as-is and investigating your question as a followup -- so I pushed this bug's patch (w/ the reftest patch folded in) to inbound:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/fa2ffdf930be
...and I filed bug 851012 on investigating your followup question.

Comment 8

[Ed Morley [:emorley]]

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

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> It sounds like you're OK w/ this patch landing as-is

(with the height=auto review comment fixed, of course -- I fixed that before landing.)