[css-logical] Implement the margin-block/inline shorthands

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P3
normal
RESOLVED FIXED
7 months ago
5 months ago

People

(Reporter: mats, Assigned: mats)

Tracking

({dev-doc-complete})

Trunk
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment)

This follows the same pattern as for padding-* in bug 1519847.
Interestingly, this fails an animation test:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2c7966b1fb7b6f6deec91e351adfaf89b109fd8&selectedJob=221789244
(so I assume padding-* would also fail if this test included a test for it)

https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/testing/web-platform/tests/css/css-logical/animation-001.html#147-158

Is this a valid test?
If so, do we already have a bug for it open somewhere?

Attachment #9036434 - Flags: review?(emilio)

:boris maybe you know the answer to my question about the animation test above?

Birtles, the animation test looks reasonable, but it doesn't match:

https://drafts.csswg.org/web-animations/#calculating-computed-keyframes

In particular, the rule (4), which this test tests, is useless, since (3) resolves any conflict between the two properties by sorting by name..

Flags: needinfo?(bbirtles)
Comment on attachment 9036434 [details] [diff] [review]
[css-logical] Implement the margin-block/inline shorthands

Review of attachment 9036434 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Per my comment above I think that test doesn't match the spec, but that it may not be intentional.

I can fix it easily if Birtles decides both the spec and Gecko requires fixing. It's a matter of discerning between logical and physical shorthands in:

  https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/servo/components/style/values/animated/mod.rs#39

Also, can you update the comment in:

  https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/servo/ports/geckolib/glue.rs#1053

To something like "We shouldn't need to care about shorthands", and add a MOZ_ASSERT(!IsShorthand(aProperty)) in:

  https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/layout/style/nsCSSProps.h#167

? It should hold given the callers.
Attachment #9036434 - Flags: review?(emilio) → review+

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Birtles, the animation test looks reasonable, but it doesn't match:

https://drafts.csswg.org/web-animations/#calculating-computed-keyframes

In particular, the rule (4), which this test tests, is useless, since (3) resolves any conflict between the two properties by sorting by name.

It seems the spec doesn't mention logical or physical in (1)(2)(3), so if (1)(2)(3) are for general cases, this test may have problems and we may need to update Gecko, as you mentioned. Brian is on PTO this week, so we can file a bug for this.

OK, I filed a follow-up bug 1520069 on that.

Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/382bf54cf7cf
[css-logical] Implement the margin-block/inline shorthands.  r=emilio
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Keywords: dev-doc-needed

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Birtles, the animation test looks reasonable, but it doesn't match:

https://drafts.csswg.org/web-animations/#calculating-computed-keyframes

In particular, the rule (4), which this test tests, is useless, since (3) resolves any conflict between the two properties by sorting by name..

Yeah, steps (3) and (4) there should be switched (or perhaps (4) should be moved before (2)).

Filed spec issue: https://github.com/w3c/csswg-drafts/issues/3532

Flags: needinfo?(bbirtles)
You need to log in before you can comment on or make changes to this bug.