773500 - nsStyleAnimation checks for common units of nsCSSValueTriplet are screwy

Status: RESOLVED FIXED

Description

[David Zbarsky (:dzbarsky)]

Created attachment 641646 [details] [diff] [review]
Patch

Comment 1

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Comment on attachment 641646 [details] [diff] [review]
Patch

I don't think ValueTriplet needs a URL check at all, and I suspect that ValuePair does in fact only need the URL check on [0] -- it's needed only for SVG paint servers.

But ValueTriplet should check against null for all three values -- and that check should be fixed in ComputeDistance in addition to AddWeighted.

(I remember pointing this out in another bug somewhere.)


This has come up before; not sure how we still managed not to fix it.  It would be great if you could write a revised patch.

Comment 2

[David Zbarsky (:dzbarsky)]

Created attachment 641738 [details] [diff] [review]
Patch

Yeah, I'm positive I've seen this issue discussed before.

Comment 3

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Comment on attachment 641738 [details] [diff] [review]
Patch

r=dbaron

Comment 4

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e62e0d2137b6

Comment 5

[:Ehsan Akhgari]

I backed this out because of mochitest-4 bustage.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfa35c4507fd

Example failure log: https://tbpl.mozilla.org/php/getParsedLog.php?id=13502718&tree=Mozilla-Inbound&full=1

Comment 6

[David Zbarsky (:dzbarsky)]

Created attachment 642061 [details] [diff] [review]
Followup

If we're going to null-check the zvalue, we should avoid setting it to null.
This change was already part of my transform-origin patch.

Comment 7

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8 (if account gets disabled due to email bounces, ask a bugzilla admin to reenable it)]

Comment on attachment 642061 [details] [diff] [review]
Followup

>From: David Zbarsky <[e-mail]>
>
>try: -b do

How about:

Store z-component of transform-origin as null rather than 0 when it is omitted.

r=dbaron with that



This should land before the other patch, I think.  Also, when you reland the other patch, don't mention nsCSSValuePair in the summary since the patch doesn't change it.

Comment 8

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/292c6ef7188f
https://hg.mozilla.org/integration/mozilla-inbound/rev/52471266a657

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/292c6ef7188f
https://hg.mozilla.org/mozilla-central/rev/52471266a657