There are several style system mochitests which were written with testharness.js framework rather than SimpleTest framework. In bug 1337674, I'm going to integrate the failure pattern check into SimpleTest framework. testharness.js is a bit harder to integrate. In addition, "fail-if" doesn't work for testharness.js tests either. Given that the number is relatively small, the easiest way forward is to simply disable them for now. We can enable them when they no longer fails with stylo. The tests include: layout/style/test/test_align_shorthand_serialization.html layout/style/test/test_font_feature_values_parsing.html layout/style/test/test_grid_container_shorthands.html layout/style/test/test_grid_item_shorthands.html layout/style/test/test_grid_shorthand_serialization.html layout/style/test/test_grid_computed_values.html layout/style/test/test_group_insertRule.html

test_group_insertRule.html will be enabled in bug 1315601.

status-firefox57=wontfix unless someone thinks this bug should block 57

There's at least one test referencing this bug, and of course it caught a bug :(

Comment on attachment 8957377 [details] Bug 1339656: [css-align] Don't restrict shorthand parsing now that's not ambiguous. https://reviewboard.mozilla.org/r/226300/#review232220

Comment on attachment 8957378 [details] Bug 1339656: Make test_align_shorthand_serialization.html account for bug 1430817. https://reviewboard.mozilla.org/r/226302/#review232222 ::: layout/style/test/test_align_shorthand_serialization.html:82 (Diff revision 1) > - { > alignSelf: "self-end safe", > shorthand: "", > }, > { > - justifySelf: "unsafe start", > + justifySelf: "start unsafe", Actually I think this is wrong, I'll look at this closer in the morning. This should be `unsafe start`, but I think we should serialize with that, since that's a perfectly legit value... Will think more about this one in the morning.

Comment on attachment 8957379 [details] Bug 1339656: Re-enable test_align_shorthand_serialization.html. https://reviewboard.mozilla.org/r/226304/#review232224 rs=me as far as it passes, and mats is fine with the test change.

Comment on attachment 8957378 [details] Bug 1339656: Make test_align_shorthand_serialization.html account for bug 1430817. https://reviewboard.mozilla.org/r/226302/#review238206 "left legacy" and "legacy left" are both valid values for justify-items, so I don't see a need to change it. "start unsafe" is an invalid value for justify-self, so r- on that change. ("unsafe start" is the correct syntax)

(In reply to Mats Palmgren (:mats) from comment #10) > Comment on attachment 8957378 [details] > Bug 1339656: Make test_align_shorthand_serialization.html account for bug > 1430817. > > https://reviewboard.mozilla.org/r/226302/#review238206 > > "left legacy" and "legacy left" are both valid values for justify-items, > so I don't see a need to change it. > > "start unsafe" is an invalid value for justify-self, so r- on that change. > ("unsafe start" is the correct syntax) Whoops, I totally forgot about this bug, yikes. So, there's an interesting thing. So, if we have: justify-self: unsafe start; align-self: auto; IMO we should serialize place-self as "auto start", since it is representable as the shorthand. I'll fix up stuff.

The safe/unsafe bits were intentionally left there in anticipation of supporting the "(no value specified)" behavior: https://drafts.csswg.org/css-align-3/#overflow-values (we would need that bit to be able to separate it from "no value", and we should serialize 'unsafe' when that bit is set) I made a note of that here, fwiw: https://github.com/w3c/csswg-drafts/issues/2229#issuecomment-378450069

Comment on attachment 8957377 [details] Bug 1339656: [css-align] Don't restrict shorthand parsing now that's not ambiguous. https://reviewboard.mozilla.org/r/226300/#review239058 Gecko bits looks fine to me, although I think we'd probably revert this if/when we implement the "(no value specified)" behavior. I didn't look at the Stylo part, you'd need a different reviewer for that. ::: layout/style/nsStyleConsts.h (Diff revision 2) > #define NS_STYLE_ALIGN_SPACE_BETWEEN 14 > #define NS_STYLE_ALIGN_SPACE_AROUND 15 > #define NS_STYLE_ALIGN_SPACE_EVENLY 16 > -#define NS_STYLE_ALIGN_LEGACY 0x20 // mutually exclusive w. SAFE & UNSAFE > +#define NS_STYLE_ALIGN_LEGACY 0x20 // mutually exclusive w. SAFE > #define NS_STYLE_ALIGN_SAFE 0x40 > -#define NS_STYLE_ALIGN_UNSAFE 0x80 // mutually exclusive w. SAFE I suggest we just comment this bit out for now and say something like "reserved for future use".

Comment on attachment 8957377 [details] Bug 1339656: [css-align] Don't restrict shorthand parsing now that's not ambiguous. https://reviewboard.mozilla.org/r/226300/#review239484 ::: layout/style/test/test_align_shorthand_serialization.html:62 (Diff revision 2) > - justifyItems: "start safe", > + justifyItems: "safe start", > shorthand: "", Actually, as Tab notes in [1], now that the grammar has been simplified (the explicit fallback value was removed and safe/unsafe must be the first keyword), this can in fact be represented as "place-items: normal safe start". So the grammar for 'place-items' is now back to its original "<align-items> <justify-items>?" form: https://drafts.csswg.org/css-align-3/#place-items-property (meaning that the full set of values can be represented in the shorthand) [1] https://github.com/w3c/csswg-drafts/issues/2276#issuecomment-378757319

I... don't understand the stuff here, and it doesn't seem to match the current status of the spec? I had a look at the comments mentioned in this bug, but cannot really extract much useful information from them. Is there any other discussions? This states that "unsafe" is the default, which appears to go against the spec, which says > If the overflow alignment isn’t explicitly specified, the default overflow alignment is a blend of "safe" and "unsafe" ... so why would this treat "unsafe" as the default and ignore it everywhere?

So what happens here is that the spec used to word the "unspecified value" behavior as computing to "unsafe", thus https://github.com/w3c/csswg-drafts/issues/2229. That being said, as mats said in comment 15, the spec has changed (again), and now all the shorthands should be representable, so we may not need to do all the has_extra_flags checks. I'll update this all to the spec after this bug I guess.

Comment on attachment 8957377 [details] Bug 1339656: [css-align] Don't restrict shorthand parsing now that's not ambiguous. I guess I'll update to the spec now here to allow all the values in the shorthand now that it's not ambiguous. It's easier.

Comment on attachment 8957377 [details] Bug 1339656: [css-align] Don't restrict shorthand parsing now that's not ambiguous. (Sigh, mozreview)

Comment on attachment 8957377 [details] Bug 1339656: [css-align] Don't restrict shorthand parsing now that's not ambiguous. https://reviewboard.mozilla.org/r/226300/#review239712 I think we should fix this too: https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/servo/components/style/properties/shorthand/position.mako.rs#641 The spec now says "unless that value is a <baseline-position> in which case it is defaulted to 'start'." so "place-content:baseline" should be valid for example. This "align.is_valid_on_both_axes()" test is redundant for place-self: https://searchfox.org/mozilla-central/rev/f860c2bf00576959b07e2d9619c7b5088b0005be/servo/components/style/properties/shorthand/position.mako.rs#686

Forgot the spec link: https://drafts.csswg.org/css-align-3/#place-content

Comment on attachment 8965265 [details] Bug 1339656: [css-align] Upstream our shorthand serialization tests. https://reviewboard.mozilla.org/r/233984/#review239714 As noted in comment 13, serializing the 'unsafe' keyword or not are both valid options depending on whether the UA implements the "no value" alignment or not. So while all these tests are valid *for Gecko*, they are not universally valid. I guess we could upstream it with all values involving 'unsafe' stripped out and keep those tests in layout/style/test/test_align_shorthand_serialization.html?

Comment on attachment 8957377 [details] Bug 1339656: [css-align] Don't restrict shorthand parsing now that's not ambiguous. https://reviewboard.mozilla.org/r/226300/#review239750 LGTM, thanks!

Comment on attachment 8965382 [details] Bug 1339656: Make the shorthand serialization test account for unsafe serialization. https://reviewboard.mozilla.org/r/234122/#review239758 ::: commit-message-67839:1 (Diff revision 1) > +Bug 1339656: Make the shorthand serialization test account for unsafe serialization. r?mats > + "for unsafe serialization" sounds scary :-) Perhaps make it 'unsafe' ?

Comment on attachment 8965265 [details] Bug 1339656: [css-align] Upstream our shorthand serialization tests. https://reviewboard.mozilla.org/r/233984/#review239756 I'm assuming that the new patch goes before this one.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/91ae8e15b175 [css-align] Don't restrict shorthand parsing now that's not ambiguous. r=mats https://hg.mozilla.org/integration/autoland/rev/bbd2f0c18f64 Make the shorthand serialization test account for 'unsafe' serialization. r=mats https://hg.mozilla.org/integration/autoland/rev/4ac493295325 [css-align] Upstream our shorthand serialization tests. r=mats

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/e90ac14a383f followup: Remove a explicit testharness.css link to appease wptlint. r=me on a CLOSED TREE

https://hg.mozilla.org/mozilla-central/rev/91ae8e15b175 https://hg.mozilla.org/mozilla-central/rev/bbd2f0c18f64 https://hg.mozilla.org/mozilla-central/rev/4ac493295325 https://hg.mozilla.org/mozilla-central/rev/e90ac14a383f

James, looks like this test wasn't upstreamed, is it a bug in wpt-sync, or it just would take a bit more time? (not sure of the process)

I asked over IRC, apparently Sync is stuck, and probably today will get fixed.

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