1242461 - height:100% should resize after switching parent from min-height to explicit height

Status: NEW

(Core :: Layout, defect)

Description

[p.van.zutven]

Attachment: heightbug.html

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20160105164030

Steps to reproduce:

I have an element that contains a "height:100%" element. The outer element initially has a min-height, but no height; that min-height is then replaced by a fixed height.


Actual results:

The element with "height:100%" stays the same size as before (so the size of its content, as height:100% is not affected by the initial min-height)


Expected results:

The element with "height:100%" should have resized to fill its parent element (the desired result can be triggered manually by resizing the browser window).

Comment 1

[Loic]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9bca608ab65a3b1b5d95f91854f027169709f261&tochange=a1829935d87e6f2b5b4d64c0da52ca733a5b0662

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Recent regression, tracking for 45+. Wontfix for 44 since we have already shipped.

Comment 3

[Jet Villegas (inactive)]

Attachment: Bug 1242461: account for relative sizing when block size changes

Comment 4

[Jet Villegas (inactive)]

bz: my patch is based on code you reviewed here:
https://reviewboard.mozilla.org/r/13593/diff/1#index_header

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8719049 [details] [diff] [review]
Bug 1242461: account for relative sizing when block size changes

This doesn't seem like the right approach to me.  This would clear intrinsic isizes on all descendants, which is really not what we want here: it's way too big a hammer.  We just want to not optimize out the relayout of the kid in this case.

In an ideal world we would detect this situation and SetBResize(true) in nsHTMLReflowState::InitResizeFlags.  But the issue in this case is that the check there finds that our current size is the size we now want, so we don't SetBResize() for the _parent_.  And then I expect we optimize out the child reflow because we think its size can't have changed....

David, any bright ideas?

Comment 6

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

Two possibilities:

 (1) store a boolean somewhere (probably need more frame state bits) that says whether a frame had a height that percentages could be relative to the last time around, and make nsHTMLReflowState::IsBResize handle that

 (2) drop the "IBResize() &&" from nsHTMLReflowState::ShouldReflowAllKids.  (We should try to get performance numbers for this if we want to do it.)

Comment 7

[Jet Villegas (inactive)]

(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #6)
>  (2) drop the "IBResize() &&" from nsHTMLReflowState::ShouldReflowAllKids. 
> (We should try to get performance numbers for this if we want to do it.)

I tried this easier fix first. It doesn't fix the bug because it's the bottom child that has a relative size in the test case.

>  (1) store a boolean somewhere (probably need more frame state bits) that
> says whether a frame had a height that percentages could be relative to the
> last time around, and make nsHTMLReflowState::IsBResize handle that

I'll try this next.

Comment 8

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

(In reply to Jet Villegas (:jet) from comment #7)
> (In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #6)
> >  (2) drop the "IBResize() &&" from nsHTMLReflowState::ShouldReflowAllKids. 
> > (We should try to get performance numbers for this if we want to do it.)
> 
> I tried this easier fix first. It doesn't fix the bug because it's the
> bottom child that has a relative size in the test case.

If that didn't work, then (1) won't work either.

But I don't understand why it didn't work.

Comment 9

[Jet Villegas (inactive)]

(In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #8)
> (In reply to Jet Villegas (:jet) from comment #7)
> > (In reply to David Baron [:dbaron] ⌚️UTC-8 from comment #6)
> > >  (2) drop the "IBResize() &&" from nsHTMLReflowState::ShouldReflowAllKids. 
> > > (We should try to get performance numbers for this if we want to do it.)
> > 
> > I tried this easier fix first. It doesn't fix the bug because it's the
> > bottom child that has a relative size in the test case.
> 
> If that didn't work, then (1) won't work either.
> 
> But I don't understand why it didn't work.

What did work is removing the IsBResize() check from nsBlockFrame::ReflowDirtyLines()

  bool selfDirty = (GetStateBits() & NS_FRAME_IS_DIRTY) ||
                     (/* aState.mReflowState.IsBResize() && */ 
                      (GetStateBits() & NS_FRAME_CONTAINS_RELATIVE_BSIZE));

I'm now trying to modify IsBResize() to return true for the test case using a new frame state flag, which is what I think you meant in (1).

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

If we come up with a reliable fix you can still request uplift.
But for now wontfixing for 45 and 46, regression since 42 and there is not a clear owner or a potential patch. Do we need to keep tracking this?

Comment 11

[p.van.zutven]

I worked around the issue by adding 0.1 to the height when switching from min-height to height (visually the same, but does trigger a relayout), so certainly no need for an uplift on my behalf.

Comment 12

[Jet Villegas (inactive)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10)
> If we come up with a reliable fix you can still request uplift.
> But for now wontfixing for 45 and 46, regression since 42 and there is not a
> clear owner or a potential patch. Do we need to keep tracking this?

I don't think we need to keep tracking this. The common case is to set a height that differs from the min-height (which is why the workaround in comment 11 works.) I think we want to keep the optimizations already in place as per comment 5.