Closed Bug 1519944 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

[css-logical] Implement the margin-block/inline shorthands
8.59 KB, patch
emilio
: review+
Details | Diff | Splinter Review

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

https://hg.mozilla.org/mozilla-central/rev/382bf54cf7cf

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox66: affected → fixed
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.

Attachment

General

Creator:
Created:
Updated:
Size: