See bug 459457 comment 4, 5, and 7: "The old (Moz/SeaMonkey) Mac Classic formatting.css gave description a margin since before it was even called description" Win/Gnomestripe give description the same margin as label, except for a larger bottom margin.

See bug 1165308 comment 2. We should really fix this.

Brings OS X in line with Windows and Linux. I'm also handling a few cases where we worked around this bug, but there are likely more. I think those are best dealt with incrementally as we discover them, unless you already notice something obviously wrong when reviewing this.

rebased

(apologies for the delay, I aim to review this tomorrow)

rebased

Comment on attachment 8610712 [details] [diff] [review] patch Review of attachment 8610712 [details] [diff] [review]: ----------------------------------------------------------------- The learn more link in the tracking protection doorhanger is now misaligned with the rest of the contents of the notification. ::: toolkit/themes/osx/global/global.css @@ -197,2 @@ > .header { > - margin-bottom: 6px; This is confusing to me - the bottom margin is not 6px now, is it? This reduces the vertical spacing between e.g. the headers in the certificate viewer and the items underneath them.

(In reply to :Gijs Kruitbosch from comment #6) > The learn more link in the tracking protection doorhanger is now misaligned > with the rest of the contents of the notification. Indeed, and it was already misaligned on Linux and Windows, because apparently this was tailored to OS X. Another symptom of this bug. > ::: toolkit/themes/osx/global/global.css > @@ -197,2 @@ > > .header { > > - margin-bottom: 6px; > > This is confusing to me - the bottom margin is not 6px now, is it? This > reduces the vertical spacing between e.g. the headers in the certificate > viewer and the items underneath them. Yep, but it should be in line with Windows and Linux. I can set it to 6px for all platforms if you think that's better, but either way let's stop treating OS X differently for no particular reason.

Comment on attachment 8611170 [details] [diff] [review] patch v2 thanks!

Remember bug 1167937? You just got bit by it again. Backed out in https://hg.mozilla.org/integration/fx-team/rev/299a93ea3f84

Good lord, another 10.6-only failure: TEST-UNEXPECTED-FAIL | layout/xul/test/test_bug477754.xul | RTL popup's right offset should be equal to the x offset passed to openPopup - got 11, expected 10 What the hell is going on with that OS...

Backed out: https://hg.mozilla.org/integration/fx-team/rev/9bd7e886acd3

(In reply to Dão Gottwald [:dao] from comment #12) > Good lord, another 10.6-only failure: > > TEST-UNEXPECTED-FAIL | layout/xul/test/test_bug477754.xul | RTL popup's > right offset should be equal to the x offset passed to openPopup - got 11, > expected 10 Ehsan, you wrote this test, any idea why my patch could make it unhappy on OS X 10.6? My patch slightly changes the margin we apply to <label> on OS X (to be in line with Windows and Linux), and the test uses two label elements. Is the xul popup code broken or is the test fragile?

(In reply to Dão Gottwald [:dao] from comment #14) > (In reply to Dão Gottwald [:dao] from comment #12) > > Good lord, another 10.6-only failure: > > > > TEST-UNEXPECTED-FAIL | layout/xul/test/test_bug477754.xul | RTL popup's > > right offset should be equal to the x offset passed to openPopup - got 11, > > expected 10 > > Ehsan, you wrote this test, any idea why my patch could make it unhappy on > OS X 10.6? My patch slightly changes the margin we apply to <label> on OS X > (to be in line with Windows and Linux), and the test uses two label > elements. Is the xul popup code broken or is the test fragile? Your patch reduces the end-margin of label elements by 1px (from 6px to 5px), so the 1px difference in the result can be explained. What I don't understand is why this failure only happens on OSX. Do you mind investigating the value of the screenPoint, anchorRect and anchorXOffset variables here on two different OSX versions? <https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsMenuPopupFrame.cpp#1341> I would expect all of them to be the same across different OSX versions...

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #15) > Do you mind > investigating the value of the screenPoint, anchorRect and anchorXOffset > variables here on two different OSX versions? > <https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsMenuPopupFrame. > cpp#1341> I don't have OS X locally, not sure how I would go about this on the try server...

You don't yet know whether that test_bug477754.xul failure was actually 10.6-only, because we're reducing test load with SETA, the Search For "Extraneous" Testing, which means we only run the full set of tests on every platform once every something like 6 hours or 10 pushes, so the entire time you were in the tree we didn't run 10.10 oth. If you've pushed to try, you probably didn't run on 10.10 there either, because it's not part of -p all -u all. You either need "try: -b do -p macosx64 -u mochitest-o[10.6,10.10] -t none" or if you're doing an all push "try: -b do -p all -u all[-platypus] -t none" to get 10.10 to run.

I'm gonna try landing this with margin: 1px 6px 2px; instead of -moz-margin-start: 6px; / -moz-margin-end: 5px;. This leaves OS X a bit off compared to Windows and Linux, but it's still much closer than what we have today.

(In reply to Dão Gottwald [:dao] from comment #18) > I'm gonna try landing this with margin: 1px 6px 2px; instead of > -moz-margin-start: 6px; / -moz-margin-end: 5px;. This leaves OS X a bit off > compared to Windows and Linux, but it's still much closer than what we have > today. Can you file a followup bug and needinfo me so I can investigate comment #15 locally?