Skip to content
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

Merged
merged 1 commit into from Mar 27, 2020

Conversation

chromium-wpt-export-bot
Copy link
Collaborator

@chromium-wpt-export-bot chromium-wpt-export-bot commented Mar 26, 2020

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}

Copy link
Collaborator

@wpt-pr-bot wpt-pr-bot left a 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.

@chromium-wpt-export-bot chromium-wpt-export-bot changed the title [css-flexbox] Move flex-flow-auto-margins-no-available-space-assert.html test from css3/flexbox to WPT [css-flexbox] Move flex-flow-auto-margins-no-available-space-assert.html test to WPT Mar 27, 2020
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-2120665 branch 3 times, most recently from 9c5b33c to 34bb4dd Compare March 27, 2020 03:22
…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}
@dholbert
Copy link
Contributor

Hi folks... This test fails in all browsers, as shown here:
https://wpt.fyi/results/css/css-flexbox/flex-flow-auto-margins-no-available-space-assert.html

It has extremely-cursed-looking styling & expectations:

* {
    display: flex;
    padding-bottom: 20pt;
    min-height: 0.7%;
    margin-top: 6000%;
    flex-shrink: 0;
    flex-basis: 7000%;
}

<abbr data-expected-height=30210272>
    <input></input>
</abbr>

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

@dholbert
Copy link
Contributor

dholbert commented Mar 31, 2020

(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:
a) the weird/cursed magic-number in the expected value
b) the lack of any comments explaining (or self-evident reason for) where that magic number comes from
c) the fact that not even Chromium passes the test right now

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.)

@davidsgrogan
Copy link
Member

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?

@davidsgrogan
Copy link
Member

(CC @cbiesinger as well since it looks like he was involved with the gerrit code review)

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.)

Totally agree, apologies.

@cbiesinger
Copy link
Contributor

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

@dholbert
Copy link
Contributor

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 data-expected-height=30210272 (which presumably is the humongous height that Chrome computes under some configurations, but likely isn't based on much besides that).

But in any case, I appreciate the sentiment & appreciate you all taking a look. :)

chromium-wpt-export-bot pushed a commit that referenced this pull request Apr 4, 2020
The test is indecipherable and only passes in content_shell, not chrome
or other browsers. #22459

Change-Id: I541772aa45428f9002a1d0909a3ad3b72dc2c9a7
@Gyuyoung
Copy link
Contributor

Gyuyoung commented Apr 6, 2020

@Gyuyoung maybe for now, move it, for whatever the latest name it has, back to css3/flexbox and mark it unmoveable?

Sorry for the late reply. yes, I will mark the test unmoveable. Then, I'm going to revert it.

@Gyuyoung
Copy link
Contributor

Gyuyoung commented Apr 6, 2020

I moved it to the css3/flexbox again. Then, I marked the test unmoveable in the sheet. My apologies, @dholbert.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants