209066 - [FIX] incremental reflow goes wrong when block has spaces around it

Status: RESOLVED FIXED

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

Description

[Hixie (not reading bugmail)]

STEPS TO REPRODUCE
  div { border: 1px solid; width: 25em; margin: 2em auto; }
  <table><tr><td><div></div><td valign="bottom"> <div></div> </td></tr></table>

  The spaces around the <div> in the second cell are required to show the bug.

EXPECTED RESULTS
  Two adjacent lines.

ACTUAL RESULTS
  The second line is first drawn too high. A reflow (font zoom change, e.g.) 
  will move the second line down.

Comment 1

[Bernd]

Attachment: dynamic testcase showing the correct behaviour

Comment 2

[Bernd]

Attachment: reflow log

Comment 3

[Bernd]

the second (resize) reflow
         cell 0266F168 r=2 a=6060,UC c=6030,UC cnt=878 
          block 0266F23C r=2 a=6030,UC c=6030,UC cnt=879 
           text 0266F2E8 r=2 a=6030,UC c=UC,UC cnt=880 
           text 0266F2E8 d=0,0 
           block 0266F39C r=2 a=6030,UC c=6000,UC cnt=881 
           block 0266F39C d=6030,30 
          block 0266F23C d=6030,1470 
         cell 0266F168 d=6060,1500 

and the style change reflow 
         cell 0266F168 r=3 a=6060,UC c=6030,UC cnt=907 
          block 0266F23C r=3 a=6030,UC c=6030,UC cnt=908 
           text 0266F2E8 r=3 a=UC,UC c=UC,UC cnt=909 
           text 0266F2E8 d=0,0 me=0 
           text 0266F2E8 r=3 a=6030,UC c=UC,UC cnt=910 
           text 0266F2E8 d=0,0 
           block 0266F39C r=3 a=6030,UC c=6000,UC cnt=911 
           block 0266F39C d=6030,30 me=6030 m=15 
           text 0266F3F0 r=3 a=UC,UC c=UC,UC cnt=912 
           text 0266F3F0 d=0,0 me=0 
           text 0266F3F0 r=3 a=6030,UC c=UC,UC cnt=913 
           text 0266F3F0 d=0,0 
          block 0266F23C d=6030,990 me=6030 m=15 
         cell 0266F168 d=6060,1020 me=6060 m=6060 

The block is reporting a too large height 1470 instead of 990 ( just a blind
guess 1470 = 990 + 990/2)
This is in my oppinion a block bug

Comment 4

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

This seems to be related to the 'auto' side margins.

Comment 5

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

Attachment: simpler testcase

Only one piece of whitespace (plus borders around the table cells).

Comment 6

[Boris Zbarsky [:bzbarsky]]

*** Bug 214394 has been marked as a duplicate of this bug. ***

Comment 7

[Boris Zbarsky [:bzbarsky]]

From bug 214394:

The rendering of the testcase changed between 2001-10-23-08 and 
2001-10-26-08.  Of the checkins in that range, the one for bug 86947 arouses the
most suspicion in me, followed by deCOMtamination of nsSpaceManager and
deCOMtamination of nsIFrame that hyatt landed...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: This may also be this same bug....

In this testcase, the bottom margin of the <div> is absent for some reason. 
Making the left/right margins not auto fixes it....

Comment 9

[Adam D. Moss]

seen on linux, -> all/all

Comment 10

[Mats Palmgren (inactive)]

FWIW, it seems this bug only occurs within tables.

Comment 11

[Mats Palmgren (inactive)]

Taking this since it's unassigned and I have a fix...

Comment 12

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

The bug is in nsHTMLReflowState::ComputeMargin() - when the containing block
size
is NS_UNCONSTRAINEDSIZE we simply ignore any specified top/bottom margin and
set the computed value to zero.  I have just removed this bit and let
ComputeHorizontalValue() deal with it (since it can do that now).

Comment 13

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1 (diff -uw for easier review)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Mats, are you confident enough in this fix to go for getting it into 1.5?  If 
so, please request approval?

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 129993 [details] [diff] [review]
Patch rev. 1

I am very confident that this patch does the right thing.

Comment 16

[Asa Dotzler [:asa]]

Comment on attachment 129993 [details] [diff] [review]
Patch rev. 1

1.5b shipped. moving request forward.

Comment 17

[Asa Dotzler [:asa]]

Mats, is this likely to break backward compatibility or cause regressions out on
the web?

Comment 18

[Mats Palmgren (inactive)]

No, it only occurs when left or right margin has the specified value 'auto' and
and a specified non-zero top or bottom margin (which we currently ignore).
This combination is rarely used by CSS authors.
(The initial margin value is zero)

It does affect <hr> though (after bug 38370 was fixed) since it now uses this
combination of margin values.  I think it's more likely that there will be
regressions from bug 38370 if this patch is NOT checked in.

Comment 19

[Mats Palmgren (inactive)]

Attachment: Screenshot of 1st testcase in bug 119606 (with the patch applied)

Funny, I just stumbled into a regression from bug 38370.  See the first
testcase in bug 119606 and compare the last two columns with this screenshot.

Comment 20

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

I think we should get this in for 1.5.

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 129993 [details] [diff] [review]
Patch rev. 1

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

Comment 22

[Mats Palmgren (inactive)]

David / Boris - could you check this in for me?  Thanks.

Comment 23

[Boris Zbarsky [:bzbarsky]]

I have no CVS access till I get back.  biesi, could you do this if dbaron
doesn't get to it soon?

Comment 24

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

Fix checked in to trunk, 2003-09-05 21:01 -0700.