padding-top results in space below float too

RESOLVED FIXED

Status

()

Core
Layout: Floats
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: none98765, Assigned: dbaron)

Tracking

(4 keywords)

Trunk
fixed-aviary1.0, fixed1.7, regression, testcase
Points:
---
Bug Flags:
blocking-aviary1.0 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

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

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

=>Browser
Assignee: firefox → nobody
Component: General → Layout
Keywords: qawanted
Product: Firefox → Browser
QA Contact: firefox.general → core.layout
Version: unspecified → Trunk

Comment 2

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

14 years ago
Created attachment 159049 [details]
Testcase

Updated

14 years ago
Status: UNCONFIRMED → NEW
Component: Layout → Layout: Floats
Ever confirmed: true
Keywords: qawanted → testcase
OS: Windows XP → All
QA Contact: core.layout → core.layout.floats
Summary: Incorrect spacing /handling of CSS elements → padding-top results in space below floater too

Comment 4

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

Comment 5

14 years ago
Yep, backing out bug 140611 fixes this bug...

Updated

14 years ago
Hardware: PC → All
*** Bug 263888 has been marked as a duplicate of this bug. ***

Comment 7

14 years ago
Created attachment 162656 [details]
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.

Updated

14 years ago
Flags: blocking-aviary1.0?

Comment 8

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

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

Updated

14 years ago
Attachment #162674 - Flags: superreview?(dbaron)
Attachment #162674 - Flags: review?(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.
Attachment #162674 - Flags: review?(smontagu) → review?(dbaron)
Yikes!  Why did bug 140611 land on the aviary branch?

Comment 12

14 years ago
This patch fixes bug 263888 as well, btw.
Summary: padding-top results in space below floater too → padding-top results in space below float too
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

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

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

Comment 16

14 years ago
sounds like we need to take this patch or back out bug 140611
Flags: blocking-aviary1.0? → blocking-aviary1.0+
Created attachment 162952 [details]
slightly simpler testcase
Attachment #162656 - Attachment is obsolete: true
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.

Updated

14 years ago
Assignee: nobody → dbaron
OK, wrong repetition code -- it's something wrong with the code in
nsBlockFrame::ComputeFinalSize, I suspect.
Actually, after looking at HorizontallyAlignFrames and the meaning of
BRS_SHRINKWRAPWIDTH a little more, I think the check ought to be unneeded.
Created attachment 163083 [details] [diff] [review]
slightly more accurate restoration of old code

I think this is a more accurate restoration of the old code.
Attachment #163083 - Flags: superreview?(roc)
Attachment #163083 - Flags: review?(roc)
Attachment #163083 - Flags: approval1.7.x?
Attachment #163083 - Flags: approval-aviary?
Attachment #163083 - Flags: superreview?(roc)
Attachment #163083 - Flags: superreview+
Attachment #163083 - Flags: review?(roc)
Attachment #163083 - Flags: review+

Comment 22

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

a=asa for aviary and 1.7 branch checkins.
Attachment #163083 - Flags: approval1.7.x?
Attachment #163083 - Flags: approval1.7.x+
Attachment #163083 - Flags: approval-aviary?
Attachment #163083 - Flags: approval-aviary+
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.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Keywords: fixed-aviary1.0, fixed1.7
Resolution: --- → FIXED

Updated

14 years ago
Attachment #162674 - Attachment is obsolete: true
Attachment #162674 - Flags: superreview?(dbaron)
Attachment #162674 - Flags: review?(dbaron)
*** Bug 263888 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.