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

Where to move vendor-imports/mozilla/mozilla-central-reftests/align3? #26884

Closed
foolip opened this issue Dec 14, 2020 · 10 comments · Fixed by #27174
Closed

Where to move vendor-imports/mozilla/mozilla-central-reftests/align3? #26884

foolip opened this issue Dec 14, 2020 · 10 comments · Fixed by #27174

Comments

@foolip
Copy link
Member

foolip commented Dec 14, 2020

We can now move tests from vendor-imports in the the main test suites under css/ (#8615) and I've sent a few PRs. For align3, however, I have questions. These are the tests:
https://wpt.fyi/results/css/vendor-imports/mozilla/mozilla-central-reftests/align3?run_id=5655021456719872&run_id=5745838489862144&run_id=5657645899841536

They were added by @dholbert and the spec links listed are mostly to https://drafts.csswg.org/css-align-3/#align-abspos-static and https://drafts.csswg.org/css-align-3/#justify-abspos-static. However, those spec sections were removed by @fantasai in w3c/csswg-drafts@829a074, and the remaining spec text is a lot shorter.

What spec text should these now link to?

That should also answer the question of where to move these. If it's not css/css-align/, then it might also be css/css-flexbox/ + css/css-grid/, since all of the tests are named either flex-abspos* or grid-abspos-*.

cc @davidsgrogan @svillar @mrego

@dholbert
Copy link
Contributor

dholbert commented Dec 15, 2020

However, those spec sections were removed by @fantasai in w3c/csswg-drafts@829a074, and the remaining spec text is a lot shorter.

That spec-edit was for w3c/csswg-drafts#1432, which had another quite-relevant spec edit that landed as well:
w3c/csswg-drafts@a595144

This second spec-edit was in response to a resolution in w3c/csswg-drafts#1432 (comment) which removed the influence of align-content/justify-content on abspos descendants, I think.

That may mean that all of the tests with align-content or justify-content in their names probably are wrong and need amended reference cases... Right now the tests are asserting that these properties do influence the positioning of the abspos children, as the spec used to require; but now the spec says they do not have any influence, I think. (Checking my understanding in w3c/csswg-drafts#1432 (comment) )

RE your main question here: I don't know offhand what the correct spec text to link to would be, at this point, but I can try to find out (and see if any of the other tests are also obsolete).

@foolip
Copy link
Member Author

foolip commented Dec 15, 2020

@dholbert is there a change in Gecko needed to align with the spec changes that happened here? Would you expect that after that, the Gecko results would be largely the same as Chrome and Firefox? If so, then most of these tests may not have value in finding interop problems, and we might be able to leave just the few that do have different cross-browser results, and a few negative tests to ensure that align-content/justify-content have no effect on abspos descendants.

Regardless of the details shake out to be, your feedback and help is much appreciated!

@dholbert
Copy link
Contributor

dholbert commented Dec 16, 2020

I need to page this spec area back into my head (and review the edits that have happened since I last looked at it), but it looks to me like a change in Gecko is indeed needed, at least for the align-content/justify-content behavior. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1682641 on that. For those tests, I think you're right that we don't need a zillion negative tests -- just a few would probably do.

That's less than half of the tests in this directory, though. I'll aim to get back to you with thoughts about the other tests in this directory (the *-self tests) within the next few days.

Thanks for your work on migrating/merging these tests with the rest of the suite!

@foolip
Copy link
Member Author

foolip commented Dec 16, 2020

Thanks @dholbert, much appreciated! I'll soon be out for the holidays but I'll check back in eventually :)

@dholbert
Copy link
Contributor

dholbert commented Jan 4, 2021

Sorry, the holidays interrupted my "the next few days" plans here. :) Back from holidays now, and this is on my list of things-to-look-at-first.

@dholbert
Copy link
Contributor

dholbert commented Jan 5, 2021

I think I misunderstood w3c/csswg-drafts@829a074 and I forgot that another section of spec text still existed. As a result, I think these tests are still valid (particularly the justify-content and align-content ones that I was initially suspicious of).

For flexbox, these tests are essentially anchored in Flexbox section 4.1, https://drafts.csswg.org/css-flexbox-1/#abspos-items , which says:

The static position of an absolutely-positioned child of a flex container is determined such that the child is positioned as if it were the sole flex item in the flex container, assuming both the child and the flex container were fixed-size boxes of their used size

(I had mistakenly thought this text had been removed, but it's still there, and I think still authoritative.)

This spec-text implies that the static position should be influenced by the flex container's justify-content (which Chrome and Firefox agree on FWIW as shown in this testcase, modulo a few small pieces failing in Chrome due to https://bugs.chromium.org/p/chromium/issues/detail?id=1011718#c12 ).

It also implies that the abspos children's static position should be influenced by the flex container's align-content, if the flex container is multiline (i.e. has flex-wrap:wrap). Chrome does not agree with Firefox on that point -- Chrome seems to just disregard align-content entirely when determining static position of abspos children in a multi-line flex container. That seems probably like a Chrome bug which I should file if I haven't already done so. That discrepancy can be seen in this testcase where Chrome renders all of the children the same in each section, despite the varying align-content values on their flex container.

So: bottom-line, the flex- tests should probably all link to https://www.w3.org/TR/css-flexbox-1/#abspos-items , and similarly the grid tests should probably all link to https://www.w3.org/TR/css-grid-1/#abspos-items , and they should probably move to those respective subdirectories.

@dholbert
Copy link
Contributor

dholbert commented Jan 5, 2021

( @foolip I think the last paragraph of my comment above is the answer to your original question here. It sounded like you might be up for making those changes/migrations -- if so, I applaud & support you doing so. :) Thanks!)

@foolip
Copy link
Member Author

foolip commented Jan 13, 2021

Thank you for digging into that, @dholbert! I take that to mean we should move the tests into css/flexbox/ and css/grid/ and would be happy to send a PR for that.

@dholbert
Copy link
Contributor

Yes. Thank you!

foolip added a commit that referenced this issue Jan 13, 2021
Links are updated as recommended by dholbert:
#26884 (comment)

Part of #8615.

Fixes #26884.
@foolip
Copy link
Member Author

foolip commented Jan 13, 2021

I've sent #27174 to do the move.

foolip added a commit that referenced this issue Jan 14, 2021
Links are updated as recommended by dholbert:
#26884 (comment)

Part of #8615.

Fixes #26884.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 15, 2021
…vendor-imports, a=testonly

Automatic update from web-platform-tests
[css-flexbox][css-grid] move tests from vendor-imports (#27174)

Links are updated as recommended by dholbert:
web-platform-tests/wpt#26884 (comment)

Part of web-platform-tests/wpt#8615.

Fixes web-platform-tests/wpt#26884.
--

wpt-commits: 526de418365074a83b72c5390b50f83c0e06f728
wpt-pr: 27174
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jan 16, 2021
…vendor-imports, a=testonly

Automatic update from web-platform-tests
[css-flexbox][css-grid] move tests from vendor-imports (#27174)

Links are updated as recommended by dholbert:
web-platform-tests/wpt#26884 (comment)

Part of web-platform-tests/wpt#8615.

Fixes web-platform-tests/wpt#26884.
--

wpt-commits: 526de418365074a83b72c5390b50f83c0e06f728
wpt-pr: 27174

UltraBlame original commit: f5b71ab585edef7e6e792b347e05818521661875
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 19, 2021
…vendor-imports, a=testonly

Automatic update from web-platform-tests
[css-flexbox][css-grid] move tests from vendor-imports (#27174)

Links are updated as recommended by dholbert:
web-platform-tests/wpt#26884 (comment)

Part of web-platform-tests/wpt#8615.

Fixes web-platform-tests/wpt#26884.
--

wpt-commits: 526de418365074a83b72c5390b50f83c0e06f728
wpt-pr: 27174
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jan 25, 2021
…vendor-imports, a=testonly

Automatic update from web-platform-tests
[css-flexbox][css-grid] move tests from vendor-imports (#27174)

Links are updated as recommended by dholbert:
web-platform-tests/wpt#26884 (comment)

Part of web-platform-tests/wpt#8615.

Fixes web-platform-tests/wpt#26884.
--

wpt-commits: 526de418365074a83b72c5390b50f83c0e06f728
wpt-pr: 27174

UltraBlame original commit: 914697e3dd1f90bfa77edc5fb03aaf1cc0183473
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants