Closed Bug 1464782 Opened 6 years ago Closed 6 years ago

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

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: rego, Assigned: emilio)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(8 files)

Bug 1464782: Rename offset-* properties to inset-*.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1464782: Update a WPT test which is using the offset properties.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1464782: Update references to the properties in layout/.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1464782: Some manual fixups to be squashed above.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1464782: Test the aliases, for now.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1464782: Update references in the rest of the tree.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1464782: Put it behind a pref.
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4217
52 bytes, text/x-github-pull-request
Details | Review 
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
Blocks: css-logical-1
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
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)
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 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 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 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 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 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 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 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 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 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+
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.
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)
Blocks: 1471838
Sent one for the new properties and another for the deprecation in bug 1471838.
Flags: needinfo?(emilio)
Keywords: site-compat
See Also: → 1476285
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)
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)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: