Closed Bug 1339656 Opened 8 years ago Closed 6 years ago

stylo: Enable style system tests written with testharness.js

Categories

(Testing :: Mochitest, defect, P5)

Product:
Testing
Component:
Mochitest
Type:
defect

Tracking

(firefox57 wontfix, firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox61 --- fixed

People

(Reporter: xidorn, Assigned: emilio)

References

Details

Attachments

(3 files, 2 obsolete files)

Bug 1339656: [css-align] Don't restrict shorthand parsing now that's not ambiguous.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1339656: [css-align] Upstream our shorthand serialization tests.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1339656: Make the shorthand serialization test account for unsafe serialization.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details 
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.
Depends on: 1315601
Priority: -- → P3
Depends on: 1353218
Depends on: 1341802, 1355721
Depends on: 1363971
Priority: P3 → --
Priority: -- → P3
Priority: P3 → P5
status-firefox57=wontfix unless someone thinks this bug should block 57
status-firefox57: --- → wontfix
There's at least one test referencing this bug, and of course it caught a bug :(
Assignee: nobody → emilio
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
Attachment #8957377 - Flags: review?(xidorn+moz) → review+
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.
Attachment #8957378 - Flags: review?(xidorn+moz) → review?(mats)
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.
Attachment #8957379 - Flags: review?(xidorn+moz) → review+
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)
Attachment #8957378 - Flags: review?(mats) → review-
(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.
Attachment #8957378 - Attachment is obsolete: true
Attachment #8957379 - Attachment is obsolete: true
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".
Attachment #8957377 - Flags: review?(mats) → review+
Attachment #8957377 - Flags: review?(xidorn+moz)
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.
Attachment #8957377 - Attachment is obsolete: true
Attachment #8957377 - Flags: review?(xidorn+moz)
Attachment #8957377 - Flags: review?(xidorn+moz)
Attachment #8957377 - Flags: review?(mats)
Attachment #8957377 - Flags: review+
Comment on attachment 8957377 [details]
Bug 1339656: [css-align] Don't restrict shorthand parsing now that's not ambiguous.

(Sigh, mozreview)
Attachment #8957377 - Flags: review?(xidorn+moz)
Attachment #8957377 - Flags: review?(mats)
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
Attachment #8957377 - Flags: review?(mats) → review-
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?
Attachment #8965265 - Flags: review?(mats) → review-
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!
Attachment #8957377 - Flags: review?(mats) → review+
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' ?
Attachment #8965382 - Flags: review?(mats) → review+
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.
Attachment #8965265 - Flags: review?(mats) → review+
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
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)
Flags: needinfo?(james)
I asked over IRC, apparently Sync is stuck, and probably today will get fixed.
Flags: needinfo?(james)
Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/10426 for changes under testing/web-platform/tests
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: