Last Comment Bug 709014 - side margins ignored on inline-blocks with 0 width (for left margins, zero height also required)
: side margins ignored on inline-blocks with 0 width (for left margins, zero he...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: 8 Branch
: All All
: -- normal with 1 vote (vote)
: mozilla30
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
:
Mentors:
: 978022 (view as bug list)
Depends on:
Blocks: 978022
  Show dependency treegraph
 
Reported: 2011-12-09 04:20 PST by fox
Modified: 2014-04-30 11:33 PDT (History)
6 users (show)
dbaron: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard: [good first verify]
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
simple_with_width_0.html (852 bytes, text/html)
2011-12-09 04:20 PST, fox
no flags Details
Reference rendering without the problematic elements (552 bytes, text/plain)
2011-12-09 04:21 PST, fox
no flags Details
Reference rendering without the problematic elements (552 bytes, text/html)
2011-12-09 04:21 PST, fox
no flags Details
patch 1: Honor margin-left and margin-right on elements in inline layout that have 0 width and/or height (commonly, inline-blocks). (14.58 KB, patch)
2014-02-16 21:35 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
roc: review+
mats: feedback+
Details | Diff | Review
diff -b of the nsLineLayout.cpp changes (1.94 KB, patch)
2014-02-17 08:18 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review

Description fox 2011-12-09 04:20:03 PST
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.
Comment 1 fox 2011-12-09 04:21:09 PST
Created attachment 580367 [details]
Reference rendering without the problematic elements
Comment 2 fox 2011-12-09 04:21:48 PST
Created attachment 580368 [details]
Reference rendering without the problematic elements
Comment 3 fox 2011-12-09 04:24:26 PST
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 2011-12-09 04:37:21 PST
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] 2011-12-09 05:34:20 PST
(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-7 (review requests must explain patch) 2014-02-16 13:17:40 PST
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-7 (review requests must explain patch) 2014-02-16 18:56:45 PST
https://tbpl.mozilla.org/?tree=Try&rev=c87d3fdeeef3
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2014-02-16 21:34:15 PST
https://tbpl.mozilla.org/?tree=Try&rev=e9259db21588
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2014-02-16 21:35:31 PST
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.
Comment 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2014-02-16 21:37:26 PST
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-7 (review requests must explain patch) 2014-02-17 08:18:46 PST
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 12 Mats Palmgren (:mats) 2014-02-17 09:12:37 PST
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) (Exited; email my personal email if necessary) 2014-02-17 18:14:28 PST
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-7 (review requests must explain patch) 2014-02-17 20:01:41 PST
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-7 (review requests must explain patch) 2014-02-17 20:11:20 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/83a2b19f7492
Comment 16 Carsten Book [:Tomcat] 2014-02-18 05:03:50 PST
https://hg.mozilla.org/mozilla-central/rev/83a2b19f7492
Comment 17 Mats Palmgren (:mats) 2014-03-04 06:34:52 PST
*** Bug 978022 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.