Closed Bug 1169606 Opened 9 years ago Closed 9 years ago

<description> and <label> should have -moz-margin-end: 5px;

Categories

(Toolkit :: Themes, defect)

Unspecified
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: dao, Assigned: Gijs)

References

Details

Attachments

(1 file)

Because of a bug involving test_bug477754.xul, I had to keep the end margin at 6px. On Windows and Linux it's 5px.

See bug 459563 comment 15:

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #15)
> (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?(gijskruitbosch+bugs)
The answer is that the test has never worked particularly well and it's now fallen victim to a rounding error. It's kind of lucky I managed to reproduce. Here's the values that Ehsan asked about:

a successful run:

Offset: screenpoint: 28843, anchorRect: 31496, anchorOffset: 600
Offset becomes: screenpoint: 28243, anchorRect: 30896, anchorOffset: 600
Offset: screenpoint: 28843, anchorRect: 31496, anchorOffset: 600
Offset becomes: screenpoint: 28243, anchorRect: 30896, anchorOffset: 600
2 INFO TEST-PASS | layout/xul/test/test_bug477754.xul | RTL popup's right offset should be equal to the x offset passed to openPopup 

(I don't know why we run this code twice, but nevermind that for now)

a broken run:

Offset: screenpoint: 28873, anchorRect: 31466, anchorOffset: 600
Offset becomes: screenpoint: 28273, anchorRect: 30866, anchorOffset: 600
Offset: screenpoint: 28873, anchorRect: 31466, anchorOffset: 600
Offset becomes: screenpoint: 28273, anchorRect: 30866, anchorOffset: 600
2 INFO 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

Note how the calculations here make sense, and the difference between the two runs is 30, not 60 (ie half a pixel instead of a full pixel, because these are CSS units). Presumably this is because the label's margins are now 1px smaller, and that difference is spread equally because the label is centered in an hbox, and so the X has shifted by half a pixel.

So we turn to the test, which in fact does this:

      is(Math.round(testAnchor.getBoundingClientRect().right) -
         Math.round(testPopup.getBoundingClientRect().right), 10,
         "RTL popup's right offset should be equal to the x offset passed to openPopup");

well, ok, but what are we rounding, exactly? Those values above don't look particularly divisible by 60 or 30 or anything close. On my machine, the answer from an info() statement in the test was:

2 INFO 556.5666809082031
3 INFO 546.3500061035156

Apologies for JS-induced rounding errors. Note how one thing is ever so slightly < 546.5 and the other slightly bigger than 556.56.


I think the test should be:

is(Math.round(testAnchor.getBoundingClientRect().right -
              testPopup.getBoundingClientRect().right), 10,
   "RTL popup's right offset should be equal to the x offset passed to openPopup");

which, with those values, would pass just fine, though part of me wonders where the discrepancy of 0.21667-odd (aka 13 CSS units) comes from.

Ehsan, is that right?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(ehsan)
In a passing run with the pre-existing margins, the info() prints:

2 INFO 557.0666809082031
3 INFO 546.8500061035156

so the discrepancy predates the changes here. We were just rounding better before than we are now.
Bug 1169606 - fix OSX margin for label/description and fix test rounding so it doesn't go orange, r?ehsan
Attachment #8612985 - Flags: review?(ehsan)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=538799b43b55

I'm hoping this was the right try incantation, including for 10.6 as well as 10.8 builds.
Your patch isn't actually using -moz-margin-end: 5px;
(In reply to Dão Gottwald [:dao] from comment #5)
> Your patch isn't actually using -moz-margin-end: 5px;

Gah, this is because I wanted to get the details that ended up in comment #2.
Comment on attachment 8612985 [details]
MozReview Request: Bug 1169606 - fix OSX margin for label/description and fix test rounding so it doesn't go orange, r?ehsan

Bug 1169606 - fix OSX margin for label/description and fix test rounding so it doesn't go orange, r?ehsan
New trypush: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a4a7d1c1b33

(which, thanks to autoland, interestingly has other random stuff people are pushing to try on it, so I wash my hands of any failures other than the ones in the test I'm changing (or, I guess, other layout/xul tests that are actually relevant))

Clearing surplus needinfo now that there's a review request.
Flags: needinfo?(ehsan)
Comment on attachment 8612985 [details]
MozReview Request: Bug 1169606 - fix OSX margin for label/description and fix test rounding so it doesn't go orange, r?ehsan

makes sense, thanks!
Attachment #8612985 - Flags: review?(ehsan) → review+
(for checkin, see trypush disclaimer in comment #8)
Keywords: checkin-needed
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/e557e50b3ba2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Thanks, Gijs!  Sorry it took me so long to see this.  Your fix makes sense.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: