1450390 - Add a few tests for 'flex-basis' that were inspired by review on Bug 1105111

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Daniel Holbert [:dholbert]]

Filing this as a followup to bug 1105111, to add a few tests for related conditions that came up during review.

Specifically:
 - test transitions on 'flex-basis' to/from the 'content' keyword, per bug 1105111 comment 17. (There'll be no interpolation; just to make sure things don't explode.)

 - test that flex-basis has no effect on abspos flex children, per bug 1105111 comment 16.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: Bug 1450390 part 1: Extend mochitest to cover transitions to/from flex-basis "content" keyword.

Review commit: https://reviewboard.mozilla.org/r/233416/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233416/

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Bug 1450390 part 2: Add reftest to verify that "flex-basis" doesn't affect layout of abspos flex children.

Review commit: https://reviewboard.mozilla.org/r/233418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233418/

Comment 3

[Mats Palmgren (inactive)]

Comment on attachment 8964707 [details]
Bug 1450390 part 2: Add reftest to verify that "flex-basis" doesn't affect layout of abspos flex children.

https://reviewboard.mozilla.org/r/233418/#review238988

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8964706 [details]
Bug 1450390 part 1: Extend mochitest to cover transitions to/from flex-basis "content" keyword.

https://reviewboard.mozilla.org/r/233416/#review238990

Thanks for writing this test!

Just nits

::: layout/style/test/test_transitions_per_property.html:1188
(Diff revision 1)
> +  div.style.setProperty(prop, "content", "");
> +  is(cs.getPropertyValue(prop), "content",
> +     "property " + prop + ": transition to 'content' (should jump directly)");
> +
> +  // Test transition from 'content' to length:
> +  div.style.setProperty("transition-property", "none", "");

no need for the third argument to setProperty afaict (here and in the other callers).

::: layout/style/test/test_transitions_per_property.html:1195
(Diff revision 1)
> +  is(cs.getPropertyValue(prop), "content",
> +     "property " + prop + ": computed value before transition from 'content'");
> +  div.style.setProperty("transition-property", prop, "");
> +  div.style.setProperty(prop, "6px", "");
> +  is(cs.getPropertyValue(prop), "6px",
> +     "property " + prop + ": transition from 'content' (should jump directly)");

s/should jump directly/should not interpolate, or maybe "should be discrete", to match better spec terms?

No real preference of which one, it doesn't really matter much.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8964706 [details]
Bug 1450390 part 1: Extend mochitest to cover transitions to/from flex-basis "content" keyword.

https://reviewboard.mozilla.org/r/233416/#review238990

> no need for the third argument to setProperty afaict (here and in the other callers).

I noticed that, too -- I did that because every other setProperty(...) call in this file does that, so I figured I'd be consistent.

So I'll keep this in for now, to avoid breaking the mold (though maybe we should drop this arg across the board? *shrug*)

> s/should jump directly/should not interpolate, or maybe "should be discrete", to match better spec terms?
> 
> No real preference of which one, it doesn't really matter much.

"should be discrete" seems good & concise -- I'll use that. Thanks!

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8964706 [details]
Bug 1450390 part 1: Extend mochitest to cover transitions to/from flex-basis "content" keyword.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233416/diff/1-2/

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8964707 [details]
Bug 1450390 part 2: Add reftest to verify that "flex-basis" doesn't affect layout of abspos flex children.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/233418/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c259dc509f8b
part 1: Extend mochitest to cover transitions to/from flex-basis "content" keyword. r=emilio
https://hg.mozilla.org/integration/autoland/rev/363cd8bba774
part 2: Add reftest to verify that "flex-basis" doesn't affect layout of abspos flex children. r=mats

Comment 9

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/c259dc509f8b
https://hg.mozilla.org/mozilla-central/rev/363cd8bba774

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I just noticed that the test here has a rel="match" that disagrees with the reftest manifest, which means it's wrong in the web-platform-tests runner.  (Also note it's the only failure reported by layout/reftests/w3c-css/submitted/check-for-references.sh .)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c87ea680f79b
followup: Fix incorrect rel="match" tag in reftest flexbox-abspos-child-002.html. (test-only)

Comment 12

[Daniel Holbert [:dholbert]]

Thanks for the heads-up!

I just pushed a followup to address that -- fixed the rel="match" tag in both of our in-tree copies of the test (in reftests & in wpt), and deleting the .ini file that was annotating the wpt failure.

Comment 13

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11452 for changes under testing/web-platform/tests

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 15

[Daniel Holbert [:dholbert]]

Note: my commit (comment 11) triggered a "Linting W" orange, apparently because I need to regenerate the MANIFEST.json file to reflect the change in reference case for this test. (I think this is our WPT equivalent of reftest.list).  The orange looks like this:
> TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-abspos-child-002.html changed test types or metadata (wpt-manifest)
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=182773706

I'm compiling right now, and once that completes, I'll run ./mach web-platform-tests --manifest-update and push the resulting changes, which I think should address this orange.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f1a58e8b2a7
followup 2: regenerate wpt manifests after fixing rel="match" tag.

Comment 17

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/c87ea680f79b
https://hg.mozilla.org/mozilla-central/rev/9f1a58e8b2a7

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged