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)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Iteration:
57.1 - Aug 15
Tracking Status
firefox57 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

(Whiteboard: [photon-visual][p1])

Attachments

(1 file, 1 obsolete file)

Whiteboard: [photon-visual][triage]
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-visual][triage] → [photon-visual]
Blocks: 1363023
Whiteboard: [photon-visual] → [photon-visual][p1]
Sometime soon we'll need to update the theme to use the expected tab height. Can we disable this test?
Flags: needinfo?(manishearth)
works for me. keep a bug open under the stylo component
Flags: needinfo?(manishearth)
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)
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)
Attached patch set expexted tab height (obsolete) — Splinter Review
Here's the patch for reproducing this. And yes, I can reproduce the unexpected pass locally.
Flags: needinfo?(dao+bmo)
(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.
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.
(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.
(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)
(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.
(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! :)
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)
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.
Attachment #8895429 - Attachment is obsolete: true
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6bda6f25258b1a635bc3837a4a90fd775f889d4
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Flags: qe-verify? → qe-verify-
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+
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
Iteration: --- → 57.1 - Aug 15
Priority: P2 → P1
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).
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
https://hg.mozilla.org/mozilla-central/rev/c94473d1f76d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
No longer depends on: 1390237
Depends on: 1397401
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: