Closed Bug 1625764 Opened 5 years ago Closed 5 years ago

New wpt failures from PR 22480 in test flexbox-overflow-auto-002.html, due to apparent dependence on Chrome's exact scrollbar-sizes

Categories

(Core :: Layout: Flexbox, defect, P5)

Product:
Core
Component:
Layout: Flexbox
Type:
defect

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: wpt-sync, Assigned: tonikitoo)

References

Details

(Whiteboard: [wpt])

The following tests have untriaged failures in the CI runs for wpt PR 22480:

Firefox-only failures

/css/css-flexbox/flexbox-overflow-auto-002.html
.flexbox, .inline-flexbox 11: FAIL
.flexbox, .inline-flexbox 2: FAIL
.flexbox, .inline-flexbox 1: FAIL
.flexbox, .inline-flexbox 6: FAIL
.flexbox, .inline-flexbox 5: FAIL
.flexbox, .inline-flexbox 12: FAIL

New Tests That Don't Pass

/css/css-flexbox/flexbox-overflow-auto-002.html
.flexbox, .inline-flexbox 10: FAIL (Chrome: PASS, Safari: FAIL)
.flexbox, .inline-flexbox 3: FAIL (Chrome: PASS, Safari: FAIL)
.flexbox, .inline-flexbox 14: FAIL (Chrome: PASS, Safari: FAIL)
.flexbox, .inline-flexbox 4: FAIL (Chrome: PASS, Safari: FAIL)
.flexbox, .inline-flexbox 13: FAIL (Chrome: PASS, Safari: FAIL)
.flexbox, .inline-flexbox 9: FAIL (Chrome: PASS, Safari: FAIL)

These updates will be on mozilla-central once bug 1625282 lands.

Note: this bug is for tracking fixing the issues and is not
owned by the wpt sync bot.

This bug is linked to the relevant tests by an annotation in
https://github.com/web-platform-tests/wpt-metadata. These annotations
can be edited using the wpt interop dashboard
https://jgraham.github.io/wptdash/

If this bug is split into multiple bugs, please also update the
annotations, otherwise we are unable to track which wpt issues are
already triaged. Resolving as duplicate or closing this issue should
be cause the bot to automatically update or remove the annotation.

I think this test just has hardcoded over-calibrated expectations; it was written as a reftest for Chrome, and it inadvertently was written to expect the precise sizing of Chrome's scrollbars, as far as I can tell.

Here's the first piece of the test (note that it's expecting the outer div to be 47px tall):

<div class="flexbox" data-expected-height="47">
  <div class="overflow"><div style="width: 100px; height: 20px"></div></div>
</div>

Here, the things contributing height are:
(a) the innermost div contributes an explicit height: 20px
(b) the .overflow div contributes 2px of border (1px top + bottom), plus the height of its horizontal scrollbar from overflow:auto
(c) the .flexbox div contributes 10px of border (5px top + bottom)

So, the total height is 20px + 2px + 10px + scrollbarHeight, which simplifies to 32px + scrollbarHeight

In Chrome, the scrollbar is apparently 15px tall, so that gives a total height of 32px + 15px = 47px, and the test seems to have been written to expect precisely that. :-/

In Firefox on my Linux machine, the scrollbar seems to be 12px tall, so that gives a total height of 32px + 12px = 44px (which "fails" because the test has a hardcoded data-expected-height="47")

The needs to be rewritten to avoid expecting Chrome's exact scrollbar-sizes. biesi, it looks like you're the reviewer on this test -- can you let me know if I'm missing something? (Or: dgrogan/tonkitoo, it looks like you were involved with the migration of this test to WPT; maybe you can chime in?)

Flags: needinfo?(cbiesinger)
Priority: -- → P5
Summary: New wpt failures from PR 22480 → New wpt failures from PR 22480 in test flexbox-overflow-auto-002.html, due to apparent dependence on Chrome's exact scrollbar-sizes

Your analysis seems spot on, chrome's scrollbars are 15px and this test hardcoded that.

tonikitoo, do you want to fix this test? There's one in the chromium repo at third_party/blink/web_tests/css3/flexbox/overflow-auto-resizes-correctly.html that measures scrollbars you can crib from.

If not, we should yank it from wpt.

I also agree with the analysis. We should probably revert that commit. I am confused why the test passes on mac even in chrome, since we use overlay scrollbars there (maybe it doesn't pass...)

Flags: needinfo?(cbiesinger)

Great analyzes. I will give it a try with fixing it.

Assignee: wptsync → tonikitoo

FYI, a fix is being reviewed at https://chromium-review.googlesource.com/c/chromium/src/+/2130046

It does sound like there are unrelated (actual?) failures in Firefox with that fix.

Note that the test has been renamed to overflow-auto-006.html

thanks cbiesinger for the updates here.

(re comment #10) I also agree that there are actual failures that the test exposes (despite the chrome-specific scrollbar size issue being fixed):
by loading the test on both Firefox and Chrome, it is easily possible to observe that the failures manifest as rendering differences on both engines.
I'd encourage further investigation to verify whether the problem is on FF on Chrome.

cbiesinger, dholbert, dgrogan, WDYT?

... and it's been merged. It is a progression, after all:

-> /css/css-flexbox/overflow-auto-006.html 5 / 17 11 / 17

But as I said the remaining failures seem to be out of the scope of this bug (and match Safari's behavior).

should we file a new bug, or re-scope this one?

(In reply to Christian :Biesinger (don't email me, ping me on IRC) from comment #9)

FYI, a fix is being reviewed at https://chromium-review.googlesource.com/c/chromium/src/+/2130046

It does sound like there are unrelated (actual?) failures in Firefox with that fix.

I'm looking at the failures in the updated version of the test from that review page, and I think they may still be signs of over-tuning.

In particular:
(1) the test mostly fails in Chrome (4 PASS / 12 FAIL) when viewed at 200% scaling (from hitting "Ctrl +" 5 times), with what seem to be off-by-1 errors ("expected 39 but got 40"). I would expect that an even scaling factor like 200% should not cause the test to fail with rounding errors like this. (This probably isn't responsible for the Firefox test failures, since as far as I can tell we're off by more-than-one, but it's something worth looking into in the test's calculations.)

(2) The tested chunks are too small to fit any scrollbars at all, in some configurations. tl;dr: they assume that 20px is enough thickness to show content + scrollbar, but that doesn't necessarily hold up. In a 100%-pixel-scaling window on my (HiDPI) laptop, the scrollbars are approximately 20 css pixels in thickness, so we don't add them at all to avoid stealing all the space from the content, and this means the calculations don't match the test's expectations. (This is responsible for tests #1, 2, 5, and 6 failing when I run ./mach wpt [path-to-test] on my laptop.)

(3) In a 200%-pixel-scaled window (the scale factor that I have configured in Ubuntu & which Firefox normally runs at), the above issue (2) is not a problem but I still fail tests 3, 4, 9, 10, 13, 14; I need to look into those. I'm not yet sure if they're real failures or other test bugs.

Also: notably, Edge 83 fails precisely the same tests that Firefox fails on the new version of the test, according to https://wpt.fyi/results/css/css-flexbox/overflow-auto-006.html (and Safari fails half of that same set of tests, too), so that suggests something's wrong with the test...

I dug into the "category (3)" tests from my previous comment (tests 3, 4, 9, 10, 13, 14), and they're all instances of bug 764076 -- spots where we have overflow:auto with an optional scrollbar, which we don't include in the intrinsic size of the element. Per bug 764076 comment 7, this particular bit of behavior is defined as being "user agent-dependent" in CSS, so ideally WPT tests shouldn't be depending on a particular strategy there. But I'm happy to punt on those particular failures and chalk them up to bug 764076 which we should probably do something about eventually.

Anyway, we should probably close this bug out; I'll file a github issue to cover this.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.