Closed Bug 427129 Opened 12 years ago Closed 12 years ago

CSS margin problem at healthedco.com

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: dveditz, Assigned: dbaron)

References

()

Details

(Keywords: regression, testcase)

Attachments

(2 files, 5 obsolete files)

Found this reported on Hendrix, and I can confirm that the form looks OK in FF2, IE7, Opera and Safari, and shifted in FF3b5 as reported. Nominating because I don't know if this is a big deal or uncommon, but clearly a web-compat issue of some sort that should be looked at.

Name:    Eugene Morgan
Summary: Firefox 3 CSS margin bug?

Comments: 
The page at
http://www.healthedco.com/storefrontB2CWEB/jsp/request_catalog.jsp has a
form at the bottom of the page where the width of a floating element
(<label>) is being added to the left margin for an <input> element. But
FF2, IE7, Opera, and Safari do not behave this way.
Flags: blocking1.9?
Attached file testcase (obsolete) —
The label floats left.  The input has a left margin that before would go under the label.  But now it goes to the right of the label.
Crap.  Now I get the same rendering with FF2.  I checked at one point and had a different rendering.  I was checking my changes after that point in FF3 but not FF2.  
Attached file real testcase
Ok I cut one too many rules.  The display: Block rule is the one that makes the difference.
Attachment #313711 - Attachment is obsolete: true
Regression range:
20060705 broken
20060704 ok
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-07-04+04%3A00%3A00&maxdate=2006-07-05+06%3A00%3A00&cvsroot=%2Fcvsroot

So this is from Bug 342531. 

This looks invalid per Bug 342531 comment #5:
CSS2.1 "9.5 Floats" says that
"The margin box of a [...] block-level replaced element [...] must not
overlap any floats in the same block formatting context [...]" 
http://www.w3.org/TR/CSS21/visuren.html#floats
Opera 9.2x gives the FF2 rendering.  But Opera 9.5beta gives the FF3 rendering.  
+'ing this for investigation.  We have a good testcase and a regression range.  Web compat issue.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
(In reply to comment #4)

Except that the wording in CSS 2.1, 9.5 changed since that bug. It now reads:
'The border box of a table, a block-level replaced element, or an element in the normal flow that establishes a new block formatting context '

'border-box' instead of 'margin-box'

Keywords: regression, testcase
Yeah, we should probably switch to the border-box thing, for web compat reasons if nothing else.

I thought we had a bug on that already...
The impact looks pretty minor - given that an the change in CSS spec do we really need to block FF3 for this?
Given it's a layout regression on a real site that works in every other browser including FF2, I think we should block unless we decide the patch will be risky.
Assignee: nobody → dbaron
Attached patch patch (work in progress) (obsolete) — Splinter Review
Here's a patch that fixes this bug.

However, Boris has convinced me to make the change in bug 427782 comment 11, which will actually make this patch significantly simpler.

So after lunch I'll combine that with this patch, and then start testing the combination of the two.
I also need to be careful not to ignore negative margins.
Attached patch patch (work in progress) (obsolete) — Splinter Review
This fixes this bug and bug 427782.  I still need to add tests for things that weren't covered by the tweaks to existing tests.
Attachment #314917 - Attachment is obsolete: true
To be a little clearer, I think the changes to existing test references in attachment 314992 [details] [diff] [review] test bug 427782 adequately, but I still need to add reftests for the issue in this bug (which was never tested adequately either way in the first place, and was already inconsistent between % and non-% widths).
This patch doesn't correctly handling having a margin on the left and needing to clear a right float, since the margin width is ignored when WidthToClearPastFloats returns just a border-box width.
Attached patch patch (obsolete) — Splinter Review
So that made it a little more involved than I thought earlier, because it requires propagating the margin information through to where we do the clearing.  This does that.

I'd like to write a few more tests (tables, block-level forms, tables-with-captions), though -- the first two sets to double check that other browsers behave the same across the range of objects, the last set to test one of the hairy bits in this patch (which also updates the table caption code here as should have been done in bug 363248).
Attached patch patch (obsolete) — Splinter Review
Here's a version with tests for images and tables added, and with the changes needed to get the tables one to pass (which had some extra complexity since the table outer frame actually manages the margins).
Attachment #314992 - Attachment is obsolete: true
Attachment #315180 - Attachment is obsolete: true
Attached patch patchSplinter Review
So I still don't have a reference for the table caption test, but I'm happy enough with its behavior while I resize the window (which really requires that I add more tests), so I'm requesting review at this point.  (It produces pretty bad results for side captions, but related to existing bugs.)

In any case, the major changes here are:

  1) Clear the border-box rather than the margin-box of replaced elements.  (This fixes this bug / bug 377664.)

     * I add nsBlockReflowState::ReplacedBlockOffsetsForFloats to compute the offsets from the containing block edge for using for the available space; these offsets become nonzero when the floats doing the displacing become bigger than the margin of the box being displaced.

     * I make WidthToClearPastFloats return a struct with side margins and a border-box width.  The changes to do this for outer tables are a bit complicated, and also include changes that should have been part of bug 363248)

  2) clear the width that we compute, rather than the min width, so that we don't clear frames that can go below their min width.  (This fixes bug 427782.)

     * this is done by removing the min-width codepath in nsBlockFrame::WidthToClearPastFloats.  (This is good to fix here because updating that codepath for (1) would have been quite difficult.)


I also need to add some testcases for boxes with negative margins, both next to and not next to floats.
Attachment #315202 - Attachment is obsolete: true
Attachment #315324 - Flags: superreview?(roc)
Attachment #315324 - Flags: review?(roc)
+  void ReplacedBlockOffsetsForFloats(nsIFrame* aFrame,
+                                     nscoord& aLeftResult,
+                                     nscoord& aRightResult,
+                                     nsBlockFrame::ReplacedElementWidthToClear
+                                                     *aReplacedWidth = nsnull);

I'd prefer this to document the results and also use a verb name, e.g. ComputeReplacedblockOffsetsForFloats.

+      // FIXME:  This doesn't treat the caption and table independently,
+      // since we adjust by only the smaller margin, and the table outer
+      // frame doesn't know about it.
+      result.marginLeft  = PR_MIN(tableMargin.left,  captionMargin.left);
+      result.marginRight = PR_MIN(tableMargin.right, captionMargin.right);

Wouldn't it be safer to use PR_MAX?

    result.borderBoxWidth =
+      aFrame->ComputeSize(aState.mReflowState.rendContext,
+                          nsSize(aState.mContentArea.width,
+                                 NS_UNCONSTRAINEDSIZE),
+                          availWidth,
+                          nsSize(offsetState.mComputedMargin.LeftRight(),
+                                 offsetState.mComputedMargin.TopBottom()),
+                          nsSize(offsetState.mComputedBorderPadding.LeftRight() -
+                                   offsetState.mComputedPadding.LeftRight(),
+                                 offsetState.mComputedBorderPadding.TopBottom() -
+                                   offsetState.mComputedPadding.TopBottom()),
+                          nsSize(offsetState.mComputedPadding.LeftRight(),
+                                 offsetState.mComputedPadding.TopBottom()),
+                          PR_TRUE).width +
+      offsetState.mComputedBorderPadding.LeftRight() -
+      (result.marginLeft + result.marginRight);

I'm confused about subtracting result.marginLeft+result.marginRight here. If the table has, say, width:100px, won't we end up subtracting the margins from that to compute borderBoxWidth here? I know we added result.marginLeft+result.marginRight to availWidth so we need to subtract them in some cases, but not all. Or maybe I'm just confused.
(In reply to comment #20)
> +      result.marginLeft  = PR_MIN(tableMargin.left,  captionMargin.left);
> +      result.marginRight = PR_MIN(tableMargin.right, captionMargin.right);
> 
> Wouldn't it be safer to use PR_MAX?

The point here is that the margin is allowed to overlap with the float.  We don't want to let any border box overlap with the float, so we want the smaller margin.

> I'm confused about subtracting result.marginLeft+result.marginRight here. If
> the table has, say, width:100px, won't we end up subtracting the margins from
> that to compute borderBoxWidth here? I know we added
> result.marginLeft+result.marginRight to availWidth so we need to subtract them
> in some cases, but not all. Or maybe I'm just confused.

The problem is that the margins of the table and the caption live inside the outer frame -- thus when we get the border-box width of the outer frame (or content-box width -- I'm pretty sure they're always the same), it's really a margin-box width.  We have to subtract the margins to get back to a "real" border-box width.
Comment on attachment 315324 [details] [diff] [review]
patch

Right, OK then.
Attachment #315324 - Flags: superreview?(roc)
Attachment #315324 - Flags: superreview+
Attachment #315324 - Flags: review?(roc)
Attachment #315324 - Flags: review+
Comment on attachment 315324 [details] [diff] [review]
patch

This patch fixes this bug and bug 427782.  It's not risk-free, but it fixes two problems that I think we need to fix, and the test coverage is pretty good
Attachment #315324 - Flags: approval1.9?
...well, it's pretty good for the cases we've thought of, at least.  The most serious risk is probably the risk of the case that I haven't thought of, and it's hard to know what that is.  But that's no different from usual.
Attachment #315324 - Flags: approval1.9? → approval1.9+
(In reply to comment #23)
> (From update of attachment 315324 [details] [diff] [review])
> This patch fixes this bug and bug 427782.  It's not risk-free, but it fixes two
> problems that I think we need to fix, and the test coverage is pretty good
> 

Can we add a reftest for the cases this fixes?  
(In reply to comment #25)
> Can we add a reftest for the cases this fixes?  

That's already part of the patch (and the main reason the patch is so big).
This was checked in just now, but I guess David is waiting on something before marking it fixed.
Checked in to trunk, 2008-04-14 18:06 -0700.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
Blocks: 414558
You need to log in before you can comment on or make changes to this bug.