Closed Bug 709014 Opened 10 years ago Closed 8 years ago

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

Categories

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

8 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: fox, Assigned: dbaron)

References

Details

Attachments

(4 files, 1 obsolete file)

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.
Attachment #580367 - Attachment is obsolete: true
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.
Component: General → Layout: Block and Inline
Product: Firefox → Core
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
Attachment #580366 - Attachment mime type: text/plain → text/html
Attachment #580368 - Attachment mime type: text/plain → text/html
(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").
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: nobody → dbaron
Status: NEW → ASSIGNED
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)
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)
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+
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).
https://hg.mozilla.org/mozilla-central/rev/83a2b19f7492
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Blocks: 978022
Duplicate of this bug: 978022
QA Whiteboard: [good first verify]
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.