1059167 - Right floated element with defined width doesn't respect right margin

Status: RESOLVED FIXED

(Core :: Layout: Floats, defect, P2)

Description

[Oleg Zasypkin [:azasypkin]]

Attachment: test-case.html

While working on bug 1041765, we've spotted blocking Gecko regression - right floated elements can't have margin-right (see attached test case).

Everything is fine for left floated elements. For right floated elements margin has effect only if element has "width: auto;".

The issue DOES occur on the latest Nightly and B2G, and DOES NOT occur on the week-old builds.

Comment 1

[Oleg Zasypkin [:azasypkin]]

[Blocking Requested - why for this release]:
Blocks v2.1 blocker

Comment 2

[Boris Zbarsky [:bzbarsky]]

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=daa84204a11a&tochange=dc352a7bf234

Comment 3

[Mats Palmgren (inactive)]

First changeset that fails the test:

changeset:   201250:54ada5ad66bb
user:        Jonathan Kew <[e-mail]>
date:        Sun Aug 24 15:34:44 2014 +0100
summary:     bug 1046950 pt 2 - convert ComputeSize to use logical-coordinate parameters. r=smontagu

Comment 4

[Boris Zbarsky [:bzbarsky]]

This is a regression from bug 1046950 as far as I can tell.

Specifically, in http://hg.mozilla.org/mozilla-central/rev/54ada5ad66bb the code in FloatMarginWidth() was changed from passing the margin, border, and padding to ComputeSize() to passing margin, borderpadding, and padding.  So the padding gets double-counted as far as I can tell.  Removing the padding style in the testcase makes the margin work correctly.

Comment 5

[Jonathan Kew [:jfkthame]]

Yes, it's a regression; oops. Will fix and test.

(FWIW, removing "box-sizing: border-box;" from the testcase style also avoids the issue.)

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: fix accidental regression in FloatMarginWidth.

This was an unintended change during logical-conversion; we weren't supposed to drop the subtraction of the padding here. I'll see about writing a reftest that would've caught this...

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: reftest.

Simple reftest based on the testcase here.

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: reftest.

It works better to actually include the test files in the patch.

Comment 9

[Jonathan Kew [:jfkthame]]

Tryserver job with the reftest: https://tbpl.mozilla.org/?tree=Try&rev=dfc8e92c5ae0

Comment 11

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a44566b9890b
https://hg.mozilla.org/integration/mozilla-inbound/rev/228f0275bfc8

Comment 14

[Daniel Holbert [:dholbert]]

FWIW: The newrelic blog (http://blog.newrelic.com/ , http://blog.newrelic.com/2014/08/26/security-document/ ) is broken in current Nightly, I think by this bug.  (main text content overflowing & getting clipped on the right side)

Glad that this is fixed on inbound; hopefully that means the newrelic blog will be working again tomorrow or the next day. :)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a44566b9890b
https://hg.mozilla.org/mozilla-central/rev/228f0275bfc8