249982 - percentage margins relative to float width not CB width if width:auto

Status: VERIFIED FIXED

(Core :: Layout: Floats, defect)

Description

[Hixie (not reading bugmail)]

If you have a width:auto float, then percentage margins are relative to the
actual width of the float, not its containing block width.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This happens for any shrink-wrapping block, actually.  It looks to me like the
code at the end of nsBlockReflowContext::ReflowBlock that does:

  // If the block is shrink wrapping its width, then see if we have percentage
  // based margins. If so, we can calculate them now that we know the shrink
  // wrap width
  if (NS_SHRINKWRAPWIDTH == aFrameRS.mComputedWidth) {
    ComputeShrinkwrapMargins(aFrameRS.mStyleMargin, mMetrics.width, mMargin, mX);
  }

is completely bogus (and has been since troy checked it in sometime in 2000). 
If the block being reflown is shrink-wrapping, its _kids_ need to use
ComputeShrinkWrapMargins; it itself does not need to.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase to not regress

In this testcase, the black square should be half the width of the red
rectangle.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

With this patch, the "testcase to not regress" is not regressed, and this bug
is fixed.  Now that we're not changing mX anymore (that part was pretty bogus
too, in my opinion), we no longer need the last arg of
ComputeShrinkwrapMargins.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153269 [details] [diff] [review]
Proposed patch

David, what do you think?

Comment 5

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

I'm suspicious of this change.  mComputeMaximumWidth isn't the normal way of
computing maximum width, so I think there has to be a third caller somewhere
(until reflow refactoring), for maximum width computation using
NS_UNCONSTRAINEDSIZE, under some condition.  Did you run the regression tests?

Comment 6

[Boris Zbarsky [:bzbarsky]]

I haven't yet; I need to clear up a gig of disk space first.... But I'll try to
do that today.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, I finished running the regression tests.  There weren't any failures (other
than the false positives).

If you'd prefer to hold off on this till after reflow refactoring, I guess I'm
OK with that.

Comment 8

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

Comment on attachment 153269 [details] [diff] [review]
Proposed patch

As long as you don't make http://dbaron.org/css/test/intrinsic/perc-marg-max
and http://dbaron.org/css/test/intrinsic/perc-marg-min any worse (the're not so
great already), r+sr=dbaron.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153269 [details] [diff] [review]
Proposed patch

Ah, there we go.  That patch does make the testcase at
http://dbaron.org/css/test/intrinsic/perc-marg-max worse (the margins compute
to 0, looks like).  The testcase at
http://dbaron.org/css/test/intrinsic/perc-marg-min is not affected (and is
really bad, yes).

So it looks like this is no good as things stand -- 
we can't easily tell whether the width is shrink-wrap because it really is or
because the parent is shrink-wrapping... :(

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: As discussed on IRC

This computes the shrinkwrap margins only if our _parent_ is shrinkwrapping,
instead of if we're shrinkwrapping.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153739 [details] [diff] [review]
As discussed on IRC

Where by "parent" I mean "containing block", since that's what the percent
margins should be using to size.

Comment 12

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

Unfortunately you need to do the equivalent for the other two callers (and I'm
not really sure how one would do that right now) -- particularly the max-width
one, since this will cause {inc} bugs without such a change.

That said, maybe the best solution is to move this code where it belongs, and
add the margins at the parent, just like all other margins are added (right?).

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153739 [details] [diff] [review]
As discussed on IRC

Hmm.. Yeah, margins are generally added at the parent (though most just add
whatever the reflow state computed margin is).

You're right that this has {inc} issues..

Comment 14

[Erik Fabert]

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

Comment 15

[philippe (part-time)]

Based on the test case:
fails with the 20061207 build
works with the 20061298 build

Bug 300030 fixed this, I think.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yeah, it would have.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Attachment: Reftests

Comment 18

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

Comment on attachment 257619 [details] [diff] [review]
Reftests

Checked in after testing in pre- and post-reflow-branch builds.