[css-logical] rename offset-* properties to inset-block-start, inset-block-end, inset-inline-start and inset-inline-end

RESOLVED FIXED in Firefox 63

Status

()

P2
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: rego, Assigned: emilio)

Tracking

(Blocks: 1 bug, {dev-doc-complete, site-compat})

unspecified
mozilla63
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(8 attachments)

(Reporter)

Description

6 months ago
Firefox already supports offset-block-start, offset-block-end, offset-inline-start and offset-inline-end.

But the "offset-" prefix has been renamed to "inset-":
https://github.com/w3c/csswg-drafts/issues/1519

Spec: https://drafts.csswg.org/css-logical-1/#inset-properties
(Reporter)

Updated

6 months ago
Blocks: 1323940
Priority: -- → P3
Given that this is just a renaming, I think this should be somewhat higher priority since we don't want the web to get stuck depending on outdated names.
Status: UNCONFIRMED → NEW
Depends on: 1120283
Ever confirmed: true
Priority: P3 → P2
Summary: [css-logical] Implement inset-block-start, inset-block-end, inset-inline-start and inset-inline-end → [css-logical] rename offset-* properties to inset-block-start, inset-block-end, inset-inline-start and inset-inline-end
(Assignee)

Comment 2

5 months ago
I agree. I guess we need to land an alias at least to leave activity-stream working until they update. David, do you think we should try to remove the alias shortly after this? (I don't think sites depend on this right now since we're the only implementer, but I don't have data on this...).
Assignee: nobody → emilio
Flags: needinfo?(dbaron)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Maybe it makes sense to put the alias behind a pref and flip that pref off as soon as possible, but ship a release with the pref in the code (but off) before actually removing the code?  (It's easier to ship a pref-flip if we need to than to ship a new release.)
Flags: needinfo?(dbaron)

Comment 10

5 months ago
mozreview-review
Comment on attachment 8987936 [details]
Bug 1464782: Rename offset-* properties to inset-*.

https://reviewboard.mozilla.org/r/253218/#review259876

::: servo/components/style/properties/longhands/position.mako.rs:32
(Diff revision 1)
> -        "offset-%s" % side,
> +        "inset-%s" % side,
>          "LengthOrPercentageOrAuto",
>          "computed::LengthOrPercentageOrAuto::Auto",
> -        spec="https://drafts.csswg.org/css-logical-props/#propdef-offset-%s" % side,
> +        spec="https://drafts.csswg.org/css-logical-props/#propdef-inset-%s" % side,
>          flags="GETCS_NEEDS_LAYOUT_FLUSH",
> +        alias="offset-%s" % side,

I agree with dbaron that we should probably put it behind a pref which would make it easier to control.

You can see https://searchfox.org/mozilla-central/rev/14cb8f1c238735ba1abe18ad44e39808983c2572/servo/components/style/properties/longhands/box.mako.rs#630 as an example for how to put an alias behind a pref.
Attachment #8987936 - Flags: review?(xidorn+moz) → review+

Comment 11

5 months ago
mozreview-review
Comment on attachment 8987938 [details]
Bug 1464782: Update references to the properties in layout/.

https://reviewboard.mozilla.org/r/253222/#review259880
Attachment #8987938 - Flags: review?(xidorn+moz) → review+

Comment 12

5 months ago
mozreview-review
Comment on attachment 8987939 [details]
Bug 1464782: Some manual fixups to be squashed above.

https://reviewboard.mozilla.org/r/253224/#review259882

::: layout/style/test/property_database.js:6198
(Diff revision 1)
> -  } else if (/^offset-(block|inline)-(start|end)/.test(property)) {
> -    property = property.substring(7);  // we want "top" not "offset-top", e.g.
> +  } else if (/^(offset|inset)-(block|inline)-(start|end)/.test(property)) {
> +    property = property.replace("offset-", "").replace("inset-", "");  // we want "top" not "offset-top", e.g.

Why do we still have `offset-` here? Shouldn't they all have been replaced with `inset-` in this database?

::: layout/style/test/test_logical_properties.html:97
(Diff revision 1)
>  
>      if ((type == CSS_TYPE_SHORTHAND_AND_LONGHAND ||
>           type == CSS_TYPE_LONGHAND && gCSSProperties[p].logical) &&
>          /-inline-end/.test(p)) {
>        var valueType;
> -      if (/margin|padding|width|offset/.test(p)) {
> +      if (/margin|padding|width|inset|offset/.test(p)) {

Again, why do we still need offset?

::: layout/style/test/test_logical_properties.html:113
(Diff revision 1)
>          inlineEnd:   p,
>          blockStart:  p.replace("-inline-end", "-block-start"),
>          blockEnd:    p.replace("-inline-end", "-block-end"),
>          type:        valueType
>        };
> -      if (/^offset/.test(p)) {
> +      if (/^offset|inset/.test(p)) {

In addition to the general offset problem, this regex doesn't work as it intended to do.

`/^offset|inset/` means `/(^offset)|inset/` which means it matches `offset*` and `*inset*`, while it should probably match only `inset*`. That should be `/^(?:offset|inset)/` instead.

But still, I wonder why we still need to test `offset` here. If you remove that, you wouldn't have any problem here.
Attachment #8987939 - Flags: review?(xidorn+moz)

Comment 13

5 months ago
mozreview-review
Comment on attachment 8987940 [details]
Bug 1464782: Test the aliases, for now.

https://reviewboard.mozilla.org/r/253226/#review259884

::: layout/style/test/property_database.js:6040
(Diff revision 1)
>        "calc(25px*3)",
>        "calc(3*25px + 50%)",
>      ],
>      invalid_values: []
>    },
> +  "offset-block-start": {

Oh I guess this is the reason you need to deal with `offset-` in the previous patch? Makes sense.

We can probably put it behind a pref as suggested before.
Attachment #8987940 - Flags: review?(xidorn+moz) → review+

Comment 14

5 months ago
mozreview-review-reply
Comment on attachment 8987939 [details]
Bug 1464782: Some manual fixups to be squashed above.

https://reviewboard.mozilla.org/r/253224/#review259882

> In addition to the general offset problem, this regex doesn't work as it intended to do.
> 
> `/^offset|inset/` means `/(^offset)|inset/` which means it matches `offset*` and `*inset*`, while it should probably match only `inset*`. That should be `/^(?:offset|inset)/` instead.
> 
> But still, I wonder why we still need to test `offset` here. If you remove that, you wouldn't have any problem here.

I guess the next patch should really be put before this patch so that some of the changes here makes more sense.

Anyway, this regex is still a problem.

Comment 15

5 months ago
mozreview-review
Comment on attachment 8987939 [details]
Bug 1464782: Some manual fixups to be squashed above.

https://reviewboard.mozilla.org/r/253224/#review259888
Attachment #8987939 - Flags: review+

Comment 16

5 months ago
mozreview-review
Comment on attachment 8987941 [details]
Bug 1464782: Update references in the rest of the tree.

https://reviewboard.mozilla.org/r/253228/#review259890
Attachment #8987941 - Flags: review?(xidorn+moz) → review+

Comment 17

5 months ago
mozreview-review
Comment on attachment 8987937 [details]
Bug 1464782: Update a WPT test which is using the offset properties.

https://reviewboard.mozilla.org/r/253220/#review259878
Attachment #8987937 - Flags: review?(xidorn+moz) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

5 months ago
mozreview-review
Comment on attachment 8988102 [details]
Bug 1464782: Put it behind a pref.

https://reviewboard.mozilla.org/r/253356/#review259976
Attachment #8988102 - Flags: review?(xidorn+moz) → review+

Comment 26

5 months ago
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7be80c866e84
Rename offset-* logical properties to inset-*. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/f65ca6e2fb79
Update references to offset-* properties in the rest of the tree. r=xidorn
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecffedeea0e2
Put offset-* aliases behind a pref. r=xidorn
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11689 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 29

5 months ago
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0b83e0056ac
Allow logical aliases in the property database. r=me
Oh, and there should be an intent to ship for this I believe (with an intent to unship for the old names).
Flags: needinfo?(emilio)
Upstream PR merged
(Assignee)

Updated

5 months ago
Blocks: 1471838
Keywords: dev-doc-needed
(Assignee)

Comment 34

5 months ago
Sent one for the new properties and another for the deprecation in bug 1471838.
Flags: needinfo?(emilio)
Keywords: site-compat
See Also: → bug 1476285

Comment 38

3 months ago
I've updated the docs for these properties, they can be found at:

https://developer.mozilla.org/en-US/docs/inset-block-start
https://developer.mozilla.org/en-US/docs/inset-block-end
https://developer.mozilla.org/en-US/docs/inset-inline-start
https://developer.mozilla.org/en-US/docs/inset-inline-end

Compat data has also been updated. A review is appreciated - thank you!
Flags: needinfo?(emilio)
(Assignee)

Comment 39

3 months ago
Thanks!

I haven't had the time to look super in-depth into all of them, but this is the only thing I found on the ones I did look up: The animation type says 'discrete', but that is no longer true since bug 1309752 / https://github.com/w3c/csswg-drafts/issues/2751.

Other than that it looks amazing, thanks for all your work Rachel :)
Flags: needinfo?(emilio)

Updated

3 months ago
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.