459563 - <description> doesn't have the same margin as <label> on OS X

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Description

[Dão Gottwald [:dao]]

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.

Comment 1

[Dão Gottwald [:dao]]

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

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

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.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

rebased

Comment 4

[:Gijs (he/him)]

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

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch

rebased

Comment 6

[:Gijs (he/him)]

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.

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch v2

(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 8

[:Gijs (he/him)]

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

thanks!

Comment 9

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/1662253c88f7

Comment 10

[Phil Ringnalda (:philor)]

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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/cab24498e095

Comment 12

[Dão Gottwald [:dao]]

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

Comment 13

[Dão Gottwald [:dao]]

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

Comment 14

[Dão Gottwald [:dao]]

(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?

Comment 15

[(no longer active)]

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

Comment 16

[Dão Gottwald [:dao]]

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

Comment 17

[Phil Ringnalda (:philor)]

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.

Comment 18

[Dão Gottwald [:dao]]

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.

Comment 19

[:Gijs (he/him)]

(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?

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/6d88bce884dc

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d88bce884dc