427129 - CSS margin problem at healthedco.com

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Daniel Veditz [:dveditz]]

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.

Comment 1

[Brian Polidoro]

Attachment: testcase

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.

Comment 2

[Brian Polidoro]

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.  

Comment 3

[Brian Polidoro]

Attachment: real testcase

Ok I cut one too many rules.  The display: Block rule is the one that makes the difference.

Comment 4

[Brian Polidoro]

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

Comment 5

[Brian Polidoro]

Opera 9.2x gives the FF2 rendering.  But Opera 9.5beta gives the FF3 rendering.  

Comment 6

[Damon Sicore (:damons)]

+'ing this for investigation.  We have a good testcase and a regression range.  Web compat issue.

Comment 7

[philippe (part-time)]

(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'

Comment 8

[Boris Zbarsky [:bzbarsky]]

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...

Comment 9

[Boris Zbarsky [:bzbarsky]]

Bug Bug 377664, to be exact.

Comment 10

[Mike Schroepfer]

The impact looks pretty minor - given that an the change in CSS spec do we really need to block FF3 for this?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 12

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

Attachment: patch (work in progress)

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.

Comment 13

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

I also need to be careful not to ignore negative margins.

Comment 14

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

Attachment: patch (work in progress)

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.

Comment 15

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

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).

Comment 16

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

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.

Comment 17

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

Attachment: patch

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).

Comment 18

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

Attachment: patch

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).

Comment 19

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

Attachment: patch

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.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 21

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

(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 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 315324 [details] [diff] [review]
patch

Right, OK then.

Comment 23

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

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

Comment 24

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

...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.

Comment 25

[Mike Schroepfer]

(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?  

Comment 26

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

(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).

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This was checked in just now, but I guess David is waiting on something before marking it fixed.

Comment 28

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

Checked in to trunk, 2008-04-14 18:06 -0700.