The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Hixie (not reading bugmail), Assigned: mats)

Tracking

({css1, testcase})

Trunk
css1, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch], URL)

Attachments

(7 attachments)

(Reporter)

Description

14 years ago
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

14 years ago
Created attachment 125435 [details]
dynamic testcase showing the correct behaviour

Comment 2

14 years ago
Created attachment 125437 [details]
reflow log

Comment 3

14 years ago
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
Assignee: table → block-and-inline
Component: Layout: Tables → Layout: Block & Inline
QA Contact: madhur → ian
This seems to be related to the 'auto' side margins.
Component: Layout: Block & Inline → Layout: Tables
QA Contact: ian → madhur
Component: Layout: Tables → Layout: Block & Inline
QA Contact: madhur → ian
Created attachment 125444 [details]
simpler testcase

Only one piece of whitespace (plus borders around the table cells).
Attachment #125444 - Attachment is patch: false
Attachment #125444 - Attachment mime type: text/plain → text/html; charset=iso-8859-1
*** Bug 214394 has been marked as a duplicate of this bug. ***
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...
Created attachment 128835 [details]
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

14 years ago
seen on linux, -> all/all
OS: Windows 2000 → All
Hardware: PC → All
(Assignee)

Comment 10

14 years ago
FWIW, it seems this bug only occurs within tables.
Keywords: testcase
(Assignee)

Comment 11

14 years ago
Taking this since it's unassigned and I have a fix...
Assignee: block-and-inline → mats.palmgren
Keywords: css1
Summary: incremental reflow goes wrong when block has spaces around it → [FIX] incremental reflow goes wrong when block has spaces around it
Whiteboard: [patch]
(Assignee)

Comment 12

14 years ago
Created attachment 129993 [details] [diff] [review]
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).
(Assignee)

Comment 13

14 years ago
Created attachment 129994 [details] [diff] [review]
Patch rev. 1  (diff -uw for easier review)
(Assignee)

Updated

14 years ago
Attachment #129993 - Flags: review?(dbaron)
Attachment #129993 - Flags: superreview+
Attachment #129993 - Flags: review?(dbaron)
Attachment #129993 - Flags: review+
Mats, are you confident enough in this fix to go for getting it into 1.5?  If 
so, please request approval?
(Assignee)

Comment 15

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

I am very confident that this patch does the right thing.
Attachment #129993 - Flags: approval1.5b?

Comment 16

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

1.5b shipped. moving request forward.
Attachment #129993 - Flags: approval1.5b? → approval1.5?

Comment 17

14 years ago
Mats, is this likely to break backward compatibility or cause regressions out on
the web?
(Assignee)

Comment 18

14 years ago
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.
(Assignee)

Comment 19

14 years ago
Created attachment 130871 [details]
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.
I think we should get this in for 1.5.

Comment 21

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

a=asa (on behalf of drivers) for checkin to Mozilla 1.5
Attachment #129993 - Flags: approval1.5? → approval1.5+
(Assignee)

Comment 22

14 years ago
David / Boris - could you check this in for me?  Thanks.
I have no CVS access till I get back.  biesi, could you do this if dbaron
doesn't get to it soon?
Fix checked in to trunk, 2003-09-05 21:01 -0700.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.