Open Bug 1242461 Opened 8 years ago Updated 2 years ago

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

Categories

(Core :: Layout, defect)

42 Branch
defect

Tracking

()

Tracking Status
firefox42 --- affected
firefox43 --- affected
firefox44 --- wontfix
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 --- wontfix

People

(Reporter: p.van.zutven, Unassigned)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files)

Attached file 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).
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9bca608ab65a3b1b5d95f91854f027169709f261&tochange=a1829935d87e6f2b5b4d64c0da52ca733a5b0662
Blocks: 1172239
Status: UNCONFIRMED → NEW
Component: Untriaged → Layout
Ever confirmed: true
Flags: needinfo?(roc)
Keywords: regression, testcase
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Version: Trunk → 42 Branch
Recent regression, tracking for 45+. Wontfix for 44 since we have already shipped.
bz: my patch is based on code you reviewed here:
https://reviewboard.mozilla.org/r/13593/diff/1#index_header
Summary: Height:100% after switching element from min-width to width → height:100% should resize after switching parent from min-height to explicit height
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?
Flags: needinfo?(dbaron)
Attachment #8719049 - Flags: review?(bzbarsky) → review-
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.)
Flags: needinfo?(dbaron)
(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.
(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.
(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).
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 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.
(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.
Flags: needinfo?(roc)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: