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

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: heycam, Assigned: dbaron)

Tracking

(Blocks 1 bug)

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(2 attachments)

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.)
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.
We shipped these logical properties in bug 1118103 / bug 1138384, so we can do this now.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
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
Attachment #8751517 - Flags: review?(cam)
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.
Attachment #8751517 - Flags: review?(cam) → review+
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
Depends on: 1272608
Depends on: 1272606
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 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"?
Attachment #8752374 - Flags: review?(cam) → review+
(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.
(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.)
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
(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.)
Rather than just land a second patch, I moved that issue to bug 1272857 to get proper feedback on it.
Blocks: 1272921
https://hg.mozilla.org/mozilla-central/rev/cdc7ecf62c5d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.