New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[css-flexbox] Move flex-flow-auto-margins-no-available-space-assert.html test to WPT #22459
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The review process for this patch is being conducted in the Chromium project.
9c5b33c
to
34bb4dd
Compare
…tml test to WPT This CL moves flex-flow-auto-margins-no-available-space-assert.html test from css3/css-flexbox to external/wpt/css/css-flexbox with WPT styles, adding links to the relevant specs, and test description. Bug: 1063749 Change-Id: I6775022c3fc8572f78e7ec19ad88b91dfdcb7a23 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2120665 Commit-Queue: Gyuyoung Kim <gyuyoung@igalia.com> Reviewed-by: Robert Ma <robertma@chromium.org> Reviewed-by: David Grogan <dgrogan@chromium.org> Cr-Commit-Position: refs/heads/master@{#753900}
34bb4dd
to
8906a9b
Compare
Hi folks... This test fails in all browsers, as shown here: It has extremely-cursed-looking styling & expectations:
This element is expected to be exactly thirty million, two hundred and ten thousand, two hundred and seventy two pixels tall. (no obvious reason why the test would be expecting precisely that height) And it's not actually that height, even in Chrome. Can we back this test out, and/or fix it to have valid expectations? @Gyuyoung @davidsgrogan |
(CC @cbiesinger as well since it looks like he was involved with the gerrit code review) I'm a bit surprised this testcase made the jump into WPT in its current state, honestly, given: In the future it would be great if reviewers & patch-authors could keep these sorts of things in mind, as part of the checklist of things to look at & check for when auditing tests that are going to be added to WPT. (Right now I'm digging out from a pile of new WPT tests that were recently added to WPT & which fail in Firefox, and cases like this one are kinda frustrating to have to spend time investigating.) |
Yeah, this has to go. Sorry @Gyuyoung I didn't realize it was this test. It only passes in chrome's local test runner. It (+ one other test) caught a regression for me last year so I'm a little hesitant to delete it, but I sure didn't find the bug by figuring out what the test was doing. @Gyuyoung maybe for now, move it, for whatever the latest name it has, back to css3/flexbox and mark it unmoveable? @cbiesinger do you know what this test was supposed to be testing? |
Totally agree, apologies. |
My apologies, @dholbert . I had assumed that this wouldn't be a problem because I knew that on Mac, we have overlay scrollbars, and so I thought tests would fail there if they don't handle that. But today I learned that our test runner disables them on Mac, and clearly we have tests relying on that. I will make sure to watch out for that in future patches. @davidsgrogan , the test was added to catch a case where we would compute a negative content size for a box (a size less than border+padding). https://codereview.chromium.org/1017673002 |
To be clear, the presence/absence of overlay scrollbars wasn't really the problem in this particular case. My gripe here is the unexplained & almost-certainly-bogus But in any case, I appreciate the sentiment & appreciate you all taking a look. :) |
The test is indecipherable and only passes in content_shell, not chrome or other browsers. #22459 Change-Id: I541772aa45428f9002a1d0909a3ad3b72dc2c9a7
Sorry for the late reply. yes, I will mark the test unmoveable. Then, I'm going to revert it. |
I moved it to the css3/flexbox again. Then, I marked the test unmoveable in the sheet. My apologies, @dholbert. |
This CL moves flex-flow-auto-margins-no-available-space-assert.html
test from css3/css-flexbox to external/wpt/css/css-flexbox with WPT
styles, adding links to the relevant specs, and test description.
Bug: 1063749
Change-Id: I6775022c3fc8572f78e7ec19ad88b91dfdcb7a23
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2120665
Commit-Queue: Gyuyoung Kim <gyuyoung@igalia.com>
Reviewed-by: Robert Ma <robertma@chromium.org>
Reviewed-by: David Grogan <dgrogan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#753900}