Closed
Bug 1169606
Opened 9 years ago
Closed 9 years ago
<description> and <label> should have -moz-margin-end: 5px;
Categories
(Toolkit :: Themes, defect)
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...
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
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.
Assignee | ||
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 6•9 years ago
|
||
(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.
Assignee | ||
Comment 7•9 years ago
|
||
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
Assignee | ||
Comment 8•9 years ago
|
||
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)
Reporter | ||
Comment 9•9 years ago
|
||
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+
Assignee | ||
Comment 10•9 years ago
|
||
(for checkin, see trypush disclaimer in comment #8)
Keywords: checkin-needed
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Comment 11•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/e557e50b3ba2
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e557e50b3ba2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•