Closed
Bug 1386964
Opened 7 years ago
Closed 7 years ago
/css/css-values-3/viewport-units-css2-001.html is flaky, passes unexpectedly depending on the browser toolbar height
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: dao, Assigned: dao)
References
Details
(Whiteboard: [photon-visual][p1])
Attachments
(1 file, 1 obsolete file)
Example try run with failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4024fe916ba18a4f0dc98a1ec91e1e5e3bdab19f With the tab strip height reduced: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22652f4a12654440bde50db6a4aa7afd0b14bca3
Assignee | ||
Updated•7 years ago
|
Whiteboard: [photon-visual][triage]
Updated•7 years ago
|
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-visual][triage] → [photon-visual]
Assignee | ||
Updated•7 years ago
|
Whiteboard: [photon-visual] → [photon-visual][p1]
Assignee | ||
Comment 1•7 years ago
|
||
Sometime soon we'll need to update the theme to use the expected tab height. Can we disable this test?
Flags: needinfo?(manishearth)
Comment 2•7 years ago
|
||
works for me. keep a bug open under the stylo component
Flags: needinfo?(manishearth)
Comment 3•7 years ago
|
||
This is not stylo-only, so I don't think we (aka the people working on stylo) should decide to disable it unconditionally. And I'm personally not a fan of disabling tests... Could we at least mark the relevant subtests failing? Also, I suspect this is not a CSS bug per se, but either the test being bogus, or making assumptions about our result on clientWidth. Anyway, David, any opinion? I haven't investigated the test in depth yet.
Flags: needinfo?(dholbert)
Comment 4•7 years ago
|
||
BTW, Dao, a few questions: * Any slightly simpler STR? The patches in the try run are quite big. * Is the failure Linux-only? * The failure you showed is an unexpected pass... But I can't make it fail locally, can you? (To test, you can open testing/web-platform/tests/css/css-values-3/viewport-units-css2-001.html on your browser, with or without your patches).
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 5•7 years ago
|
||
Here's the patch for reproducing this. And yes, I can reproduce the unexpected pass locally.
Flags: needinfo?(dao+bmo)
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #4) > * Is the failure Linux-only? It appears to be Linux-only, but that might just be due to toolbar height differences compared to Windows and Mac.
Comment 7•7 years ago
|
||
Isn't the failure stylo only? At least, that's what I was originally CCd on this bug for. I'm fine with disabling the test for stylo only. We should fix the followup by the time stylo hits release.
Comment 8•7 years ago
|
||
(In reply to Manish Goregaokar [:manishearth] from comment #7) > Isn't the failure stylo only? At least, that's what I was originally CCd on > this bug for. It is not, the failures in the try run are on all |linux-| platforms.
Comment 9•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3) > Anyway, David Daniel ;) > any opinion? I haven't investigated the test in depth yet. I'm unfamiliar with this test as well, but I assume we should prefer to update the test expectations, if it reliably starts passing. Those expectations are here: http://searchfox.org/mozilla-central/source/testing/web-platform/meta/css/css-values-3/viewport-units-css2-001.html.ini Also: if I run the test locally, I see this output alongside the (currently-expected) failure, on Linux: > assert_equals: expected 97 but got 96 This looks like our rounding behavior doesn't match the test's expectations. This makes sense, because IIRC we do rounding differently for *borders in particular* (and the properties in question here are border-top-width and border-bottom-width), though I forget the details. And in this case, the test is just using "Math.round()" to do its own rounding when coming up with the expected value, and it's mistakenly assuming that we round exactly like Math.round() does for our border sizes. It also makes complete sense that the browser toolbar height would influence the sizing here, because that will reduce the size of the viewport and hence the size of viewport units. So it is probably shifting us away from an ambiguously-rounded-size (where our rounding behavior is different from Math.round) towards a predictably-rounded-size, and we go from failing to passing. So: Bottom line, I'd say we should just remove the "fails" annotations when this patch lands: http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/testing/web-platform/meta/css/css-values-3/viewport-units-css2-001.html.ini#9-13 (Or, if we still fail on some platforms, it looks like we can construct a platform-specific annotation like we have for other parts of that test. I suspect jgraham could help with getting that right.)
Flags: needinfo?(dholbert)
Assignee | ||
Comment 10•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #9) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #3) > > Anyway, David > > Daniel ;) > > > any opinion? I haven't investigated the test in depth yet. > > I'm unfamiliar with this test as well, but I assume we should prefer to > update the test expectations, if it reliably starts passing. For some definition of "reliably", maybe. If for some reason we change the UI on Linux again it would of course start failing again.
Comment 11•7 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #9) > (In reply to Emilio Cobos Álvarez [:emilio] from comment #3) > > Anyway, David > > Daniel ;) Gah, was about to type :dbaron in the ni? box and then changed my mind, but forgot to update the comment, whoops! :)
Comment 12•7 years ago
|
||
RE the rounding difference for borders -- for example, we render the border 1px wide here in both cases below, on my screen (which is normal-DPI -- I imagine the behavior might be different on HiDPI screens) data:text/html,<div style="border: 0.1px solid green">hi data:text/html,<div style="border: 0.4px solid green">hi data:text/html,<div style="border: 1.9px solid green">hi I don't recall the exact logic involved, but it's a pixel-snapping thing of some sort (snapping the border width to an exact whole number of pixels in some pixel-space, without wanting to impact the element's size too much). (In reply to Dão Gottwald [::dao] from comment #10) > If for some reason we change the > UI on Linux again it would of course start failing again. It might, or it might not. Anyway, if this is a linux-specific unexpected pass, I suspect this is as simple as changing expected: FAIL ...to: expected: if os != "linux": FAIL ...in the .ini file. And that's simpler than disabling the test and doesn't reduce our test coverage, so I'd opt for that. If we end up incurring further churn/pain on future UI tweaks, then maybe we could disable the test (or call this part "expected: RANDOM" or whatever the equivalent is, if there is a WPT equivalent)
Comment 13•7 years ago
|
||
See e.g. https://dxr.mozilla.org/mozilla-central/rev/a921bfb8a2cf3db4d9edebe9b35799a3f9d035da/testing/web-platform/meta/assumptions/ahem.html.ini for another .ini file which has a functional "fails on non-linux" annotation. Copypasting from there (or comment 12) should be fine I think.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8895429 -
Attachment is obsolete: true
Assignee | ||
Comment 15•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6bda6f25258b1a635bc3837a4a90fd775f889d4
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: qe-verify? → qe-verify-
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8895509 [details] Bug 1386964 - Set default tab height, mark flaky viewport-units-css2-001.html subtests as passing on Linux as well as anchoring-with-bounds-clamping.html on Mac. https://reviewboard.mozilla.org/r/166706/#review171868 Looks good - thanks!
Attachment #8895509 -
Flags: review?(dholbert) → review+
Comment 17•7 years ago
|
||
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4c274336b01e Set default tab height and mark flaky viewport-units-css2-001.html subtests as passing on Linux. r=dholbert
Updated•7 years ago
|
Iteration: --- → 57.1 - Aug 15
Priority: P2 → P1
Comment 18•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/ebf79263fc0e - one more flaky to fix, that (shrug) made /scroll-anchoring/anchoring-with-bounds-clamping.html | Anchoring combined with scroll bounds clamping in the document unexpectedly pass on OS X, https://treeherder.mozilla.org/logviewer.html#?job_id=122170975&repo=autoland (and on OS X Stylo, https://treeherder.mozilla.org/logviewer.html#?job_id=122092271&repo=autoland, if that's a separate condition).
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=73f6542b431194695f4d5ee817bba7a60cf37e05
Comment 21•7 years ago
|
||
Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c94473d1f76d Set default tab height, mark flaky viewport-units-css2-001.html subtests as passing on Linux as well as anchoring-with-bounds-clamping.html on Mac. r=dholbert
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c94473d1f76d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 24•7 years ago
|
||
Screenshots: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=4d54ac07b8c97f0e6713dab2ba694023b5b2f3b5&newProject=mozilla-central&newRev=5322c03f4c8587fe526172d3f87160031faa6d75
You need to log in
before you can comment on or make changes to this bug.
Description
•