375304 - Wrong absolute positioning when the containing block has a min-height and overflow:hidden

Status: VERIFIED FIXED

(Core :: Layout: Positioned, defect, P2)

Description

[Bruno Fassino]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3

A position:relative box with a min-height and overflow:hidden contains a position:absolute box with bottom:0.  
The absolute box is positioned at the bottom of the content of the containing block, as if no min-height were specified on it.


Reproducible: Always




If the containing block doesn't have both a min-height and overflow:hidden the problem does not occur. 
Doesn't seem present in older versions (1.7), it is present in latest Minefield.

Comment 1

[Bruno Fassino]

Attachment: Minimal test case

The red absolutely positioned box has bottom:0, its containing box is the yellow box having a min-height and overflow:hidden.

Comment 2

[philippe (part-time)]

Can't seem to find a dupe, so confirming.
That regressed, based on Mac builds, between the 20050428-07 and 20050428-20 builds.

A regression from bug 240276 ?

Comment 3

[Eli Friedman]

Attachment: Patch?

This works, but I'm not sure this is what we want to do... (it obviously doesn't handle a horizontal scrollbar correctly, but that's not a new bug.)

Comment 4

[Eli Friedman]

dbaron, roc, any suggestions?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm ... I actually think that's reasonable, if you adjust for the horizontal scrollbar. But don't you want this for computed widths too?

Comment 6

[Eli Friedman]

(In reply to comment #5)
> Hmm ... I actually think that's reasonable, if you adjust for the horizontal
> scrollbar. But don't you want this for computed widths too?
> 

I don't think it really matters for computed widths; we come up with the right width anyway.

Hmm, and probably also want to get rid of the hack in nsHTMLReflowState that skips over the scrolled frame for percentage height calculations.

Comment 7

[Eli Friedman]

Do I need to do anything special with respect to http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsGfxScrollFrame.cpp&rev=3.307&mark=598-600#598 ?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No --- that's already broken, you won't break it worse

Comment 9

[Eli Friedman]

Attachment: WIP

Okay, here's the complete fix; I need to mess with nsHTMLScrollFrame::ReflowContents a bit more, though.

Comment 10

[Eli Friedman]

Okay, a bit of an explanation: the second patch is a lot bigger and a lot scarier than the first patch.  However, it's necessary if we want to account for horizontal scrollbars without introducing incremental reflow problems.

If the complete fix is too scary performance-wise to do at this point, we can go with the first patch.  It doesn't account for horizontal scrollbars, but to do height computation including the horizontal scrollbar correctly, we have to do the complete fix.

Comment 11

[Eli Friedman]

Attachment: WIP v2

This version is a bit more cleaned up and partially tested.

Comment 12

[Eli Friedman]

Okay, my current progress: WIP v2 seems to be working correctly for basic cases, and it shouldn't change performance for cases where the NS_FRAME_CONTAINS_RELATIVE_HEIGHT bit is not set or we don't need a horizontal scrollbar.  I mostly need to do some more testing to make sure everything behaves sanely.

Note that the current code isn't always successful at coming up with a minimal arrangement; the
"if (nsRect(nsPoint(0, 0), InsideBorderSize).Contains(scrolledRect)) {" check isn't sufficient to catch all the cases where we don't need a scrollbar because making a little more width available can lead to a large change in the height.  I'm not sure if we care, though; not removing scrollbars when they're no longer needed is suboptimal, but not really wrong, and rather expensive to get right.

Comment 13

[Eli Friedman]

(Note to self: CalcQuirkContainingBlockHeight needs to be fixed.)

Comment 14

[Eli Friedman]

Attachment: Patch v1

Okay, I think this is finished.  I'll make sure to together some reftests (both quirks and standards-mode).

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

     // XXX Adding a horizontal scrollbar could cause absolute children positioned
     // relative to the bottom padding-edge to need to be reflowed. But we don't,
     // because that would be slow.

This comment is incorrect now, right? We are reflowing if anything could be depending on the bottom padding-edge. Where this comment is, you're no longer doing TryLayout in this situation.

+    // XXX Adding a horizontal scrollbar could cause absolute children positioned
+    // relative to the bottom padding-edge to need to be reflowed. But we don't,
+    // because that would be slow.

Similarly, here you just reflowed the scrollbar...

Otherwise this looks good, but a couple of things to consider:
-- The parameters to TryLayout should be switched to H/V to be consistent with everything else. Could be done separately but the code is confusing me as is. Yes, I know I wrote that :-)
-- Could we simplify ReflowContents by moving the ReflowScrolledFrame calls into TryLayout itself, with a check in there to suppress the reflow if only the presence of a horizontal scrollbar is changing and NS_FRAME_CONTAINS_RELATIVE_HEIGHT is not set? We'd need state variables to record the scrollbar configuration used for the last reflow of the scrolled content. Then I think we can dispense with checking style in ReflowContents, it can become an initial ReflowScrolledFrame followed by 6 TryLayout calls (the first one conditional on the size of the child fitting).

Comment 16

[Eli Friedman]

Attachment: Real patch v1

I accidentally asked for review on the wrong revision of the patch; sorry about that.  I'll be more careful about that in the future.

I believe this version addresses the issue of the comments, and partially addresses the issue of the parameter order by using named variables more frequently. I can switch the parameter order if it would make it easier to review, though. Which order do you prefer?  (Or do you not really care as long as it's consistent?)

This patch also fixes some bugs in the earlier version.

I think moving the calls to ReflowScrolledFrame is complicated enough to warrant a separate patch.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 273392 [details] [diff] [review]
Real patch v1

relativeHeight perhaps could be named "heightMatters" or something a bit more descriptive. Other than that, this looks good.

Should have a followup bug to refactor things the way I described.

Comment 18

[Eli Friedman]

Attachment: Reftests

I'd like you to take a quick look over this to make sure I'm not missing any tests (or if you have other suggestions).  This is tricky stuff.

Note that none of the browsers on my computer other than my patched trunk build pass any of the horizontal scrollbar tests.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 278476 [details] [diff] [review]
Reftests

I find it really hard to review tests. Lets go with these.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll take this and land the patch when the tree is open for blocker landings.

Comment 21

[Reed Loden [:reed]]

Patch is bitrot. bz says reftests are missing stuff.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: unbitrotted patch

I'll look at the tests later...

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Boris, Reed says you said "reftests are missing stuff". What did you have in mind?

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, Reed says you just meant a hook from reftests/reftest.list.

Checked in with that.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Yeah, it just needed a hook (and some indication of where in the dir tree to put it).

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This actually caused a reftest failure in bugs/28811-1a.html and bugs/28811-1b.html, so I'm backing it out. Looks a real bug, an unnecessary horizontal scrollbar appears.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The reftest failure is fixed by my patch in bug 399531. So we'll need to land that first.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch + tests

Here's the patch and reftests rolled together, ready to be relanded.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in again.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This caused 28811-1a.html and 28811-1b.html to fail again on Linux and Windows, but curiously not on Mac. This time there is no horizontal scrollbar, instead the problem is that the bottom border of the IFRAME in the reference page appears to be missing. Reopening again.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This may also have caused a Tp2 regression of bl-bldxp01, not sure. Need to recheck that, there should be another cycle before the backout takes effect.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Unclear if there was a Tp2 regression. Something to watch when relanding.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The regression is basically that if we have a horizontal and a vertical scrollbar, and we resize contents so there's a consistent layout with a vertical but no horizontal scrollbar but no other scrollbar combination gives a consistent layout, and the contents have set relativeHeight, then there is no path that tries the vertical but no horizontal combination. So we fall back to displaying both scrollbars.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated again

This is an updated patch that fixes the regression. I've only changed nsGfxScrollFrame.cpp/.h from the previous iteration so they're the only files that need review again.

I'm addressing the issue I just mentioned, which is that when relativeHeight is set, we don't try all combinations of scrollbar visibilities. The code was getting hard to understand so I've refactored it by moving the calls to ReflowScrolledFrame when scrollbar status has changed from ReflowContents down into TryLayout. I think this makes it much easier to see what's going on. In particular it is now very easy to see that we try all scrollbar combinations.

Comment 35

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

Comment on attachment 291373 [details] [diff] [review]
updated again

r+sr=dbaron

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in again

Comment 37

[Al Billings [:abillings - ex-MoCo]]

Verified in Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre.