1111440 - convert all in-tree style sheets logical properties from -moz-padding-start etc. to padding-inline-start etc.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Cameron McCormack (:heycam)]

Once we've added the new padding-inline-start etc. logical properties, we can change all occurrences of the old, prefixed ones in the tree to the new names.  (The old names will remain as aliases for the new names.)

Comment 1

[Cameron McCormack (:heycam)]

Well, the new logical properties are going to go behind a pref to begin with, so unless we enable them unconditionally in UA style sheets, we should wait until we're ready to enable the pref before converting everything in the tree.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We shipped these logical properties in bug 1118103 / bug 1138384, so we can do this now.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=655c964f69e0&group_state=expanded

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Replace -moz- prefixed logical margin/padding/border properties with their standard versions

This patch was generated by the command:
  find . -name "*.css" -exec sed -i -f mozpropsub {} \;
in the root of a mozilla-central tree, with the file mozpropsub
containing the contents:
s/-moz-padding-end\>/padding-inline-end/g
s/-moz-padding-start\>/padding-inline-start/g
s/-moz-margin-end\>/margin-inline-end/g
s/-moz-margin-start\>/margin-inline-start/g
s/-moz-border-end\>/border-inline-end/g
s/-moz-border-end-color\>/border-inline-end-color/g
s/-moz-border-end-style\>/border-inline-end-style/g
s/-moz-border-end-width\>/border-inline-end-width/g
s/-moz-border-start\>/border-inline-start/g
s/-moz-border-start-color\>/border-inline-start-color/g
s/-moz-border-start-style\>/border-inline-start-style/g
s/-moz-border-start-width\>/border-inline-start-width/g

While I didn't manually review all the changes, I did review the list of
files, and manually reviewed the changes in the files that I thought
were more interesting.

Note that there are a few tests that should be fixed up as well, but
I'll do that in a later patch.

MozReview-Commit-ID: EiQTuuV0MNQ

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8751517 [details] [diff] [review]
Replace -moz- prefixed logical margin/padding/border properties with their standard versions

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

I skimmed through the changed lines, and the sed script looks good.

::: browser/themes/shared/incontentprefs/preferences.inc.css
@@ +255,5 @@
>  #weavePrefsDeck > vbox > #pairDevice > label,
>  #weavePrefsDeck > #needsUpdate > hbox > #loginError,
>  #weavePrefsDeck > #hasFxaAccount > vbox > label,
>  #weavePrefsDeck > #hasFxaAccount > hbox:not(#tosPP-normal) > label {
>    /* no margin-start for elements at the begin of a line */

s/margin-start/margin-inline-start/ in the comment, and then s/begin/beginning/ while you're there.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/fx-team/rev/e56017a57ea7b6068766a825698077451ab78710
Bug 1111440 - Replace -moz- prefixed logical margin/padding/border properties with their standard versions.  r=heycam

https://hg.mozilla.org/integration/fx-team/rev/943ca7ad1ea35e01a6ba6bfa0a4f3896f1150e15
Bug 1111440 - Manual comment changes along with replacing -moz- prefixed logical margin/padding/border properties with their standard versions.  r=heycam

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e56017a57ea7
https://hg.mozilla.org/mozilla-central/rev/943ca7ad1ea3

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Replace rest of -moz-/Moz prefixed logical margin/padding/border properties with their standard versions

This patch was generated by the command:
  find ??* -type f -exec sed -i -f ../mozpropsub {} \;
in the root of the repository, with the file ../mozpropsub containing:
s/-moz-padding-end\>/padding-inline-end/g
s/-moz-padding-start\>/padding-inline-start/g
s/-moz-margin-end\>/margin-inline-end/g
s/-moz-margin-start\>/margin-inline-start/g
s/-moz-border-end\>/border-inline-end/g
s/-moz-border-end-color\>/border-inline-end-color/g
s/-moz-border-end-style\>/border-inline-end-style/g
s/-moz-border-end-width\>/border-inline-end-width/g
s/-moz-border-start\>/border-inline-start/g
s/-moz-border-start-color\>/border-inline-start-color/g
s/-moz-border-start-style\>/border-inline-start-style/g
s/-moz-border-start-width\>/border-inline-start-width/g
s/\<MozPaddingEnd\>/paddingInlineEnd/g
s/\<MozPaddingStart\>/paddingInlineStart/g
s/\<MozMarginEnd\>/marginInlineEnd/g
s/\<MozMarginStart\>/marginInlineStart/g
s/\<MozBorderEnd\>/borderInlineEnd/g
s/\<MozBorderEndColor\>/borderInlineEndColor/g
s/\<MozBorderEndStyle\>/borderInlineEndStyle/g
s/\<MozBorderEndWidth\>/borderInlineEndWidth/g
s/\<MozBorderStart\>/borderInlineStart/g
s/\<MozBorderStartColor\>/borderInlineStartColor/g
s/\<MozBorderStartStyle\>/borderInlineStartStyle/g
s/\<MozBorderStartWidth\>/borderInlineStartWidth/g

The diffs for the following files:
  layout/style/nsCSSPropAliasList.h
  layout/style/test/property_database.js
  layout/style/test/test_value_computation.html
were then manually removed from the patch.

MozReview-Commit-ID: 8fbYnlZcn9U

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a3c5bd665af&group_state=expanded

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8752374 [details] [diff] [review]
Replace rest of -moz-/Moz prefixed logical margin/padding/border properties with their standard versions

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

> find ??* -type f -exec sed -i -f ../mozpropsub {} \;

Was the "??*" to deliberately skip db/ and js/?

I'm a bit wary of changing fuzzer-created tests like layout/generic/crashtests/767765.html, which might well be relying on that specific content.  Maybe skip that one?

::: devtools/client/shared/widgets/AbstractTreeItem.jsm
@@ +48,5 @@
>   *     node.appendChild(arrowNode);
>   *     ...
>   *     // Use `this.itemDataSrc` to customize the tree item and
>   *     // `this.level` to calculate the indentation.
> + *     node.marginInlineStart = (this.level * 10) + "px";

This comment line looks wrong.  If you care to, can you (in a separate patch) make that read "node.style.marginLineStart"?

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #10)
> Was the "??*" to deliberately skip db/ and js/?

Er, that was just silly.  It doesn't skip db and js.  But ".??*" is a trick to skip "." and ".."; "*" would have been fine; I just needed to use "*" rather than "." to avoid .hg.

> I'm a bit wary of changing fuzzer-created tests like
> layout/generic/crashtests/767765.html, which might well be relying on that
> specific content.  Maybe skip that one?

I'd rather make the change; if it relied on the property being set, it will break when we remove it.

> This comment line looks wrong.  If you care to, can you (in a separate
> patch) make that read "node.style.marginLineStart"?

I'll look at this more closely in the morning.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In particular, the odd thing about the last comment is that devtools/client/performance/test/helpers/synth-utils.js , which is not in a comment, has the same issue.)

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cdc7ecf62c5df89e32ed69a5a17e4fd89ee22170
Bug 1111440 - Replace rest of -moz-/Moz prefixed logical margin/padding/border properties with their standard versions.  r=heycam

Comment 14

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) from comment #11)
> I'd rather make the change; if it relied on the property being set, it will
> break when we remove it.

OK.  I guess it won't break, but will just continue not to crash.  (When I said "relying on that specific content" I meant more that before the fix, we might have been crashing due to e.g. particular document content lengths.  But you are right that once we remove the alias, we might make the crashtest no longer test the thing it was testing originally, but in a different way from the change due to switching to the official property name now.)

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Rather than just land a second patch, I moved that issue to bug 1272857 to get proper feedback on it.

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cdc7ecf62c5d