[css-logical] Implement the margin-block/inline shorthands
Categories
(Core :: CSS Parsing and Computation, enhancement, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox66 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file)
|
8.59 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
|
Assignee | |
Comment 1•5 years ago
|
||
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)
Is this a valid test?
If so, do we already have a bug for it open somewhere?
|
|
Assignee | |
Comment 2•5 years ago
|
||
:boris maybe you know the answer to my question about the animation test above?
|
||
Comment 3•5 years ago
|
||
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..
|
|
||
Comment 4•5 years ago
|
||
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.
|
||
Comment 5•5 years ago
•
|
||
(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.
|
|
Assignee | |
Comment 6•5 years ago
|
||
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
|
||
Comment 8•5 years ago
|
||
| bugherder | ||
|
||
Comment 9•5 years ago
|
||
(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
|
|
||
Comment 10•5 years ago
|
||
Spec issue resolved in: https://github.com/w3c/csswg-drafts/commit/3eba53b04f24a28348cc1e058459277b4a2fbe17
|
||
Comment 11•5 years ago
|
||
Docs completed; Rachel Andrew created the docs a while ago, and recently updated the compat data too:
https://developer.mozilla.org/en-US/docs/Web/CSS/margin-block#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline#Browser_compatibility
I also added a note to the Fx66 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/66#CSS
Description
•