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

RESOLVED FIXED in Firefox 34

Status

()

Core
Layout: Floats
P2
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: azasypkin, Assigned: jfkthame)

Tracking

({regression, testcase})

unspecified
mozilla34
regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox31 unaffected, firefox32 unaffected, firefox33 unaffected, firefox34+ fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Created attachment 8479737 [details]
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.
(Reporter)

Comment 1

4 years ago
[Blocking Requested - why for this release]:
Blocks v2.1 blocker
blocking-b2g: --- → 2.1?
First changeset that fails the test:

changeset:   201250:54ada5ad66bb
user:        Jonathan Kew <jkew@mozilla.com>
date:        Sun Aug 24 15:34:44 2014 +0100
summary:     bug 1046950 pt 2 - convert ComputeSize to use logical-coordinate parameters. r=smontagu
Blocks: 1046950
Keywords: testcase
OS: Linux → All
Priority: -- → P2
Hardware: x86_64 → All
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.
Flags: needinfo?(smontagu)
Flags: needinfo?(jfkthame)
Keywords: testcase
OS: All → Linux
Priority: P2 → --
Hardware: All → x86_64
Keywords: testcase
OS: Linux → All
Priority: -- → P2
Hardware: x86_64 → All
(Assignee)

Comment 5

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

(FWIW, removing "box-sizing: border-box;" from the testcase style also avoids the issue.)
Flags: needinfo?(smontagu)
Flags: needinfo?(jfkthame)
(Assignee)

Comment 6

4 years ago
Created attachment 8479906 [details] [diff] [review]
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...
Attachment #8479906 - Flags: review?(smontagu)
(Assignee)

Updated

4 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 7

4 years ago
Created attachment 8479920 [details] [diff] [review]
reftest.

Simple reftest based on the testcase here.
Attachment #8479920 - Flags: review?(smontagu)
(Assignee)

Comment 8

4 years ago
Created attachment 8480048 [details] [diff] [review]
reftest.

It works better to actually include the test files in the patch.
Attachment #8480048 - Flags: review?(smontagu)
(Assignee)

Updated

4 years ago
Attachment #8479920 - Attachment is obsolete: true
Attachment #8479920 - Flags: review?(smontagu)
status-firefox31: --- → unaffected
status-firefox32: --- → unaffected
status-firefox33: --- → unaffected
status-firefox34: --- → affected
tracking-firefox34: ? → +
(Assignee)

Comment 9

4 years ago
Tryserver job with the reftest: https://tbpl.mozilla.org/?tree=Try&rev=dfc8e92c5ae0
Duplicate of this bug: 1059633
Attachment #8479906 - Flags: review?(smontagu) → review+
Attachment #8480048 - Flags: review?(smontagu) → review+

Updated

4 years ago
Duplicate of this bug: 1059845
Duplicate of this bug: 1059809
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. :)
https://hg.mozilla.org/mozilla-central/rev/a44566b9890b
https://hg.mozilla.org/mozilla-central/rev/228f0275bfc8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox34: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED

Updated

4 years ago
blocking-b2g: 2.1? → ---
You need to log in before you can comment on or make changes to this bug.