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

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[fox]

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

Comment 1

[fox]

Attachment: Reference rendering without the problematic elements

Comment 2

[fox]

Attachment: Reference rendering without the problematic elements

Comment 3

[fox]

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.

Comment 4

[fox]

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

Comment 5

[Paul Rouget [:paul]]

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

Comment 6

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

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

Comment 7

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

https://tbpl.mozilla.org/?tree=Try&rev=c87d3fdeeef3

Comment 8

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

https://tbpl.mozilla.org/?tree=Try&rev=e9259db21588

Comment 9

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

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

Comment 10

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

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.

Comment 11

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

Attachment: diff -b of the nsLineLayout.cpp changes

This should be clearer, though interdiff refuses to give more than 1 line of context.

Comment 12

[Mats Palmgren (inactive)]

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.

Comment 13

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

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

Comment 14

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

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

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/83a2b19f7492

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/83a2b19f7492