259683 - padding-top results in space below float too

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect)

Description

[none98765]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040803 Firefox/0.9.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10

In the page at http://www.amrichmond.com/test there is a gap between the tabbed
interface that is created with stylesheets.

Gap appears as additional padding below tab images, but above the bottom of the
bg image (which appears as a bottom border)

This gap does not appear in Firefox 0.9.3 nor in other browsers - just in the
newest pre-release of Firefox 0.10

Reproducible: Always
Steps to Reproduce:
1. went to web page
2.
3.

Actual Results:  
gap appeared

Expected Results:  
no gap

CSS validates as correct using : W3C CSS Validation  at
http://jigsaw.w3.org/css-validator/

HTML validates at : http://validator.w3.org/

Comment 1

[Bill Mason]

If this is a bug, it is not Firefox specific.  Current trunk builds show the
same rendering.

=>Browser

Comment 2

[Gérard Talbot]

Can you create a *reduced* testcase out of that file? In particular, the html
file links to 2 distinct separate stylesheets with over 48 css rules and many
more declarations; some of these are contradictory. 
Is the behavior related to javascript? If not, can you remove the relation to
imagemanip.js file?

"When you've cut away as much HTML, CSS, and JavaScript as you can, and cutting
away any more causes the bug to disappear, you're done."
What is a Simplified Test Case, and How Do I Make One?
http://www.mozilla.org/newlayout/bugathon.html#testcase

Comment 3

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 4

[Mats Palmgren (inactive)]

Regression window: 2004-06-19-06 -- 2004-06-21-05.
No obvious candidates in that time frame, bug 140611 looks closest.

Comment 5

[Mats Palmgren (inactive)]

Yep, backing out bug 140611 fixes this bug...

Comment 6

[Uri Bernstein]

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

Comment 7

[Sander]

Attachment: a more common testcase

Adding another testcase which will be more common in the wild. As bug 140611
was checked in on the 1.7 branch (including aviary), Firefox 1.0 will have this
bug if it isn't fixed. I don't know exactly how many websites will suffer from
it, but I think this might do with some attention.

Comment 8

[Adam]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20041002 Firefox/0.10.1
(TierMann)

I see red under the green div as well.

Comment 9

[Sander]

Attachment: Fix

Comparing code from before and after, this patch seems obvious (the check for
shrinkwraping had been moved outside the check for RTLness - accidentally, I
assume), but I am quite unaware of the larger context which might come into
play. Luckily, that's what review is for. :) 
(Patch tested on testcases for bug 140611 and this one.)

Comment 10

[Simon Montagu :smontagu]

Comment on attachment 162674 [details] [diff] [review]
Fix

I don't think my r+ is worth much in layout code, better if dbaron gives r+sr.

Comment 11

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

Yikes!  Why did bug 140611 land on the aviary branch?

Comment 12

[Sander]

This patch fixes bug 263888 as well, btw.

Comment 13

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

So what's the deal here?  Are you saying that this was always broken for RTL,
but was changed to be broken for everybody, and you now want to change it to be
broken only for RTL?  Or does this code actually make sense?  If so, why?

Comment 14

[Sander]

*blinks and goes test*
Oh, heh. Yeah, you're right. This has always been broken for rtl, and all this
patch does is return to non-brokenness for non-rtl (while not damaging the
changes from bug 140611). Obviously a real fix will be necessary for the rtl
case at some point, but I suspect I'm nowhere near qualified to create such.
Despite that, I'll go and look if I can anyway, but whatever I could come up
with would probably be too risky for the aviary and 1.7 branches. This patch on
the other hand shouldn't be, and is in my estimate vastly preferable to both
backing out bug 140611 and to doing nothing.

*ponders* Actually, would there be any harm in simply removing the entire check
and return there? I focused on returning to the status quo in my previous patch,
under the assumption that such should be safe and correct, but as it isn't...

Comment 15

[Sander]

Unsurprisingly, removing that check completely has disastrous results for rtl cases.

Comment 16

[chris hofmann]

sounds like we need to take this patch or back out bug 140611

Comment 17

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

Attachment: slightly simpler testcase

Comment 18

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

So I suspect that the patch that caused this regression is actually OK (although
it missed a whole bunch of obvious simplifications in HorizontallyAlignFrames so
I'm not exactly confident).

It seems the difference is that for some reason (I don't know why yet) it's
triggering the "redoFloat" code within nsBlockFrame::ReflowFloat.  At least
that's what I think is causing the repetition based on a reflow log -- I didn't
check yet.  And there's probably some underlying bug related to that code,
perhaps involving space manager translation.

Comment 19

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

OK, wrong repetition code -- it's something wrong with the code in
nsBlockFrame::ComputeFinalSize, I suspect.

Comment 20

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

Actually, after looking at HorizontallyAlignFrames and the meaning of
BRS_SHRINKWRAPWIDTH a little more, I think the check ought to be unneeded.

Comment 21

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

Attachment: slightly more accurate restoration of old code

I think this is a more accurate restoration of the old code.

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 163083 [details] [diff] [review]
slightly more accurate restoration of old code

a=asa for aviary and 1.7 branch checkins.

Comment 23

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

Fix checked in to trunk, 2004-10-23 16:23 -0700.
Fix checked in to AVIARY_1_0_20040515_BRANCH, 2004-10-23 16:24 -0700.
Fix checked in to MOZILLA_1_7_BRANCH, 2004-10-23 16:24 -0700.

Filed bug 265796 to follow up for RTL cases.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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