bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in Firefox 41

Status

()

Toolkit
Themes
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Depends on: 1 bug)

Trunk
mozilla41
x86
Mac OS X
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

3 years ago
See bug 1165308 comment 2. We should really fix this.
Points: --- → 2
Flags: firefox-backlog+
Summary: Pinstripe's global.css doesn't give description a margin → <description> doesn't have a margin on OS X
(Assignee)

Updated

3 years ago
Summary: <description> doesn't have a margin on OS X → <description> doesn't have the same margin as <label> on OS X
(Assignee)

Comment 2

3 years ago
Created attachment 8609572 [details] [diff] [review]
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.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #8609572 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

3 years ago
Iteration: --- → 41.1 - May 25
Flags: qe-verify-
(Assignee)

Comment 3

3 years ago
Created attachment 8609659 [details] [diff] [review]
patch

rebased
Attachment #8609572 - Attachment is obsolete: true
Attachment #8609572 - Flags: review?(gijskruitbosch+bugs)
Attachment #8609659 - Flags: review?(gijskruitbosch+bugs)

Comment 4

3 years ago
(apologies for the delay, I aim to review this tomorrow)
(Assignee)

Comment 5

3 years ago
Created attachment 8610712 [details] [diff] [review]
patch

rebased
Attachment #8609659 - Attachment is obsolete: true
Attachment #8609659 - Flags: review?(gijskruitbosch+bugs)
Attachment #8610712 - Flags: review?(gijskruitbosch+bugs)

Comment 6

3 years ago
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.
Attachment #8610712 - Flags: review?(gijskruitbosch+bugs) → feedback+
(Assignee)

Comment 7

3 years ago
Created attachment 8611170 [details] [diff] [review]
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.
Attachment #8610712 - Attachment is obsolete: true
Attachment #8611170 - Flags: review?(gijskruitbosch+bugs)

Comment 8

3 years ago
Comment on attachment 8611170 [details] [diff] [review]
patch v2

thanks!
Attachment #8611170 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Updated

3 years ago
Attachment #8611170 - Attachment description: patch → patch v2
Remember bug 1167937? You just got bit by it again. Backed out in https://hg.mozilla.org/integration/fx-team/rev/299a93ea3f84
Depends on: 1167937
(Assignee)

Comment 12

3 years ago
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...
(Assignee)

Comment 14

3 years ago
(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?
Flags: needinfo?(ehsan)

Comment 15

3 years ago
(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...
Flags: needinfo?(ehsan)
(Assignee)

Comment 16

3 years ago
(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.
(Assignee)

Comment 18

3 years ago
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

3 years ago
(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?
(Assignee)

Updated

3 years ago
Blocks: 1169606
https://hg.mozilla.org/mozilla-central/rev/6d88bce884dc
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1170500
You need to log in before you can comment on or make changes to this bug.