Closed Bug 1263845 Opened 8 years ago Closed 8 years ago

A table of height:100% should size to its parentNode, NOT stretch its parentNode, when the parent node changes from fixed to auto height

Categories

(Core :: Layout: Tables, defect)

45 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 + fixed
firefox48 + fixed
firefox49 --- fixed

People

(Reporter: nige, Assigned: bzbarsky)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: regression, testcase)

Attachments

(6 files)

Attached file ffbug.html
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.110 Safari/537.36

Steps to reproduce:

A table has height:100% and is a child of a div with a fixed height. It correctly stretches its height to match its parent.

Remove the height from the parent, and on ALL other browsers, the height collapses back to the content height.

In FF45, the height remains the same with nothing now commanding it to stretch.

I have screenshots of this working on all other browsers. I also see many related reports for FF with regard to table heighting and bug reports being rejected. I think your layout code is in a mess, and you don't want to fix it.



Actual results:

The height does not collapse back to content height.


Expected results:

The height should collapse back to content height.
Attached image Safari result
Attached image Chrome result
Attached image IE11 result
Attached image FF result
Component: Untriaged → Layout: Tables
Product: Firefox → Core
Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=789a931f4344ad2d36a8e7bb92634b618d4e3b58&tochange=beb4cf7d167970d5c774bfe903211e8d58633829
Blocks: 1209994
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bzbarsky)
Keywords: regression, testcase
OS: Unspecified → All
Hardware: Unspecified → All
Before bug 1209994 we took the "SetBResize(mCBReflowState->IsBResize())" path for the table outer and table inner frame (since both would certainly test false for nsLayoutUtils::IsNonWrapperBlock(frame)) and that happened to "work" in the sense of setting the BResize flag on the reflow state, because the parent is in fact resizing vertically.

Now we take the if (ComputedBSize() == NS_AUTOHEIGHT) path, which does not set the BResize flag, because we're in standards mode, not resizing horizontally, and not dirty.

The thing is, it seems to me like the BResize flag _should_ get set in this case, because we went from having a non-auto height to having an auto height... It's not clear to me why things work correctly if the inner div is a block instead of a table, but it might just be because block reflow doesn't optimize as aggressively around BResize or something.

In particular, nsTableFrame only really recomputes its height if this condition holds:

  if (NS_SUBTREE_DIRTY(this) ||
      aReflowState.ShouldReflowAllKids() ||
      IsGeometryDirty() ||
      aReflowState.IsBResize()) {

I wish I could ask dbaron for the expected semantics of the BResize flag here, but he's not accepting needinfo requests right now...
Blocks: 1240361
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(bzbarsky)
Summary: A table of height:100% should size to its parentNode, NOT stretch its parentNode. → A table of height:100% should size to its parentNode, NOT stretch its parentNode, when the parent node changes from fixed to auto height
I'm having trouble convincing myself this is correct.

What if:

 * the containing block changed from 'auto' height to the non-'auto' height that leaves its size the same, which means the containing block failed to set the b-resize flag in the last else in the chain.  This patch could propagate that false b-resize further even if this frame has a percentage height.  Is that OK?

 * ComputedBSize() == NS_AUTOHEIGHT and IsIResize() is true, and we're not in quirks mode:  the current code uses IsIResize, and you stop doing that.  (Then again, I'd need to look at blame to understand why we use IsIResize(), and haven't gotten to that yet, beyond figuring out that it dates to the origin of the function on the reflow branch.)



I'm also trying to remember whether the BResize flag means more than that we need to look for percentage height children (which should, I though, only matter for non-'auto' height).  (I *think* that's what it was designed for.)  Maybe some of the users of the flag are overoptimizing?  (As I said, I don't understand that IsIResize() bit.)  I'm not sure if we have a great definition of what the flag means; the comment suggests it's entirely about whether the basis for percentage heights can change, but I'm not sure if that's still the case.
> * the containing block changed from 'auto' height to the non-'auto' height that leaves its size the same,

This is the situation of bug 1242461, right?  It's definitely still broken after this change.  All I'm trying to do here is to restore us closer to the state we were in before bug 1209994 without reintroducing it in the process.  I agree that the result is not actually "correct" in any meaningful sense...

> ComputedBSize() == NS_AUTOHEIGHT and IsIResize() is true, and we're not in quirks mode:  the current code
> uses IsIResize, and you stop doing that.  

It didn't use to do that before bug 1209994, note.

> I'd need to look at blame to understand why we use IsIResize()

Well, before bug 1209994 the IsIResize() path was taken only for non-wrapper blockframes.  For those, a horizontal resize plus auto height generally implies a vertical resize.  But yes, I totally agree that the meaning of the BReize flag is ... unclear at best.
Comment on attachment 8741232 [details] [diff] [review]
When a parent changes from auto height to non-auto height or vice versa, a percentage height non-block child needs to realize it's doing a vertical resize

OK, the code here makes more sense in the context of bug 1209994, although I'm less sure the comment does (i.e., I'm really not sure what point the comment is trying to make, or how particular parts of the first paragraph of the comment relate to particular pieces of the code).

r=dbaron
Attachment #8741232 - Flags: review?(dbaron) → review+
That's fair.  I've revised the comment to ideally make it clearer, like so:

    // Some non-block frames (e.g. table frames) aggressively optimize out their
    // BSize recomputation when they don't have the BResize flag set.  This
    // means that if they go from having a computed non-auto height to having an
    // auto height and don't have that flag set, they will not actually compute
    // their auto height and will just remain at whatever size they already
    // were.  We can end up in that situation if the child has a percentage
    // specified height and the parent changes from non-auto height to auto
    // height.  When that happens, the parent will typically have the BResize
    // flag set, and we want to propagate that flag to the kid.
    //
    // Ideally it seems like we'd do this for blocks too, of course... but we'd
    // really want to restrict it to the percentage height case or something, to
    // avoid extra reflows in common cases.  Maybe we should be examining
    // mStylePosition->BSize(wm).GetUnit() for that purpose?
    //
    // Note that we _also_ need to set the BResize flag if we have auto
    // ComputedBSize() and a dirty subtree, since that might require us to
    // change BSize due to kids having been added or removed.

Does that help?
https://hg.mozilla.org/mozilla-central/rev/4e08f29820a3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Target Milestone: mozilla48 → mozilla49
Did you want to make approval requests for this, for aurora and maybe beta?
Flags: needinfo?(bzbarsky)
I think this flag was just a mistake from comment 5, and 48 is affected.
Hmm.  I'm a bit worried about uplifting this, but I guess it should be safe enough, since it's really a partial backout of bug 1209994's fix...
Flags: needinfo?(bzbarsky)
Comment on attachment 8741232 [details] [diff] [review]
When a parent changes from auto height to non-auto height or vice versa, a percentage height non-block child needs to realize it's doing a vertical resize

Approval Request Comment
[Feature/regressing bug #]: Bug 1209994
[User impact if declined]: Some sites will render incorrectly on dynamic changes
[Describe test coverage new/current, TreeHerder]: Clearly not quite enough.  I
   added tests for this specific situation, of course.
[Risks and why]: Medium.  Normally, I'd say changes to this code are high-risk,
   but this patch is just backing out part of bug 1209994, so reverting to the
   behavior we used to have before that fix in various cases.  So it's a lot
   safer than a typical change to this code.
[String/UUID change made/needed]: None.
Attachment #8741232 - Flags: approval-mozilla-beta?
Attachment #8741232 - Flags: approval-mozilla-aurora?
Comment on attachment 8741232 [details] [diff] [review]
When a parent changes from auto height to non-auto height or vice versa, a percentage height non-block child needs to realize it's doing a vertical resize

Baked in Nightly for a few days, includes automated tests, Aurora48+, Beta47+
Attachment #8741232 - Flags: approval-mozilla-beta?
Attachment #8741232 - Flags: approval-mozilla-beta+
Attachment #8741232 - Flags: approval-mozilla-aurora?
Attachment #8741232 - Flags: approval-mozilla-aurora+
Depends on: 1348848
Depends on: 1403656
You need to log in before you can comment on or make changes to this bug.