235558 - using max-width and overflow CSS properties on a div cause wrong height to be calculated

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect, P2)

Description

[Flo Ledermann]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040113

if i assign to an absolutely positioned DIV element the max-width property via
CSS (no width or height properties set), and override the defalt 'overflow'
setting by either 'hidden' or 'scroll', the height is set incorrectly to only
the first line of the content.

expected result: width should be set to max-width, height should not be
constrained (since not set)

it seems that this is a regression, since i am absolutely sure that this worked
in 1.5.

Reproducible: Always
Steps to Reproduce:
1. load the attached testcase
2. the first and fourth box are laid out incorrectly, because bothe are using
max-width and overflow properties
3. the second and third box are laid out correctly.
4. details in the css definitions for the four elements

Actual Results:  
first and fourth box are laid out incorrectly (calculated height is to small,
only showing the first line). the fourth box shows paradoxically a *horizontal*
scrollbar, when it should show none or (with too small height) a vertical.

Expected Results:  
all four boxes should be laid out identically

Comment 1

[Flo Ledermann]

Attachment: sample file to reproduce the bug. test cases 2 and 3 are correct, 1 and 4 are incorrect. details in the css code.

added testcase

Comment 2

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

Weird. Must be a reflow problem, but I'll take it.

Comment 3

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

Attachment: fix

Okay, unsurprisingly the problem is that nsGfxScrollFrame doesn't handle
max-width/max-height very well. The problem in the testcase box 1 is simple:
the computed width and height are NS_INTRINSICSIZE, so in
nsGfxScrollFrame;:GetPrefSize we don't pass any width or height constraints
down to the scrolled block, so it reflows itself all into one line and returns
a preferred height of one line. nsGfxScrollFrame returns that as its preferred
height. But, later in nsBoxFrame we actually apply the max-width and oops, the
block doesn't fit on one line anymore.

This patch refactors the GetPrefSize logic a little bit. We distinguish between
the computed size of the scrollframe, if any, and the maximum size (the
computed size if known, otherwise the CSS max if there is one). The maximum
size is what we pass down as the width/height constraints for reflowing the
block. That fixes all aspects of this testcase nicely.

Metacomment: nsGfxScrollFrame really wants to be a block-like thing, not a XUL
box.

Comment 4

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

Attachment: fix #2

Slightly better fix. Passing a height constraint to the scrolled block just
confuses it because it thinks we're printing (see especially nsBlockReflowState
constructor). So let's disable that.

Comment 5

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

Attachment: Testcase including max-height

This extends the provided testcase (thanks!! for that) with max-height on the
DIVs. Use Ctrl+ and Ctrl- to grow and shrink the text to see everything working
in its full glory.

Comment 6

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

Another layout regression fix that I would like to have in 1.7b, although we
could probably slip it in between 1.7b and 1.7final without losing too much sleep.

Comment 7

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

Comment on attachment 143090 [details] [diff] [review]
fix #2

r+sr=dbaron.  I may try to comment on the patch at a later date, but for now
we're probably better off just getting it in so it gets tested.

Comment 8

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

Comment on attachment 143090 [details] [diff] [review]
fix #2

layout regression, needs 1.7b exposure

Comment 9

[chris hofmann]

Comment on attachment 143090 [details] [diff] [review]
fix #2

a=chofmann for 1.7b

Comment 10

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

checked in

Comment 11

[Boris Zbarsky [:bzbarsky]]

This seems to be causing bug 237622... the fact that simply setting
overflow:hidden on a div changes its dimensions after this patch is bad.

Comment 12

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

Attachment: backout

Without major changes, I think it's impossible for us to simultaneously handle
A) float or any block with CSS max-width, with overflow:hidden, width:auto,
height:auto, intrinsincally narrower than its container, that contains
width:auto child blocks (i.e., where the float/block needs to be reflowed with
NS_UNCONSTRAINEDSIZE available width to get its preferred size)
and
B) float or any block with CSS max-width, with overflow:hidden, width:auto,
height:auto, intrinsincally wider than its container and needing wrapping
(i.e., where the float/block needs to be reflowed with some fixed available
width to get its preferred size)

The fix in this bug switched us from doing A correctly to doing B correctly.
This backout will switch us back to doing A correctly and B incorrectly.

Comment 13

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

Comment on attachment 144629 [details] [diff] [review]
backout

David, I'm not sure that this is the right thing to do, but it's really a
policy decision I don't feel qualified to make. We're backed into a bit of a
corner here and I think we have to choose one bug to ship with.

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 144629 [details] [diff] [review]
backout

a=asa (on behalf of drivers) for checkin to 1.7

Comment 15

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

Backout checked in.

Comment 16

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

A real fix to this will require major changes to nsGfxScrollFrame. I think we're
going to have to separate it into nsXULScrollFrame that behaves like a XUL box,
and nsHTMLScrollFrame that behaves like an HTML block. The change will be
painful but ultimately will simplify things, I think (especially if you include
the hacks currently in nsBoxToBlockAdapter). I'd like to get to this in 1.8a
because until it's done we're stuck with some fairly serious layout bugs, like
this one.

Comment 17

[Rick Stockton]

FYI, We're being slammed about this on on ZDNet,
http://news.zdnet.com/2100-9588-5438955.html

Comment 18

[Hixie (not reading bugmail)]

That doesn't seem like the same bug to me.

Comment 19

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

Both of these are completely fixed by my upcoming patch in bug 240276.

Comment 20

[Steve England [:stevee]]

Testcase fails for me with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.8b2) Gecko/20050422 Firefox/1.0+
Testcase works for me with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US;
rv:1.8b2) Gecko/20050428 Firefox/1.0+
Resolved Fixed by bug 240276 & comment 20