Last Comment Bug 376365 - update margin collapsing to slight rule change
: update margin collapsing to slight rule change
Status: ASSIGNED
[not-ready-for-cedar]
:
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: fantasai
:
:
Mentors:
http://dbaron.org/css/test/2007/0329-...
Depends on: 50959 post2.0
Blocks: css2.1-tests
  Show dependency treegraph
 
Reported: 2007-04-03 09:27 PDT by David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
Modified: 2011-05-09 10:15 PDT (History)
9 users (show)
roc: wanted‑next+
dbaron: blocking1.9-
roc: wanted1.9-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
suggested patch (921 bytes, patch)
2007-04-24 16:22 PDT, fantasai
roc: review+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2007-04-03 09:27:33 PDT
There's a slight change to the margin collapsing rules in http://lists.w3.org/Archives/Member/w3c-css-wg/2007JanMar/0535.html that's been accepted for the next draft of CSS2.1.  We should implement this.

Steps to reproduce:
 load http://dbaron.org/css/test/2007/0329-blog-examples/1

Actual results:
 hello is the same distance below the blue block as A is above it

Expected results:
 there should be a gap between hello and the blue block so hello is 50px below A
Comment 1 fantasai 2007-04-24 16:22:04 PDT
Created attachment 262698 [details] [diff] [review]
suggested patch

I'm pretty sure that conditional fixes the bug. I'm less sure about whether it's the right place to put the conditional.
Comment 2 fantasai 2007-04-24 16:23:57 PDT
My build still passes http://www.hixie.ch/tests/adhoc/css/box/block/margin-collapse/microsoft/all.html with flying colors (except 005.html, of course, since the test is now wrong). I also ran the reftests and nothing new failed.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-04-25 17:07:50 PDT
I probably should be the reviewer...

Shouldn't the fix be after "clearance = aState.mY - (currentY + topMargin);"?

+      clearance -= clearance + incomingMargin.get();

This is equivalent to "clearance = -incomingMargin.get();" Can you explain why this is correct? I don't see it. I think it would be better if we explicitly did the calculation described in Hixie's email.
Comment 4 fantasai 2007-04-25 19:20:51 PDT
> Shouldn't the fix be after "clearance = aState.mY - (currentY + topMargin);"?

That's where I thought it should go, but it didn't work there. I spent hours trying to figure out why, but my programming fu wasn't strong enough. Even setting clearance to random numbers there didn't break my testcases. -__-;; I don't understand at all.

> Can you explain why this is correct?

I can try to explain why I *think* it's correct...

The point of the spec fix is that clearance should never move an element *up*.
It only moves an element up if it's negative, hence the check for negativeness.
It also only moves the element up if it winds up overcompensating for the hypothetical margin collapse. The margin can't collapse more than the amount of the previous margin, so if the magnitude of negative clearance is more than that, then it's overcompensating, and it's overcompensating by
  |clearance| - incomingMargin
which, given that clearance is negative, is the same as
  clearance + incomingMargin
Subtract out the overcompensation, and the element no longer moves up.

That's my logic. Hyatt thinks its wrong, so maybe I'm missing something important here. Hence "suggested patch" rather than "proposed patch" and a request for dbaron's review since he's more of a margin collapsing guru than I am.
Comment 5 fantasai 2007-04-25 19:43:21 PDT
Note that, if my logic serves me correctly, if a negative margin is one (or both) of two margins collapsing, then the amount of the collapse must be non-negative. A  negative margin collapsing with a positive one is the sum of the two (no difference if they collapse or don't collapse), and two negative margins collapsing are the max of the two, i.e. the difference between not collapsing and collapsing is positive. Therefore (correct me if I'm wrong) clearance is only negative if it's compensating for a positive-positive collapse.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2008-12-26 11:43:12 PST
Comment on attachment 262698 [details] [diff] [review]
suggested patch

I should have transferred this review request ages ago.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-03-01 17:15:39 PST
Comment on attachment 262698 [details] [diff] [review]
suggested patch

I kinda forgot about this. I think it's right, though.

We should have a reftest.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-11-24 13:48:05 PST
And updated version of this patch is:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/7c922ff64911/clearance-should-not-move-up
and it fixes our handling of the following tests in the CSS 2.1 test suite:
http://test.csswg.org/suites/css2.1/20101027/html4/margin-collapse-clear-005.htm
http://test.csswg.org/suites/css2.1/20101027/html4/margin-collapse-clear-011.htm
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-11-24 13:50:06 PST
Do you want to land it for FF4?
Comment 10 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-11-24 14:13:03 PST
I tend to think not; I'd rather land it afterwards.

Also see:
http://lists.w3.org/Archives/Public/public-css-testsuite/2010Nov/0124.html
Comment 11 fantasai 2010-11-29 18:46:31 PST
I'll note that the CSS2.1 spec could get blocked on whether this fix breaks web compat, and currently only Mozilla seems capable of figuring that out within the next year or two.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-25 10:12:41 PST
This seems to break Acid2.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-25 16:25:13 PST
This also fixes http://test.csswg.org/suites/css2.1/20110111/html4/margin-collapse-164.htm
Comment 14 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-25 17:03:56 PST
So apparently:
 * the IE folks say fixing this breaks some sites
 * the tests are actually invalid, although only unintentionally so, since "hypothetical position" is defined **relative to the parent**

So it sounds like we might fix the tests.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-01-25 17:15:48 PST
Though http://lists.w3.org/Archives/Member/w3c-css-wg/2007JanMar/0514.html shows that the explicit intent of the discussion was to fix the test that has now become http://test.csswg.org/suites/css2.1/20101027/html4/margin-collapse-clear-005.htm
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2011-03-28 08:42:11 PDT
Sounds like this patch is not ok to go on m-c, right?  If I'm wrong, please let me know!

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