734953 - Remove skew() transformation function (leaving skewX() and skewY())

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The skew() transform is misleadingly named and it's not clear what it's useful for, if anything:

http://lists.w3.org/Archives/Public/public-fx/2012JanMar/0156.html

Would Gecko be willing to try dropping support for it?  Do we know if any pages are using it?  It's interoperably implemented in all browsers, so it might well not be worth the pain to get rid of it even if it's useless.

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The spec was updated to remove skew(): https://www.w3.org/Bugs/Public/show_bug.cgi?id=16300

Do we want to remove support entirely, or just when we unprefix?

Comment 2

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

I think we should remove support ASAP.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v1

I looked for all mentions of "skew" in layout/style/.  Should I look anywhere else?  I'm not sure I got all the tests, but the try server will tell me that . . .

Comment 4

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 607015
	Branch: mozilla-central => try
Patch 607015 could not be applied to mozilla-central.
patching file layout/style/test/property_database.js
Hunk #1 FAILED at 1013
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/property_database.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patch 607015 could not be applied to mozilla-central.
patching file layout/reftests/transform/compound-1-fail.html
Hunk #1 FAILED at 10
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/compound-1-fail.html.rej
patching file layout/reftests/transform/compound-1-ref.html
Hunk #1 FAILED at 17
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/compound-1-ref.html.rej
patching file layout/reftests/transform/compound-1a.html
Hunk #1 FAILED at 14
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/compound-1a.html.rej
patching file layout/reftests/transform/reftest.list
Hunk #1 FAILED at 76
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/reftest.list.rej
unable to find 'layout/reftests/transform/skew-1-ref.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-1-ref.html.rej
unable to find 'layout/reftests/transform/skew-1a.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-1a.html.rej
unable to find 'layout/reftests/transform/skew-1b.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-1b.html.rej
unable to find 'layout/reftests/transform/skew-2-ref.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-2-ref.html.rej
unable to find 'layout/reftests/transform/skew-2a.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-2a.html.rej
patching file layout/style/nsCSSKeywordList.h
Hunk #1 FAILED at 413
1 out of 1 hunks FAILED -- saving rejects to file layout/style/nsCSSKeywordList.h.rej
patching file layout/style/nsCSSParser.cpp
Hunk #1 FAILED at 7929
1 out of 1 hunks FAILED -- saving rejects to file layout/style/nsCSSParser.cpp.rej
patching file layout/style/nsStyleAnimation.cpp
Hunk #1 FAILED at 1005
Hunk #2 FAILED at 1108
Hunk #3 FAILED at 1523
Hunk #4 FAILED at 1564
Hunk #5 FAILED at 2024
5 out of 5 hunks FAILED -- saving rejects to file layout/style/nsStyleAnimation.cpp.rej
patching file layout/style/nsStyleTransformMatrix.cpp
Hunk #1 FAILED at 416
Hunk #2 FAILED at 590
2 out of 2 hunks FAILED -- saving rejects to file layout/style/nsStyleTransformMatrix.cpp.rej
patching file layout/style/test/property_database.js
Hunk #1 FAILED at 1013
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/property_database.js.rej
patching file layout/style/test/test_bug721136.html
Hunk #1 FAILED at 18
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/test_bug721136.html.rej
patching file layout/style/test/test_transitions_per_property.html
Hunk #1 FAILED at 1410
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/test_transitions_per_property.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.

Comment 5

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

Comment on attachment 607015 [details] [diff] [review]
Patch v1

>-      // It would probably be nicer to animate skew in tangent space
>-      // rather than angle space.  However, it's easy to specify
>-      // skews with infinite tangents, and behavior changes pretty
>-      // drastically when crossing such skews (since the direction of
>-      // animation flips), so interop is probably more important here.

This comment (in nsStyleAnimation.cpp) still applies to skewx and skewy.  Leave it.

>@@ -2025,18 +1944,17 @@ nsStyleAnimation::AddWeighted(nsCSSPrope
>       return true;
>     }
>     case eUnit_Transform: {
>       const nsCSSValueList *list1 = aValue1.GetCSSValueListValue();
>       const nsCSSValueList *list2 = aValue2.GetCSSValueListValue();
> 
>       // We want to avoid the matrix decomposition when we can, since
>       // avoiding it can produce better results both for compound
>-      // transforms and for skew and skewY (see below).  We can do this
>-      // in two cases:
>+      // transforms and for skewY (see below).  We can do this in two cases:

Wrap "cases:" to the next line to match the existing wrapping (which is likely tw=72).

r=dbaron with those fixed

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v2

(In reply to David Baron [:dbaron] from comment #5)
> Wrap "cases:" to the next line to match the existing wrapping (which is
> likely tw=72).

Is tw=72 standard at Mozilla?  I generally use tw=79.  All I found is <https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Line_length>, which is somewhat nonspecific.

Comment 7

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 607182
	Branch: mozilla-central => try
Patch 607182 could not be applied to mozilla-central.
patching file layout/style/test/property_database.js
Hunk #1 FAILED at 1013
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/property_database.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patch 607182 could not be applied to mozilla-central.
patching file layout/reftests/transform/compound-1-fail.html
Hunk #1 FAILED at 10
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/compound-1-fail.html.rej
patching file layout/reftests/transform/compound-1-ref.html
Hunk #1 FAILED at 17
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/compound-1-ref.html.rej
patching file layout/reftests/transform/compound-1a.html
Hunk #1 FAILED at 14
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/compound-1a.html.rej
patching file layout/reftests/transform/reftest.list
Hunk #1 FAILED at 76
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/reftest.list.rej
unable to find 'layout/reftests/transform/skew-1-ref.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-1-ref.html.rej
unable to find 'layout/reftests/transform/skew-1a.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-1a.html.rej
unable to find 'layout/reftests/transform/skew-1b.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-1b.html.rej
unable to find 'layout/reftests/transform/skew-2-ref.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-2-ref.html.rej
unable to find 'layout/reftests/transform/skew-2a.html' for patching
1 out of 1 hunks FAILED -- saving rejects to file layout/reftests/transform/skew-2a.html.rej
patching file layout/style/nsCSSKeywordList.h
Hunk #1 FAILED at 413
1 out of 1 hunks FAILED -- saving rejects to file layout/style/nsCSSKeywordList.h.rej
patching file layout/style/nsCSSParser.cpp
Hunk #1 FAILED at 7929
1 out of 1 hunks FAILED -- saving rejects to file layout/style/nsCSSParser.cpp.rej
patching file layout/style/nsStyleAnimation.cpp
Hunk #1 FAILED at 1005
Hunk #2 FAILED at 1108
Hunk #3 FAILED at 1523
Hunk #4 FAILED at 1569
Hunk #5 FAILED at 2024
5 out of 5 hunks FAILED -- saving rejects to file layout/style/nsStyleAnimation.cpp.rej
patching file layout/style/nsStyleTransformMatrix.cpp
Hunk #1 FAILED at 416
Hunk #2 FAILED at 590
2 out of 2 hunks FAILED -- saving rejects to file layout/style/nsStyleTransformMatrix.cpp.rej
patching file layout/style/test/property_database.js
Hunk #1 FAILED at 1013
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/property_database.js.rej
patching file layout/style/test/test_bug721136.html
Hunk #1 FAILED at 18
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/test_bug721136.html.rej
patching file layout/style/test/test_transitions_per_property.html
Hunk #1 FAILED at 1410
1 out of 1 hunks FAILED -- saving rejects to file layout/style/test/test_transitions_per_property.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir

Patchset could not be applied and pushed.

Comment 8

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v3

Oops -- it was conflicting with the bug 719054 patch I have applied locally.  I should make sure any patches I submit are bottommost before I post them.

Comment 9

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 607192
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=04c24b87c645
Try run started, revision 04c24b87c645. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=04c24b87c645

Comment 10

[Mozilla RelEng Bot]

Try run for 04c24b87c645 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=04c24b87c645
Results (out of 217 total builds):
    exception: 2
    success: 159
    warnings: 40
    failure: 14
    other: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-04c24b87c645

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch v4

Fixed test failures in test_bug435293-skew.html.  Otherwise the same as the last version.  Will re-request review when I get a green try run.

Comment 12

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 607617
	Branch: mozilla-central => try
An error occurred while cloning https://hg.mozilla.org/mozilla-central.

Comment 13

[Mozilla RelEng Bot]

Autoland Patchset:
	Patches: 607617
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=6f350f60aa0f
Try run started, revision 6f350f60aa0f. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=6f350f60aa0f

Comment 14

[Mozilla RelEng Bot]

Try run for ce075b94be45 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ce075b94be45
Results (out of 217 total builds):
    success: 175
    warnings: 28
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ce075b94be45

Comment 15

[Mozilla RelEng Bot]

Try run for 6f350f60aa0f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6f350f60aa0f
Results (out of 219 total builds):
    exception: 1
    success: 178
    warnings: 26
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6f350f60aa0f
 Timed out after 12 hours without completing.

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 607617 [details] [diff] [review]
Patch v4

(In reply to Aryeh Gregor from comment #11)
> Created attachment 607617 [details] [diff] [review]
> Patch v4
> 
> Fixed test failures in test_bug435293-skew.html.  Otherwise the same as the
> last version.  Will re-request review when I get a green try run.

Okay, re-requesting review for the test changes.

Comment 17

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

Comment on attachment 607617 [details] [diff] [review]
Patch v4

r=dbaron

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b48e42774f6

Comment 19

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/8b48e42774f6

Comment 20

[Anthony Ricaud (:rik)]

I've noticed a broken page due to this change.

http://sudweb.fr/2012/ Below the "Inscrivez-vous" box, skew() is used to create an arrow. The source code of this website is available so I can remove it. Just wondering if this is a sign that this change is affecting real world websites.

Comment 21

[Jean-Yves Perrier [:teoli]]

Updated https://developer.mozilla.org/en/Firefox_14_for_developers and https://developer.mozilla.org/en/CSS/transform .

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(FTR, the patch for bug 747637 accidentally landed with this bug's number: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad1dadae3427 )