side margins ignored on inline-blocks with 0 width (for left margins, zero height also required)

RESOLVED FIXED in mozilla30

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: fox, Assigned: dbaron)

Tracking

8 Branch
mozilla30
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 580366 [details]
simple_with_width_0.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/535.2 (KHTML, like Gecko) Chrome/15.0.874.121 Safari/535.2

Steps to reproduce:

While trying to answer this question on StackOverflow : http://stackoverflow.com/questions/8435834/ensuring-similar-block-height-of-horizontally-aligned-divs-with-unknown-content/8438954#8438954 I observed a bug in element sizing/positioning.

The bug appears when alternating normally sized inline-block elements (width=98px) with elements that have width=0, margin-left: -100px; margin-right: 100px;


Actual results:

Firefox places all elements at nearly the same position.


Expected results:

The non-"width=0" elements should have been displayed one after another.

Two things to note :
 - The display when the bug appears is nearly the same as what would have hapenned if I didn't use a positive margin right to compensate for the negative margin left.
 - The bug is specific to width=0 as could be seen in the sample files simply using width=1 fixes the problem.
(Reporter)

Comment 1

5 years ago
Created attachment 580367 [details]
Reference rendering without the problematic elements
(Reporter)

Comment 2

5 years ago
Created attachment 580368 [details]
Reference rendering without the problematic elements
Attachment #580367 - Attachment is obsolete: true
(Reporter)

Comment 3

5 years ago
I can't seem to find how to have a clean attachment that could be opened directly in the browser. I should have uploaded a zip file.

Anyway the reference rendering show the base status before adding the problematic elements and the other file show what happens after. Changing "width: 0" with "width: 1px" in the css show a drastic change in rendering.
(Reporter)

Updated

5 years ago
Component: General → Layout: Block and Inline
Product: Firefox → Core
(Reporter)

Comment 4

5 years ago
As I did some tests with other browsers here are the results :
 - Aurora 10.0a2 (2011-12-08) BUG
 - Opera 11.50 OK
 - IE 9.0.8112 OK
 - Chrome 15.0.874.121 m OK
 - Safari 5.1.2 (7534.52.7) OK

Updated

5 years ago
Attachment #580366 - Attachment mime type: text/plain → text/html

Updated

5 years ago
Attachment #580368 - Attachment mime type: text/plain → text/html

Comment 5

5 years ago
(In reply to fox from comment #3)
> I can't seem to find how to have a clean attachment that could be opened
> directly in the browser. I should have uploaded a zip file.

Fixed (changed the MIME type in "Details").
(Assignee)

Comment 6

3 years ago
It seems like the underlying problem here is:
 * we ignore margin-left on an inline-block when it has height 0 *and* width 0
 * we ignore margin-right on an inline-block when it has width 0
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Summary: width=0 on an inline-block with a negative left margin collapse all other inline-blocks to width=0 → side margins ignored on inline-blocks with 0 width (for left margins, zero height also required)
(Assignee)

Updated

3 years ago
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
(Assignee)

Comment 7

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=c87d3fdeeef3
(Assignee)

Comment 8

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=e9259db21588
(Assignee)

Comment 9

3 years ago
Created attachment 8376938 [details] [diff] [review]
patch 1:  Honor margin-left and margin-right on elements in inline layout that have 0 width and/or height (commonly, inline-blocks).

Prior to this patch, we failed to honor:
 * margin-left on elements in inline layout with 0 width and 0 height
 * margin-right on elements in inline layout with 0 width
I think that was because the code in CanPlaceFrame to discard both
margins when the width was 0 was running after the left-margin was
applied, unless the later code in PlaceFrame (checking both width 0 and
height 0) un-applied that left margin.

The assertion count change in test_value_computation.html is due to 2
additional "bad width" assertions (I presume from honoring large
margins that were previously ignored).

The change to 538935-1-ref.html is to match an improvement in rendering
of the margins in the test, where both sides of the margin are now
honored.

The change to layout/reftests/text-overflow/marker-basic-ref.html is to
keep the reference (which uses margins) rendering the same way following
the changes to margin handling.

The new behavior (in the reftests added in layout/reftests/inline/)
matches at least Chromium; I didn't check any other browsers.
Attachment #8376938 - Flags: review?(roc)
(Assignee)

Comment 10

3 years ago
Comment on attachment 8376938 [details] [diff] [review]
patch 1:  Honor margin-left and margin-right on elements in inline layout that have 0 width and/or height (commonly, inline-blocks).

Mats, if you're interested in looking at this change:
>The change to layout/reftests/text-overflow/marker-basic-ref.html is to
>keep the reference (which uses margins) rendering the same way following
>the changes to margin handling.
to check that it makes sense to you, you're welcome to do so.  But it's also fine if you don't want to bother.
Attachment #8376938 - Flags: feedback?(matspal)
(Assignee)

Comment 11

3 years ago
Created attachment 8377178 [details] [diff] [review]
diff -b of the nsLineLayout.cpp changes

This should be clearer, though interdiff refuses to give more than 1 line of context.
Comment on attachment 8376938 [details] [diff] [review]
patch 1:  Honor margin-left and margin-right on elements in inline layout that have 0 width and/or height (commonly, inline-blocks).

The change to marker-basic-ref.html looks fine to me.
Attachment #8376938 - Flags: feedback?(matspal) → feedback+
Comment on attachment 8376938 [details] [diff] [review]
patch 1:  Honor margin-left and margin-right on elements in inline layout that have 0 width and/or height (commonly, inline-blocks).

Review of attachment 8376938 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsLineLayout.cpp
@@ +1298,5 @@
> +  // Count the number of non-placeholder frames on the line...
> +  if (pfd->mFrame->GetType() == nsGkAtoms::placeholderFrame) {
> +    NS_ASSERTION(pfd->mBounds.width == 0 && pfd->mBounds.height == 0,
> +                 "placeholders should have 0 width/height (checking "
> +                 "equivalence with old code in this function)");

The comment in the assertion isn't quite correct; the new code isn't equivalent (zero-size frames that aren't placeholders will now be counted).

I guess this will make some testcases behave differently but I haven't been able to come up with one...
Attachment #8376938 - Flags: review?(roc) → review+
(Assignee)

Comment 14

3 years ago
I'll change it to:
    NS_ASSERTION(pfd->mBounds.width == 0 && pfd->mBounds.height == 0,
                 "placeholders should have 0 width/height (checking "
                 "placeholders were never counted by the old code in "
                 "this function)");

I agree it changes behavior in some edge cases, but I'd rather have the behavior be less quirky, and it seems obscure enough that it's not even worth testing other browsers to see if they might match that quirkiness (especially given that they don't on the more visible behavior here).
(Assignee)

Comment 15

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/83a2b19f7492
https://hg.mozilla.org/mozilla-central/rev/83a2b19f7492
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

Updated

3 years ago
Blocks: 978022

Updated

3 years ago
Duplicate of this bug: 978022
QA Whiteboard: [good first verify]
(Assignee)

Updated

3 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.