We need to integrate CSS Filters with CSS Animations and CSS Transitions. http://www.w3.org/TR/filter-effects/#integration-with-css-animations-transitions

This is an initial patch for interpolation of CSS Filter properties. It still depends on other patches like drop-shadow and keyword table patches that were not landed yet. I would like to get some early feedback if the patch is going in the right direction or needs some major redesign. The patch introduces animation of basic filter functions (all but URL and drop-shadow). It has a basic implementation of a test suite for filter animations. I am happy for a hints on this as well. Right now it is very primitive. I did not implement ComputeDistance. I am not sure how much of a use it would be. If it should be implemented, can this be done in a follow up patch?

Seeking for the same feedback as described in previous comment. Just "rebaselined" the patch after fixing bug 898175.

Comment on attachment 787597 [details] [diff] [review] Filter animation without drop-shadow (In reply to Dirk Schulze from comment #1) > Created attachment 785829 [details] [diff] [review] > filter-animation-moz.patch > > This is an initial patch for interpolation of CSS Filter properties. It > still depends on other patches like drop-shadow and keyword table patches > that were not landed yet. > > I would like to get some early feedback if the patch is going in the right > direction or needs some major redesign. Based on a quick look, it seems roughly like what I'd expect the code to look like. I'm curious if there were any specific design questions you had in mind, though. > The patch introduces animation of basic filter functions (all but URL and > drop-shadow). > It has a basic implementation of a test suite for filter animations. I am > happy for a hints on this as well. Right now it is very primitive. > > I did not implement ComputeDistance. I am not sure how much of a use it > would be. If it should be implemented, can this be done in a follow up patch? I think not implementing should be ok, as long as these aren't exposed to SMIL animation as something that would expect to support paced animation. ># HG changeset patch ># User Dirk Schulze <krit@webkit.org> > >diff --git a/layout/style/nsStyleAnimation.cpp b/layout/style/nsStyleAnimation.cpp >+static void >+AddCSSValuePercentNumber(const uint32_t aValueRestrictions, >+ double aCoeff1, const nsCSSValue &aValue1, >+ double aCoeff2, const nsCSSValue &aValue2, >+ nsCSSValue &aResult) >+{ >+ nsCSSUnit u1 = aValue1.GetUnit(); >+ nsCSSUnit u2 = aValue2.GetUnit(); >+ NS_ABORT_IF_FALSE(u1 == eCSSUnit_Number || u1 == eCSSUnit_Percent, >+ "unexpected unit"); >+ NS_ABORT_IF_FALSE(u2 == eCSSUnit_Number || u2 == eCSSUnit_Percent, >+ "unexpected unit"); >+ if (u1 == eCSSUnit_Percent && u2 == eCSSUnit_Percent) { >+ AddCSSValuePercent(aCoeff1, aValue1, aCoeff2, aValue2, >+ aResult, aValueRestrictions); >+ } else { >+ float n1 = (u1 == eCSSUnit_Number) ? >+ aValue1.GetFloatValue() : aValue1.GetPercentValue(); >+ float n2 = (u2 == eCSSUnit_Number) ? >+ aValue2.GetFloatValue() : aValue2.GetPercentValue(); >+ aResult.SetFloatValue(RestrictValue(aValueRestrictions, >+ aCoeff1 * n1 + aCoeff2 * n2), >+ eCSSUnit_Number); >+ } >+} This seems rather weird. If there's a case where percentages and numbers are semantically the same, I'd expect you to only have one of them in the computed data in nsStyleStruct, and thus to maintain the invariant that you only have one of them in the nsStyleAnimation::Value objects, and thus to not need this function. >diff --git a/layout/style/test/test_transitions_per_property.html b/layout/style/test/test_transitions_per_property.html In the tests, it seems worth having some tests to test things that can't be animated. Perhaps also some tests covering some additional edge cases, if there are more to cover.

Comment on attachment 787597 [details] [diff] [review] Filter animation without drop-shadow Er, wait, I thought I was plussing a feedback request, not a review request.

Comment on attachment 787597 [details] [diff] [review] Filter animation without drop-shadow Actually, could you address the above and then request review from dholbert?

Also, I'd expect you to convert all the angles to a single type of angle units -- you ought to be able to animate between different angle units, and with this patch it looks like you can't. (Probably just document in nsStyleAnimation:Value that angles should always be in radians, fix StyleCoordToCSSValue to use GetAngleValueInRadians for all four angle units.) And add a test!

Oh, never mind, AddCSSValueAngle already handles that. But still add the tests.

Oh, you have two tests testing it in there, never mind.

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #6) > Comment on attachment 787597 [details] [diff] [review] > Filter animation without drop-shadow > > Actually, could you address the above and then request review from dholbert? Thanks a lot for taking the time to review the patch. I already have some test with not animatable values like URIs and (currently) drop-shadow. I'll look if I can add more edge cases. Currently I am investigating in a failure on the build bots in one of the SMIL tests on 'filter'. For some reason the URI value is not excepted and replaced by the keyword 'none'. A simplified test case that has SMIL animations on filter and gives the computedStyle at the beginning, middle and end works fine. I need to find the main difference. I'll upload a new patch when I fixed the problem. https://tbpl.mozilla.org/?tree=Try&rev=1a88157a23be

So from a bit of GDB debugging, it looks like we're accepting the filter value just fine most of the way through, but then when we end up producing a string that gets put into the style system via nsSMILMappedAttribute::SetAnimValue() and subsequently ParseMappedAttrAnimValueCallback, it's improperly-terminated for some reason. This is visible in the debug-build mochitest output in your Try run -- it's got tons of "###!!! ASSERTION: data should be null terminated: 'data[len] == PRUnichar(0)', file ../../../../xpcom/string/src/nsSubstring.cpp, line 250" inside of ParseMappedAttrAnimValueCallback. So the question is why we're ending up with a non-null-terminated string there.

Also: the most proximate reason that the URL gets rejected is displayed in the error console, as a parse error: { [12:07:42.572] Expected end of value but found '�'. Error in parsing value for 'filter'. Declaration dropped. @ http://mochi.test:8888/tests/content/smil/test/test_smilMappedAttrFromTo.xhtml?autorun=1&closeWhenDone=1&consoleLevel=INFO&failureFile=/scratch/work/builds/mozilla-central/mozilla-central.12-12-07.08-38/obj/.mozbuild/mochitest_failures.json } (That happens after we feed the improperly-terminated ParseMappedAttrAnimValueCallback-produced string into the style system.) The reason your code is affecting the type of string we produce here is that formerly, we probably took the "// Just capture the specified value" UnparsedString code-path in nsStyleAnimation::ComputeValue, which just got directly returned in ::UncomputeValue (for use in SetAnimValue & then ParseMappedAttrAnimValueCallback). With your patch, though, we'll actually be parsing the filter string, and then generating a string on-the-fly in ::UncomputeValue. And apparently that string may have issues, or violate expectations, or something. (?)

(Also: one reason you may be seeing different results in the mochitest vs. your attempts at a standalone testcase could be that mochitest URLs are really long and have some special characters (?,&,= at least) in them -- so your filter string value ends up being huge & interesting in the mochitest case, vs. shorter & less-interesting in your standalone testcase.)

I attach an simple example. It is mainly the test suite, stripped down to its core. (Surprising how small the test actually is when you look at the whole suite.) And now it gets really weird" The test as attached works perfectly fine with my Nightly build. I saved the content as SVG-animation.xhtml. I was extremely surprised that the same test (really exactly the same; character by character) failed when I saved it as test_smilCSSFromTo.xhtml. And as you point out, the passed filter function name is not recognized by the CSS Parser. It is just a random byte stream (possibly because it is not null terminated as the assertion warns). However, when you have the test file test_smilCSSFromTo.xhtml and save or copy it with a different name (say SVG-animation.xhtml) then it starts working again!

I should also mention that it just happens if the attributeType attribute on the animate element is set to XML. It works fine if it is set to CSS. <animate begin="1.0" dur="1.0" attributeName="filter" attributeType="CSS" from="url(#idA)" to="url(#idB)"/> works <animate begin="1.0" dur="1.0" attributeName="filter" attributeType="XML" from="url(#idA)" to="url(#idB)"/> doesn't. I checked the code in nsStyleAnimation, nsCSSParser and nsCSSScanner. * nsStyleAnimation just passes it to the parser * nsCSSParser passes it to nsCSSScanner and uses it for supporting unitless <length> values. * nsCSSScanner needs it for special casing comments (not allowed for presentation attributes in FF) and to create a exponent number token. No decoding switch here. nsStyleAnimation is called here: http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILCSSValueType.cpp#360 This code sets isSVGMode to true and is called in both cases, for attributeType="CSS" and for attributeType="XML". You can also see it in the following example: <animate attributeType="CSS" attributeName="stroke-width" from="0.1e1" to="0.1e2" dur="1.0" begin="1.0"/> This works even if from and to have exponent values but animate the CSS property. My assumption at this point is that the decoding of the string aString is different.

I debugged the strings from http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILCSSValueType.cpp#360 and they are "url(#idA)" and "url(#idB)" in ASCII. The tokenizer has "#idA" and "#idB", CSS parsing is successful as well and generates the right nsCSSValue.

With the switch from CSS -> XML on attributeType, MappedAttrParser::ParseMappedAttrValue is called (from ParseMappedAttrAnimValueCallback). This is not the case for CSS. The attribute value has a String of the form 'url("http://...#idB")' all together 65 chars and the next position is null terminated. However, the length is of the nsAString is still specified with 127. The code that calls ParseMappedAttrAnimValueCallback seems to be generated and it is unclear to me how the string is created and why the length of character is not the specified length. I would need some help to figure out the caller site.

So ParseMappedAttrAnimValueCallback is called on a string that's stored as in the element's property table (nothing to do with CSS properties - it's basically just a per-element hash-map that stores infrequently-needed data). That happens here: > 1345 doc->PropertyTable(SMIL_MAPPED_ATTR_ANIMVAL)-> > 1346 Enumerate(this, ParseMappedAttrAnimValueCallback, &mappedAttrParser); http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGElement.cpp#1345 The actual string value comes from here, in nsSMILMappedAttribute::SetAnimValue(): > 88 nsAutoString valStr; > 89 if (!nsSMILCSSValueType::ValueToString(aValue, valStr)) { [...] > 102 nsStringBuffer* valStrBuf = > 103 nsCSSValue::BufferFromString(nsString(valStr)).get(); > 104 nsresult rv = mElement->SetProperty(SMIL_MAPPED_ATTR_ANIMVAL, > 105 attrName, valStrBuf, > 106 ReleaseStringBufferPropertyValue); http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILMappedAttribute.cpp#104 (Note that line 89 there ends up invoking nsStyleAnimation::UncomputeValue under-the-hood, to produce a string.) So, you want to see if |valStr| is looking sane in that code (in nsSMILMappedAttribute::SetAnimValue()) -- if it's fine there, then we must be doing something wrong when putting it into the property table or when taking it out.

valStr is an nsAutoString and returns 64 characters in mStorage (last null termination). The IRI is longer than this string, so that the data in mStorage just contain: url("file:....test_smilCSSFromTo.xhtml#id The substring A") is missing. mFixedCapacity is set to 63. That would explain the stripped string. Looking at UncomputeValue, the returned string is 66 characters and is correct. The member mData and mLength are set to the correct URL and 66 characters. I am not sure what mFixedCapacity is used for, but the stored data in mData are correct.

I just figured out that the problem is not the specific file name, but the overall length of the URL string. This might be related to mFixedCapacity. But I don't know. This is the URL when it fails: url("file:///Users/dschulze/Desktop/test_smilCSSFrom.xhtml#idA") If I shorten the name of the file: url("file:///Users/dschulze/Desktop/test_smilCSSFro.xhtml#idA") It works. If I change the position, to an upper directory it works as well: url("file:///Users/dschulze/test_smilCSSFrom.xhtml#idA") This string fails again: url("file:///Users/dschulze/abcdefghijklmnopqrstuvwx.xhtml#idA") Note that the length of the string is more than 64 characters again. I reproduced the same behavior with animation of fill and url(): [08:10:56.129] Expected color but found 'transparent�ꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥ'. Expected end of value but found 'transparent�ꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥꖥ'. Error in parsing value for 'fill'. Declaration dropped. @ file:///Users/dschulze/abcdefghijklmnopqrstuvwx.xhtml So this is unrelated to my patch, but just happened to show up on my patch.

While working on drop-shadow animation I realized that the code can be simplified more. This patch removes some helper functions that are not needed anymore and is generally more readable. (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #4) > >diff --git a/layout/style/nsStyleAnimation.cpp b/layout/style/nsStyleAnimation.cpp > > >+static void > >+AddCSSValuePercentNumber(const uint32_t aValueRestrictions, > >+ double aCoeff1, const nsCSSValue &aValue1, > >+ double aCoeff2, const nsCSSValue &aValue2, > >+ nsCSSValue &aResult) > >+{ > >+ nsCSSUnit u1 = aValue1.GetUnit(); > >+ nsCSSUnit u2 = aValue2.GetUnit(); > >+ NS_ABORT_IF_FALSE(u1 == eCSSUnit_Number || u1 == eCSSUnit_Percent, > >+ "unexpected unit"); > >+ NS_ABORT_IF_FALSE(u2 == eCSSUnit_Number || u2 == eCSSUnit_Percent, > >+ "unexpected unit"); > >+ if (u1 == eCSSUnit_Percent && u2 == eCSSUnit_Percent) { > >+ AddCSSValuePercent(aCoeff1, aValue1, aCoeff2, aValue2, > >+ aResult, aValueRestrictions); > >+ } else { > >+ float n1 = (u1 == eCSSUnit_Number) ? > >+ aValue1.GetFloatValue() : aValue1.GetPercentValue(); > >+ float n2 = (u2 == eCSSUnit_Number) ? > >+ aValue2.GetFloatValue() : aValue2.GetPercentValue(); > >+ aResult.SetFloatValue(RestrictValue(aValueRestrictions, > >+ aCoeff1 * n1 + aCoeff2 * n2), > >+ eCSSUnit_Number); > >+ } > >+} > > This seems rather weird. If there's a case where percentages and numbers > are semantically the same, I'd expect you to only have one of them in the > computed data in nsStyleStruct, and thus to maintain the invariant that you > only have one of them in the nsStyleAnimation::Value objects, and thus to > not need this function. > I changed the function so that it does always return a number as computed value. However, I kept the function since it seemed cleaner to transform percentage into number in a little helper function, together with different kind of animations where the initial value is 1 rather than zero. > > > >diff --git a/layout/style/test/test_transitions_per_property.html b/layout/style/test/test_transitions_per_property.html > > In the tests, it seems worth having some tests to test things that can't be > animated. Perhaps also some tests covering some additional edge cases, if > there are more to cover. I added more test that check animation on none-animatable code. (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #8) > Oh, never mind, AddCSSValueAngle already handles that. But still add the > tests. I added more tests that check animation between different angle values as well. The patch should not land until bug 904158 is fixed. I agree with dholbert that we do not want to skip the test unless it is a bigger burden on fixing this bug.

Comment on attachment 789523 [details] [diff] [review] filter-animation-moz.patch Review comments on the mochitest: >diff --git a/layout/style/test/test_transitions_per_property.html b/layout/style/test/test_transitions_per_property.html >+var filterTests = [ >+ // function from none (number/length) >+ { start: "none", end: "brightness(0)", >+ expected: "brightness(0.75)" }, [...] >+ // function to none (number/length) >+ { start: "brightness(0)", end: "none", >+ expected: "brightness(0.25)" }, [....] >+ // function to same function (number/length) >+ { start: "brightness(0)", end: "brightness(1)", >+ expected: "brightness(0.25)" }, Nit: "brightness(1)" (and its equivalent for the other filter functions) is unfortunately a bad value to use for testing "function to same function" interpolation. It's bad because if you (or someone editing this test in the future) make a typo, the test will still silently pass, since your typo value would be ignored and we'd use the initial value "none", which happens to be treated the same as "brightness(1)". In that situation, the test will pass, *without testing the function-to-function interpolation code that it's trying to test*. It'd be better to use different endpoints. e.g.: { start: "brightness(0.1)", end: "brightness(0.2)", expected: "brightness(0.125)" }, This is true of most of the "filterTests" entries after this one. I'd suggest replacing all of those "0 to 1" / "0 to 100%" / etc. tests with something like 0.1 to 0.2, to avoid the problem I described above. >@@ -952,16 +1098,55 @@ function test_border_color_transition(prop) { >+ var number_reg = /[-+]?[0-9]*\.?[0-9]+/; >+ >+ function compare(s1, s2, round_error_ok) { >+ function s(s1, s2) { >+ return Math.abs(Number(s1.match(number_reg)) >+ - Number(s2.match(number_reg))) < 0.0001; >+ } You should probably declare number_reg inside the scope of compare() (the only thing that uses it). (and you could declare it as 'const' instead of 'var', if you like) Also: why is your fuzzy-equality function called "s"? A descriptive name like "is_approx_equal()" would be better. Also: Currently, "s()" will silently succeed if you pass it strings that don't contain any numbers -- because "abcd".match(number_reg) returns null, and if you pass null to Number(), you get 0. So s("abcd", "efgh") will end up comparing 0 to 0 and returning true. It'd be good to sanity-check that we don't accidentally end up performing any bogus comparisons like that (both to sanity-check your current testcases and to make this more future-proof) by adding something like: ok(s1.match(number_reg), "bug in test - expecting partially-numeric value"); ok(s2.match(number_reg), "bug in test - expecting partially-numeric value"); >+ var cond1 = round_error_ok ? s(s1, s2) : s1; >+ var cond2 = round_error_ok ? true : s2; >+ is(cond1, cond2, prop + " property value: computed value before transition: " >+ + s2); "before transition" isn't correct there - you invoke this function twice, and only one is "before transition". Also: the logic structure here is a bit weird -- your is() might be comparing the values themselves, or it might be comparing *the result of their comparison* against "true". This is hard to read, and when it fails, it'll produce error messages in our test logs like like "expected true, got false; filter property value: computed value..." which make it sound like true & false might be filter values (which they aren't). It'd really be better to call "ok()" on result of the fuzzy-equality-test. Something like the following: var msg = prop + " property value: computed value " + s2 + " should match expected."; if (round_error_ok) { ok(fuzzy_equal(actual, expected), msg + " (using fuzzy comparison)"); } else { is(actual, expected, msg); } BUT -- one solution that might even be cleaner (and more robust) would be to allow the filterTests to specify their own fuzzy-equality function (instead of using "round_error_ok" which pidgeon-holes you into using only one fuzzy-equals function that has to work for all of them). If they specify one, use it, and invoke "ok()" on its result; if they don't, then just compare actual value to expected value using "is()" (like in my sample code above). And you always have the option of specifying the same function for multiple types, if it suffices; but this will let you e.g. make a custom fuzzy-equals for multi-function values and for "drop-shadow", if we ever need to.

Comment on attachment 789523 [details] [diff] [review] filter-animation-moz.patch PART 2 OF REVIEW (prev. comment was part 1) (and part 3 is coming). >+ nsCSSUnit u1 = aValue1.GetUnit(); >+ nsCSSUnit u2 = aValue2.GetUnit(); >+ NS_ABORT_IF_FALSE(u1 == eCSSUnit_Number || u1 == eCSSUnit_Percent, >+ "unexpected unit"); >+ NS_ABORT_IF_FALSE(u2 == eCSSUnit_Number || u2 == eCSSUnit_Percent, >+ "unexpected unit"); >+ float n1 = (u1 == eCSSUnit_Number) ? >+ aValue1.GetFloatValue() : aValue1.GetPercentValue(); >+ float n2 = (u2 == eCSSUnit_Number) ? >+ aValue2.GetFloatValue() : aValue2.GetPercentValue(); You perform the same 3 operations on two different values here, and it makes for more duplicated logic (and duplicated copies of "u1" vs "u2" where we could be making a mistake) than I'm comfortable with. Could you clean it up with a helper-function, e.g. "GetNumberOrPercent", which has the logic from the above-quoted code, and then invoke that function twice like so: float n1 = GetNumberOrPercent(aValue1); float n2 = GetNumberOrPercent(aValue2); >+ float result = (n1 - scale) * aCoeff1 + (n2 - scale) * aCoeff2; >+ aResult.SetFloatValue(RestrictValue(aValueRestrictions, result + scale), >+ eCSSUnit_Number); I'm not clear on why we need "initialZero" here. Why can't we just do: result = n1 * aCoeff1 + n2 * aCoeff2; aResult.SetFloatValue(RestrictValue(..., result, eCSSUnit_Number); ? (Note that we don't have any special "initialZero" logic for the 'opacity' property, whose initial value is 1; so I'm not clear on why we need this special logic here...) If we do actually need it, an explanatory comment would be helpful. (But I'm not convinced we need it.) >+static bool >+AddFilterFunction(const nsCSSValueList* aList1, double aCoeff1, >+ const nsCSSValueList* aList2, double aCoeff2, >+ nsCSSValueList**& aListTail) >+{ >+ NS_ABORT_IF_FALSE(aList1 || aList2, "one function list item must not be null"); >+ if (!aList1) >+ return AddFilterFunction(aList2, aCoeff2, aList2, 0, aListTail); >+ if (!aList2) >+ return AddFilterFunction(aList1, aCoeff1, aList1, 0, aListTail); If we take the recursive call, we'll end up doing three redundant null-checks on our list. (once in the outer call, and twice in the recursive call (as list1 and as list2)) It'd be cleaner to make AddFilterFunction() into a short wrapper-function consisting basically of just the above-quoted code (with one more "normal" return case that passes the args straight through), and then split off the remaining part of the function into a helper-function named something like "AddFilterFunctionImpl", which takes the same arguments, and which assumes that *both* of its lists are non-null. (with an assertion at the top to enforce that) >+ nsRefPtr<nsCSSValue::Array> a1 = aList1->GetArrayValue.mValue(), >+ a2 = aList2->mValue.GetArrayValue(); >+ nsCSSKeyword filterFunction = a1->Item(0).GetKeywordValue(); Before we start querying for the function-name, we should check (with an assertion, if we're confident?) that aList1 and aList2 are both actually functions (that they both have GetUnit() == eCSSUnit_Function). (Unless we already verify that elsewhere?) >+ if (filterFunction != a2->Item(0).GetKeywordValue()) >+ return false; A comment would be nice here, e.g. "// Can't add two filters of different types", to the right of "return false". >+ nsRefPtr<nsCSSValue::Array> result = nsCSSValue::Array::Create(2); >+ result->Item(0).SetIntValue(filterFunction, eCSSUnit_Enumerated); >+ >+ nsCSSValueList *item = new nsCSSValueList; >+ item->mValue.SetArrayValue(result, eCSSUnit_Function); "item" seems like an odd name for a nsCSSValueList, particularly when it's declared right after a line where we invoke "Item(0)", which returns something of a completely different type. :) Maybe rename "item" to "resultListEntry"? Also, nsCSSValue has a convenience method "InitFunction()" that will simplify the above-quoted code a bit. I think you can replace that chunk with: nsCSSValueList *resultListEntry = new nsCSSValueList; nsCSSValue::Array* funcArray = resultListEntry->mValue.InitFunction(filterFunction, 1); (This includes the s/item/resultListEntry/ rename)

Comment on attachment 789523 [details] [diff] [review] filter-animation-moz.patch REVIEW COMMENTS, PART 3: >+ *aListTail = item; >+ aListTail = &item->mNext; Don't do this aListTail-setting until the end of the function, when we're sure we'll be returning true. (There's a general Mozilla coding-style guideline that functions with success return-codes and outparams should only modify the outparam when they succeed.) That means you'll actually want to make "item" (or "resultListEntry" as I suggest calling it above) a nsAutoPtr instead of a raw pointer, so that it'll be cleaned up if we return early. (And you'll need to invoke "forget()" on it when copying it into aListTail, too.) >+ // Most filter functions have a NONNEGATIVE restrtiction. >+ uint32_t restrictions = CSS_PROPERTY_VALUE_NONNEGATIVE; Typo in the comment (s/restrtiction/restriction/) Also, this comment is a bit unsettling. :) It sounds like we'll incorrectly reject negative values for some filter functions. I *think* the code it's talking about is fine, fortunately. I referred to our CSS parser for which filter-functions allow negative values... http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSParser.cpp?rev=5fdf867f67d6#10121 ...and it looks like hue-rotate is the only one. And fortunately we don't use this "restrictions" variable in the eCSSKeyword_hue_rotate case in your patch. So it's not an issue (but that's not clear from your comment). So: maybe clarify the comment a bit, e.g. by adding: // ("hue-rotate" is the only filter-function that accepts negative values, and // we don't use this "restrictions" variable in its clause below.) or something like that. >+ bool initialZero = false; [...] >+ case eCSSKeyword_sepia: >+ initialZero = true; As noted above, I'm not sure we need to bother with 'initialZero'. But if we do, I think I'd marginally rather we declare it as "initialVal", like so: float initialVal = 1.0f; and then update it to 0.0f in the one place where you need to, instead of tweaking the boolean value. (And pass that float value directly to AddCSSValuePercentNumber, instead of passing a bool and expecting it to know what to do with it). >+ switch (filterFunction) { >+ case eCSSKeyword_blur: { >+ nsCSSUnit unit; >+ if (a1->Item(1).GetUnit() == a2->Item(1).GetUnit()) { >+ unit = a1->Item(1).GetUnit(); [etc] This next chunk of code refers to a1->Item(1) five times, a2->Item(1) four times, and result->Item(1) three times. All the ...1...->Item(1) vs. ...2...->Item(1) are a bit hard to follow. For readability's sake, maybe declare a local reference to each of these Items(), like so: const nsCSSValue& func1Arg = a1->Item(1); const nsCSSValue& func2Arg = a2->Item(1); nsCSSValue& resultArg = result->Item(1); (maybe you can think of better variable-names) That would make that first check there into: if (func1Arg.GetUnit() == func2Arg.GetUnit()) which is IMHO easier to read than: if (a1->Item(1).GetUnit() == a2->Item(1).GetUnit()) { >+ if (a1->Item(1).GetUnit() == a2->Item(1).GetUnit()) { >+ unit = a1->Item(1).GetUnit(); >+ } else { >+ unit = eCSSUnit_Calc; Add a comment to the "Calc" clause, along the lines of: // If units differ, we'll just combine them with calc(). (Otherwise it's not clear why we're pulling eCSSUnit_Calc out of thin air there. On first reading, I thought maybe the code was assuming that one of the values must *already* be calc, which didn't make sense to me, but I figured it out when I re-read AddCSSValuePixelPercentCalc(). Anyway, a comment would make it clear.) >+ case eCSSKeyword_hue_rotate: >+ AddCSSValueAngle(a1->Item(1), aCoeff1, >+ a2->Item(1), aCoeff2, >+ result->Item(1)); >+ break; >+ default: >+ NS_ABORT_IF_FALSE(false, "unknown filter function"); It'd be worth adding a "return false" here. That way, if an opt build happens to hit this clause (unexpectedly -- maybe after someone breaks this code when adding a new filter-type in the future :)), we won't modify the outparam (given my notes on moving the aListTail-setting stuff to the end), and the caller will treat it as a "can't be added" situation instead of proceeding under the assumption that the addition succeeded. >@@ -2055,16 +2150,60 @@ nsStyleAnimation::AddWeighted(nsCSSProperty aProperty, >+ nsCSSUnit u1 = eCSSUnit_Null, u2 = eCSSUnit_Null; >+ if (list1) { >+ u1 = list1->mValue.GetUnit(); >+ } >+ if (list2) { >+ u2 = list2->mValue.GetUnit(); >+ } >+ if ((u1 != eCSSUnit_Function && u1 != eCSSUnit_Null) >+ || (u2 != eCSSUnit_Function && u2 != eCSSUnit_Null)) { >+ // Just interpolate filter functions, no URLs. >+ return false; >+ } Style-nit: don't put "||" at the beginning of a line. It should be at the end of the prev. line. ALSO: this whole chunk could be simplified to remove "u1" and "u2" entirely, I think -- this should suffice: if ((list1 && list1->GetUnit() != eCSSUnit_Function) || (list2 && list2->GetUnit() != eCSSUnit_Function)) { // If we don't have filter-functions, we must have filter-URLs, which // we can't add or interpolate. return false; } (aside: ah, so this is why we're sure we have eCSSUnit_Function, when we get into AddFilterFunction. Anyway, still worth asserting about the unit inside of AddFilterFunction (or in its helper), as I suggested in my previous comment.) >@@ -3007,16 +3162,68 @@ nsStyleAnimation::ExtractComputedValue(nsCSSProperty aProperty, >+ const nsTArray<nsStyleFilter>& filters = svgReset->mFilters; [...] >+ const nsStyleFilter& filter = filters.ElementAt(i); Just use filters[i]. (Same as ElementAt, but more consise) >+ nsCSSKeyword functionName = nsCSSProps::ValueToKeywordEnum( >+ type, >+ nsCSSProps::kFilterFunctionKTable); This would be cleaner as: nsCSSKeyword functionName = nsCSSProps::ValueToKeywordEnum(type, nsCSSProps::kFilterFunctionKTable); Still gross, but less-gross, IMHO. I can't read functions whose arguments are indented way to the left of them. :) >+ nsCSSValue filterValue; >+ nsRefPtr<nsCSSValue::Array> filterArray = >+ filterValue.InitFunction(functionName, 1); You shouldn't need to capture filterArray in a nsRefPtr -- a raw pointer should be fine. It's owned by filterValue, which is in the same scope, so you don't need an additional reference here. Also: it looks like you actually the array to item->mValue right away... So you don't end up actually using filterValue itself at all. You probably should just call InitFunction directly on "item->mValue", so then it'll already be there.

Comment on attachment 789523 [details] [diff] [review] filter-animation-moz.patch REVIEW COMMENTS PART 4 (last part): >+ if (type >= NS_STYLE_FILTER_BLUR && type <= NS_STYLE_FILTER_HUE_ROTATE) { >+ StyleCoordToCSSValue( >+ filter.GetFilterParameter(), >+ filterArray->Item(1)); You probably should be checking the return value of StyleCoordToCSSValue, and returning false (or asserting?) if it fails. >+ } else if (type == NS_STYLE_FILTER_DROP_SHADOW) { >+ return false; >+ } else { >+ NS_NOTREACHED("no other filter functions defined"); >+ return false; It looks like you don't check (explicitly at least) for NS_STYLE_FILTER_NONE here. Do you need to? (If not, be worth adding a comment mentioning it by name & briefly explaining where it'll end up or why we don't need to check for it -- since you're explicitly checking for all the other NS_STYLE_FILTER_* values defined in nsStyleConsts.h. _NONE seems to be the lone exception.) >+ if (!filters.Length()) { >+ aComputedValue.SetAndAdoptCSSValueListValue(nullptr, eUnit_Filter); >+ return true; >+ } >+ aComputedValue.SetAndAdoptCSSValueListValue(result.forget(), >+ eUnit_Filter); >+ break; This should be "filters.IsEmpty()", not !Length(). BUT: I'm pretty sure you can just drop that clause entirely, because your other (main) SetAndAdoptCSSValueListValue() invocation will already do the right thing, since |result| will still be uninitialized and so result.forget() will return nullptr. So, just drop that whole !Length() clause. >@@ -3294,22 +3501,24 @@ nsStyleAnimation::Value::operator=(const Value& aOther) >- NS_ABORT_IF_FALSE(mUnit == eUnit_Shadow || aOther.mValue.mCSSValueList, >- "value lists other than shadows may not be null"); >+ NS_ABORT_IF_FALSE(mUnit == eUnit_Shadow || mUnit == eUnit_Filter >+ || aOther.mValue.mCSSValueList, Shift the "||" to the end of the previous line. (Aside: I wonder if SetAndAdoptCSSValueListValue() should be asserting the same thing? Right now it only asserts that value-lists can't be null for eUnit_Dasharray) >@@ -376,17 +377,18 @@ public: > static bool IsCSSValueListUnit(Unit aUnit) { > return aUnit == eUnit_Dasharray || aUnit == eUnit_Shadow || >- aUnit == eUnit_Transform || aUnit == eUnit_BackgroundPosition; >+ aUnit == eUnit_Transform || aUnit == eUnit_BackgroundPosition || >+ aUnit == eUnit_Filter; nit: I think this list was in order (matching the "enum Unit { ... }" list) before your patch. Please insert Filter after Dasharray (before Shadow) here, adding newlines as-necessary, so that the order still matches. Also: two other out-of-order nits (jumping back towards the beginning of the patch). FIRST NIT: >+static bool >+AddFilterFunction(const nsCSSValueList* aList1, double aCoeff1, >+ const nsCSSValueList* aList2, double aCoeff2, >+ nsCSSValueList**& aListTail) Reorder the args here so that "aCoeff1" is *before* aList1, and the same for aCoeff2/aList2, for consistency with the prevailing ordering of similar functions in this file. (We're mostly, but not entirely, consistent on this; I filed bug 905449 on making us more consistent.) SECOND NIT: >@@ -748,16 +748,17 @@ nsStyleAnimation::ComputeDistance(nsCSSProperty aProperty, >+ case eUnit_Filter: > case eUnit_Transform: { > return false; > } I think we actually can & should do reasonable distance-computation for these filter values, in most (if not all) of the situations where we can interpolate. This makes a difference for paced-mode SMIL animation (and maybe for something in transitions as well? not sure), with something like: <animate calcMode="paced" values="brightness(0.5); brightness(0.6); brightness(0); brightness(0.4)"> There, we can easily add up the traversed differences and make a constant-speed (paced) animation, *if* we have the piecewise distance-computation function implemented. That distance-computation part probably doesn't have to block the rest of this patch, though -- but maybe file a followup on that? (depending on this bug) I'll hold off on granting r+, since that's a lot of (mostly small) things to change. But this probably has r=me with all of that addressed. :)

[just a heads-up: bug 905449 just landed on inbound, and it may bitrot this slightly. In particular, it reordered the parameters in AddCSSValueAngle() (to put the coefficients before the values), so you'll need to fix your patch's invocation of AddCSSValueAngle() accordingly. The contextual-code changes may cause your patch not to apply, too, but I imagine you can fix those with fuzz (e.g. "patch -F10"). Ping me on IRC if you need help with that.]

(In reply to Daniel Holbert [:dholbert] from comment #26) > [just a heads-up: bug 905449 just landed on inbound, and it may bitrot this > slightly. In particular, it reordered the parameters in AddCSSValueAngle() > (to put the coefficients before the values), so you'll need to fix your > patch's invocation of AddCSSValueAngle() accordingly. The contextual-code > changes may cause your patch not to apply, too, but I imagine you can fix > those with fuzz (e.g. "patch -F10"). Ping me on IRC if you need help with > that.] I'll look into it next week.

(In reply to Daniel Holbert [:dholbert] from comment #23) > >+ float result = (n1 - scale) * aCoeff1 + (n2 - scale) * aCoeff2; > >+ aResult.SetFloatValue(RestrictValue(aValueRestrictions, result + scale), > >+ eCSSUnit_Number); > > I'm not clear on why we need "initialZero" here. Why can't we just do: > result = n1 * aCoeff1 + n2 * aCoeff2; > aResult.SetFloatValue(RestrictValue(..., result, eCSSUnit_Number); > ? > > (Note that we don't have any special "initialZero" logic for the 'opacity' > property, whose initial value is 1; so I'm not clear on why we need this > special logic here...) > > If we do actually need it, an explanatory comment would be helpful. (But > I'm not convinced we need it.) Opacity has a default value of 1, which is known (and set). So we can interpolate from what ever value to the known number. This is different for filter functions. If we pass just one filter function (because the other function list is shorter), then we pass the same function a second time. At this point we don't know the function and can not modify the passed coefficients. So what the animation does is animating around a neutral number, which is 0. For many functions we need to change this to 1. And this code is just changing the neutral number to 1. Btw. it is done for the scale transform function as well. I'll add a comment, but don't think that we can avoid it.

(In reply to Dirk Schulze from comment #28) > If we pass just one filter function (because the other > function list is shorter), then we pass the same function a second time. Ah, right -- and we pass it **with a coefficient of 0**, which was the key part I was missing. If the coefficients to add up to 1 (which they normally do when we're interpolating), then the initialVal in AddFilterFunction literally does not matter, because it just ends up cancelling out in the math. That's why I was confused about why you needed it. **But**, because of this trick you do with invoking the function with one of the coefficients being 0, then the coefficients *don't* end up adding up to 1, so the initial value *does* end up affecting the math. So: I agree, you do need initialVal after all.

Comment on attachment 792948 [details] [diff] [review] filter-animation-moz.patch >+static bool >+AppendCSSShadowValue(const nsCSSShadowItem *aShadow, >+ nsCSSValueList **&aResultTail) >+{ [...] >+ return true; >+} This function only returns true. Hence, no need for it to have a return value. Just change its return type to "void". >+static inline float GetNumberOrPercent(const nsCSSValue &aValue) >+{ Insert a newline after "float". >+ nsCSSUnit unit = aValue.GetUnit(); >+ NS_ABORT_IF_FALSE(unit == eCSSUnit_Number || unit == eCSSUnit_Percent, >+ "unexpected unit"); >+ return (unit == eCSSUnit_Number) ? >+ aValue.GetFloatValue() : aValue.GetPercentValue(); Odd excessive indentation there. I believe local style would have that final line just indented 2 spaces more than the previous line. (so, the initial "a" in "aValue" should be aligned with the "t" in "return") >+static inline void >+AddCSSValuePercentNumber(const uint32_t aValueRestrictions, >+ double aCoeff1, const nsCSSValue &aValue1, >+ double aCoeff2, const nsCSSValue &aValue2, >+ nsCSSValue &aResult, float initialVal) "initialVal" is an argument, so it should be named "aInitialVal" >+ // The interpolation is done depending on the neutral element for >+ // interpolation which is 0. With the following code the neutral element >+ // can be changed to initialVal (usually 1). See AddTransformScale(). "which is 0" is misleading there, and "See AddTransformScale()." isn't really helpful since the comment in AddTransformScale is kinda transform-component-specific. Maybe replace with something like: // Rather than interpolating aValue1 and aValue2 directly, we // interpolate their *distances from aInitialVal* (the initial value, // which is either 1 or 0 for "filter" functions). This matters in // cases where aInitialVal is nonzero and the coefficients don't add // up to 1. For example, if initialVal is 1, aCoeff1 is 0.5, and // aCoeff2 is 0, then we'll return the value halfway between 1 and // aValue1, rather than the value halfway between 0 and aValue1. // Note that we do something similar in AddTransformScale(). >+static bool >+AddFilterFunctionImpl(double aCoeff1, const nsCSSValueList* aList1, >+ double aCoeff2, const nsCSSValueList* aList2, >+ nsCSSValueList**& aListTail) >+{ s/aListTail/aResultTail/ (to emphasize that this is an "result" (an outparam), and also to match the naming of this param in AddShadowItems().) >+ NS_ABORT_IF_FALSE(aList1->mValue.GetUnit() == eCSSUnit_Function, >+ "expected function"); >+ NS_ABORT_IF_FALSE(aList2->mValue.GetUnit() == eCSSUnit_Function, >+ "expected function"); Before those assertions, we should assert (with ABORT_IF_FALSE) that aList1 and aList2 are non-null, with a message like "AddFilterFunction should be our only caller, and it should ensure that both args are non-null". With the current patch, if either of those args end up being, then we'll crash *while evaluating the condition* in one of your other NS_ABORT_IF_FALSE() statements there, that's a confusing sort of crash. >+ nsAutoPtr<nsCSSValueList> resultListEntry; >+ resultListEntry = new nsCSSValueList; No need for the separation there -- combine those two lines: nsAutoPtr<nsCSSValueList> resultListEntry = new nsCSSValueList; >+ // "hue-rotate" is the only filter-function that accepts negative values, and >+ // we don't use this "restrictions" variable in its clause below. >+ uint32_t restrictions = CSS_PROPERTY_VALUE_NONNEGATIVE; (Maybe make this "const uint32_t", to emphasize that we're not changing it?) >+ const nsCSSValue& funcArg1 = a1->Item(1); >+ const nsCSSValue& funcArg2 = a2->Item(1); >+ nsCSSValue& resultArg = result->Item(1); [...] >+ case eCSSKeyword_drop_shadow: { >+ nsCSSValueList* shadow = result->Item(1).SetListValue(); >+ nsAutoPtr<nsCSSValueList> shadowValue; >+ nsCSSValueList **shadowTail = getter_Transfers(shadowValue); >+ if (!AddShadowItems(aCoeff1, a1->Item(1).GetListValue()->mValue, >+ aCoeff2, a2->Item(1).GetListValue()->mValue, >+ shadowTail)) { Use the helper-variables funcArg1, funcArg2, and resultArg in this case, like you do in the other cases. (There shouldn't be any need for any "Item(1)" here.) >+ *shadow = *shadowValue; Replace "shadow" with "shadowResult", or something like that, to make it clearer what this assignment is doing. (we're putting the result of all our work into a "result" variable.) Also: as I suggested over in bug 904603 comment 6: we need to assert that our two input shadow-lists have length 1 here. (and we're not dropping anything on the floor) I think that'd be something like: NS_ABORT_IF_FALSE(!funcArg1->GetListValue()->mNext && !funcArg2->GetListValue()->mNext, "drop-shadow filter func doesn't support lists"); >+ nsCSSValueList *test = resultListEntry.forget(); >+ *aListTail = test; >+ aListTail = &test->mNext; As noted in IRC: "test" isn't a great name here. :) resultListEntryPtr maybe, or you can do it without the helper-variable like so: *aListTail = resultListEntry.forget(); aListTail = &(*aListTail)->mNext; >+static bool >+AddFilterFunction(double aCoeff1, const nsCSSValueList* aList1, >+ double aCoeff2, const nsCSSValueList* aList2, >+ nsCSSValueList**& aListTail) >+{ As above, s/aListTail/aResultTail/ >+ NS_ABORT_IF_FALSE(aList1 || aList2, "one function list item must not be null"); >+ if (!aList1) >+ return AddFilterFunctionImpl(aCoeff2, aList2, 0, aList2, aListTail); >+ if (!aList2) >+ return AddFilterFunctionImpl(aCoeff1, aList1, 0, aList1, aListTail); >+ return AddFilterFunctionImpl(aCoeff1, aList1, aCoeff2, aList2, aListTail); The first line is too long -- add a newline after the comma. Also: an explanatory comment would be helpful here, before the "if" checks. Maybe: Note that one of our arguments could be null, indicating that it's the initial value. Rather than adding special null-handling logic, we just check for null values and replace them with 0 * the other value. That way, AddFilterFunctionImpl can assume its args are non-null. or something like that. Also: add a blank line after the first two "return" statements, to make the clauses more clearly separate. >+ } else if (type == NS_STYLE_FILTER_DROP_SHADOW) { >+ nsCSSValueList *shadowValue = filterArray->Item(1).SetListValue(); >+ nsAutoPtr<nsCSSValueList> shadowResult; >+ nsCSSValueList **shadowResultTail = getter_Transfers(shadowResult); >+ nsCSSShadowArray *shadowArray = filter.GetDropShadow(); Nit: Shift the stars to the left (to hug the type, rather than the variable-name). So, s/nsCSSValueList *shadowValue/nsCSSValueList* shadowValue/, etc >+ NS_ABORT_IF_FALSE(shadowArray->Length() == 1, >+ "expected exactly one shadow"); >+ if (!AppendCSSShadowValue( >+ shadowArray->ShadowAt(0), shadowResultTail)) { >+ return false; The newline/un-indenting in the "AppendCSSShadowValue()" call is a bit awkward / unnecessary. Just put a newline after the comma, and indent normally, like so: if (!AppendCSSShadowValue(shadowArray->ShadowAt(0), shadowResultTail)) { ALSO: As noted at the very top of this bugzilla-comment, AppendCSSShadowValue() never fails, so it shouldn't return bool, and you don't need to bother checking its return-value here. >+ } >+ *shadowValue = *shadowResult; The variable-naming feels a bit backwards here. (In particular, it's backwards w.r.t. your variable-naming in AddFilterFunctionImpl() -- there, shadowValue was the temporary local variable, whereas here, shadowValue is the outparam). Intuitively, I expect "shadowResult" to hold the result of our work in this clause, and "shadowValue" to be a temporary value that's about to go out of scope. Maybe just swap the names?(and/or add "tmp" to the naming of the temporary variable?) You'll need to make a corresponding change to the naming of "shadowResultTail" (declared right below "shadowResult"), too. >+ } else { >+ NS_NOTREACHED("no other filter functions defined"); >+ return false; As noted in comment 25 -- you're not checking NS_STYLE_FILTER_NONE here. Can you add a comment explaining why we don't need to, since that's the one NS_STYLE_FILTER_* value missing from this "if/else" list? Also: please file a followup to add a ComputeDistance implementation, per comment 25. (And it'd probably be good to mention that bug number in a code-comment to the right of "case eUnit_Filter:" in this patch's ComputeDistance chunk.) (I just noticed dbaron mentioned ComputeDistance in Comment 4, too -- he said we could leave it un-implemented "as long as these aren't exposed to SMIL animation as something that would expect to support paced animation" -- and I think they *are* exposed to SMIL animation and authors could easily try to animate them with paced animation, as in my sample code in Comment 25. So I think we do need to implement ComputeDistance, and that needs a followup bug.)

(In reply to Daniel Holbert [:dholbert] from comment #31) > (I just noticed dbaron mentioned ComputeDistance in Comment 4, too -- he > said we could leave it un-implemented "as long as these aren't exposed to > SMIL animation as something that would expect to support paced animation" -- > and I think they *are* exposed to SMIL animation and authors could easily > try to animate them with paced animation, as in my sample code in Comment > 25. So I think we do need to implement ComputeDistance, and that needs a > followup bug.) If you would do that, you are violating the current definition of SMIL animations which just supports animation of previously defined types as SVGAnimatedRect, SVGAnimatedString, SVGAnimatedLength, SVGAnimatedLengthList, SVGAnimatedNumber, SVGAnimatedNumberList, SVGAnimateNumberOptionalNumber, SVGAnimatedEnumeration, SVGAnimatedBoolean, SVGAnimatedColor, SVGAnimatedColor, SVGAnimatedInteger, SVGAnimatedIntegerOptionalInteger, SVGAnimatedPath, SVGAnimatedPointList, SVGAnimatePreserveAspectRatio and SVGAnimatedTransformList but no SVGAniamtedFilterList. So you will be having a hard time to expose it to SVG DOM. I can nearly guarantee that it won't be supported by WebKit, Blink (where I wrote big chunks of the current animation code) and (unless MS has second thoughts on SMIL) IE. That said, I'll add a comment and try to implement it in a separate patch. I need to check what is necessary to do it but it not be to much work.

(In reply to Dirk Schulze from comment #33) > If you would do that, you are violating the current definition of SMIL > animations which just supports animation of previously defined types as > SVGAnimatedRect, SVGAnimatedString, SVGAnimatedLength, > SVGAnimatedLengthList, SVGAnimatedNumber, SVGAnimatedNumberList, > SVGAnimateNumberOptionalNumber, SVGAnimatedEnumeration, SVGAnimatedBoolean, > SVGAnimatedColor, SVGAnimatedColor, SVGAnimatedInteger, > SVGAnimatedIntegerOptionalInteger, SVGAnimatedPath, SVGAnimatedPointList, > SVGAnimatePreserveAspectRatio and SVGAnimatedTransformList > but no SVGAniamtedFilterList. So you will be having a hard time to expose it > to SVG DOM. Why does it need to be exposed using an SVG DOM interface? We typically haven't done that for properties. ('transform' being different since it used to be a plain attribute.)

(In reply to Dirk Schulze from comment #33) > If you would do that, you are violating the current definition of SMIL > animations which just supports animation of previously defined types as > SVGAnimatedRect, SVGAnimatedString, SVGAnimatedLength, [...] > but no SVGAniamtedFilterList. First of all: That's an argument against supporting *any* SMIL animation (w/ interpolation) of the "filter" property, not an argument against ComputeDistance specifically. And I'm 95% sure that SMIL animation of the filter property *is going to work*, with the exception of calcMode="paced", after your patch lands. :) Do you not think that should happen? > So you will be having a hard time to expose it > to SVG DOM. Why does that matter? "filter" is a property / presentational attribute, and IIRC the other presentational attributes don't get any special SVG DOM treatment, as heycam indicated. For comparison: "stroke-dasharray" is a property / presentational attribute with a fairly complex list-based syntax, and both Gecko and Webkit will gladly SMIL-animate *that* property, despite the fact that it's not exposed to the DOM (I think) and it's not any of the SVGAnimatedXYZ types that you listed above.

Comment on attachment 796197 [details] [diff] [review] filter-animation-moz.patch Code looks great! Just a few comments on the mochitest: >+function test_filter_function_list(string, expectedList) >+{ >+ // The regular expression does not filter out the last parenthesis. >+ // Remove last character for now. >+ string = string.substring(0, string.length - 1); Assert (using "is") that the last character is a close-paren, before you get rid of it. Otherwise, someone down the line is going to add more testcase entries to your list here, and they'll forget to include that paren, and they'll bang their heads against the wall wondering why their entries are having the last character ignored. :) >+ var reg = /\)*\s*(blur|brightness|contrast|grayscale|hue\-rotate|invert|opacity|saturate|sepia|drop\-shadow|url)\(/ >+ var matches = string.split(reg); >+ // First item must be empty. All other items are of functionName, functionValue. >+ if (!matches || matches.shift() != "") { >+ return false; >+ } Add a hint that something went wrong here, rather than just returning false. It's useful to distinguish this sort of "the test tripped over itself" failure-case from the normal "successfully sliced up the string and determined that it doesn't match" failure-case. e.g.: ok(false, "computed style of 'filter' isn't in the format we expect") or something. >+ // Odd items are the function name, even items the function value. >+ if (!matches.length || matches.length % 2) { >+ return false; >+ } Same here. ALSO: you don't currently sanity-check the length of expectedList -- your loop just starts indexing into expectedList, assuming it's the right length. Please add a check that expectedList.length == matches.length (and an ok(false) & early-return) before you start the loop, so if someone adds a testcase with the wrong length expected list, they get more useful feedback than a JS index-out-of-bounds exception. >+ } else if (functionName == "hue-rotate") { >+ // Last two characters must be "deg". >+ if (functionValue.search("rad") != functionValue.length - 3) { >+ return false; Your code doesn't match your comment there -- I think the comment wants to say "rad"? :) >+ } else if (functionName == "url") { >+ if (functionValue.search(expected) < 0) { >+ return false; >+ } Hmm... so you're checking that the computed style's URL *contains* the expected-value-string here, anywhere. That doesn't seem very robust, for URL-matching. (An expected value of "" or "h" or many other small strings would then trivially match.) And it could easily be made more robust. Could you broaden this slightly to check just 2 things: a) expected's first character is "#" b) functionValue *ends with* expected ...and if both of those are true, count it as a match? (Gecko supports String.endsWith, but it looks like webkit doesn't; not sure how much it matters that this test-code be engine-agnostic, but if you want to avoid non-webkit-supported stuff, you could just paste in the line of code from the top "indexOf" solution here: http://stackoverflow.com/questions/280634/endswith-in-javascript ) >+function test_filter_transition(prop) { [...] >+ is(test_filter_function_list(actual, test.expected), true, >+ "Filter property is " + actual); Two things on this: * Don't compare boolean values to "true" with is(). That unnecessarily exercises the "expected: ... actual: ..." logging and checking code. Just use "ok()" for boolean values. So, do: ok(test_filter_function ... ) * Rename "test_filter_function_list()" to something like "filter_function_list_equals". (This file's "test_XYZ" functions generally *do their own is() checks*, rather than returning true/false and having that checked. So this isn't in the style of a "test_XYZ" function and should be named differently for clarity.) r=me with that.

Addressed the comments above. For url checking I build the expected link with document.URL + fragment identifier. Now actual value and expected value must match exactly... no hacks around contains the fragment identifier anymore. I'll upload the patch to the try bots once bug 904158 is closed.

Comment on attachment 796597 [details] [diff] [review] filter-animation-moz.patch >+function filter_function_list_equals(string, expectedList) >+{ [...] >+ is(string.substring(string.length - 1, string.length), ')', "Check if last character is close-paren.") Nit: You're missing a semicolon at the end of that line. And while you're at it, add a newline before the message, so that this line isn't so huge. Also -- sorry for not catching this sooner -- "string" is a bit too generic of an param-name. Something clearer & more self-documenting like "computedValStr" would be better, IMHO. Not the end of the world if you don't get to fixing that, though.

> I'll upload the patch to the try bots once bug 904158 is closed. It might be a day or two until that happens (the tree's been closed during my two opportunities to land it today, so hopefully it'll get landed by a checkin-needed sheriff in the next day or two). I'd suggest just importing the latest patch there and using that for your Try push, to get testing results back sooner.

I pushed the patch to the try bots: https://tbpl.mozilla.org/?tree=Try&rev=c26cc4d652bb I can reproduce the 4 assertions on the test test_smilMappedAttrFromTo.xhtml but they are related to the cursor property, not the filter property. I did not touch the code for cursor.

There is no way I'm pushing this patch with those M1 failures. You'll need to discuss them with Dan and decide what needs to be done about them.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #42) > There is no way I'm pushing this patch with those M1 failures. You'll need > to discuss them with Dan and decide what needs to be done about them. I understand. Is it certain that it is caused by this patch? Does M1 fail on other tests lately as well? Just looking at the changelog on the bots, I see a bunch of these errors across all tests in M1: 13:45:16 INFO - System JS : ERROR (null):0 13:45:16 INFO - uncaught exception: 2147746065 But mainly behind properties that take URLs. Not very descriptive still.

(In reply to Dirk Schulze from comment #43) > I understand. Is it certain that it is caused by this patch? Yes; it looks like your Try run hit the exact same issue on all debug M1 jobs, which makes it virtually certain that it's caused by something in your patch. > Does M1 fail on > other tests lately as well? There are sporadic failures in basically all of our test suites, but they're sporadic (and each one itself is low-frequency), so you won't see those failures cropping up across-the-board in a Try run. If your try run hits an across-the-board type failure (for some definition of across-the-board -- "debug M1" in this case), please do *not* use checkin-needed until it's sorted out. (Ryan, thanks for catching that before landing!) > Just looking at the changelog on the bots, I see a bunch of these errors > across all tests in M1: > 13:45:16 INFO - System JS : ERROR (null):0 > 13:45:16 INFO - uncaught exception: 2147746065 I don't know what that is, out of context, but it's not what's causing the problem here. The problem is assertion-failures -- so you need to search for "ASSERTION" to find them. The failures are like this: { 13:45:56 INFO - 169234 INFO TEST-PASS | /tests/content/smil/test/test_smilMappedAttrFromTo.xhtml | filter: checking that 'from' value is set at start of animation 13:45:56 INFO - [Parent 2414] ###!!! ASSERTION: data should be null terminated: 'data[len] == PRUnichar(0)', file ../../../../xpcom/string/src/nsSubstring.cpp, line 252 13:45:56 INFO - nsStringBuffer::ToString(unsigned int, nsAString_internal&, bool) [xpcom/string/src/nsSubstring.cpp:252] 13:45:56 INFO - nsSMILMappedAttribute::SetAnimValue(nsSMILValue const&) [content/smil/nsSMILMappedAttribute.cpp:97] 13:45:56 INFO - nsSMILCompositor::ComposeAttribute() [content/smil/nsSMILCompositor.cpp:101] 13:45:56 INFO - DoComposeAttribute(nsSMILCompositor*, void*) [content/smil/nsSMILAnimationController.cpp:348] } ...which looks like another version of bug 904158. I can dig in a bit and figure out what's going on early next week. Until that's sorted out, we should hold off on landing this.

Now that bug 911451 has fixed the issue from comment 44, this should be good to land! I gave it a linux/android try sanity-run yesterday... https://tbpl.mozilla.org/?tree=Try&rev=a6ef69dc72a1 ...which came back green, modulo some known randomorange. So, I landed the main patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/4eee1c589bb0 ...along with a followup with the minor mochitest tweaks from comment 39 (with a few additional newlines thrown in for readability, as well): https://hg.mozilla.org/integration/mozilla-inbound/rev/c10263f35b2c

(In reply to Daniel Holbert [:dholbert] from comment #45) > Now that bug 911451 has fixed the issue from comment 44, this should be good > to land! > > I gave it a linux/android try sanity-run yesterday... > https://tbpl.mozilla.org/?tree=Try&rev=a6ef69dc72a1 > ...which came back green, modulo some known randomorange. > > So, I landed the main patch: > https://hg.mozilla.org/integration/mozilla-inbound/rev/4eee1c589bb0 > ...along with a followup with the minor mochitest tweaks from comment 39 > (with a few additional newlines thrown in for readability, as well): > https://hg.mozilla.org/integration/mozilla-inbound/rev/c10263f35b2c Thanks a lot for taking care of it!

https://hg.mozilla.org/mozilla-central/rev/4eee1c589bb0 https://hg.mozilla.org/mozilla-central/rev/c10263f35b2c

Is this web developer interesting enough to release note? If so, could someone write up the one sentence note for it and post that here? If not, say so and we'll not include it.

(In reply to Asa Dotzler [:asa] from comment #48) > Is this web developer interesting enough to release note? If so, could > someone write up the one sentence note for it and post that here? If not, > say so and we'll not include it. No, it is just the animation of the property values without visual results. It would just be interesting for developers once you actually see the filter applying.

Comment on attachment 796597 [details] [diff] [review] filter-animation-moz.patch >@@ -2055,16 +2232,54 @@ nsStyleAnimation::AddWeighted(nsCSSProperty aProperty, >+ case eUnit_Filter: { >+ const nsCSSValueList *list1 = aValue1.GetCSSValueListValue(); >+ const nsCSSValueList *list2 = aValue2.GetCSSValueListValue(); >+ >+ nsAutoPtr<nsCSSValueList> result; >+ nsCSSValueList **resultTail = getter_Transfers(result); >+ while (list1 || list2) { [...] >+ if (list2) { >+ list2 = list2->mNext; >+ } >+ }; ^------------------------------------. | BTW: I just pushed a trivial patch to remove this semicolon at the end of the "while" loop (which has no functional effect). I noticed it when skimming nsStyleAnimation while investigating another bug. https://hg.mozilla.org/integration/mozilla-inbound/rev/4939a51edb2d