Closed Bug 1377541 Opened 7 years ago Closed 7 years ago

The serialized animation value of shadow is different from computed values.

Categories

(Core :: DOM: Animation, enhancement, P2)

Product:
Core
Component:
DOM: Animation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

test-case.html
1.28 KB, text/html
Details
Bug 1377541 - Change the order of serialized value for mochitest.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1377541 - Reorder shadow array when serializing.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Attached file test-case.html — 
Currently, the serialized animation value is different from computed values. This animation value mean the value which got via getKeyframes() function.

If we run attached test case, result is as follow:

--------------------------------------------------
Animation Value:10px 10px 10px 0px rgb(0, 255, 0)
Computed Value:rgb(0, 255, 0) 10px 10px 10px 0px
--------------------------------------------------

Animation values is `(x offset) (y offset) (blur) (spread) (color)`, however computed value is `(color) (x offset) (y offset) (blur) (spread)`.

I think that we need to follow computed values order.
I discussed with Brian, we need to modify the gecko implementation by creating function which reorder array.

The gecko will call the AppendCSSShadowValue() when storing the shadow data as nsCSSValue::Array[1].

[1] http://searchfox.org/mozilla-central/rev/e8f4f51cd543f203e9cb861cecb7545ac43c836c/layout/style/StyleAnimationValue.cpp#328

This array order starts not length value but color, and we use this value as serialized data, so serialized order is different from computed value. However this order use from interpolation / accumulation and parsing, so I think that we had better to leave this order.
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
I think I made a mistake when we last discussed this bug. The result of getKeyframes() and getComputedStyle() does *not* need to match. getKeyframes() will return *specified* values (except for CSS animations and transitions where the values will happen to be computed values).

However, we still should ensure that the serialization of specified values and getKeyframes() values is consistent and that things like the order of components is consistent between specified values, getKeyframes(), and getComputedStyle().
Thanks, Brian.

(In reply to Brian Birtles (:birtles) from comment #2)
> I think I made a mistake when we last discussed this bug. The result of
> getKeyframes() and getComputedStyle() does *not* need to match.
> getKeyframes() will return *specified* values (except for CSS animations and
> transitions where the values will happen to be computed values).
> 
> However, we still should ensure that the serialization of specified values
> and getKeyframes() values is consistent and that things like the order of
> components is consistent between specified values, getKeyframes(), and
> getComputedStyle().

Let me clarify my understanding as follows:

Do you mean that we need to match the order of component? and we don't need to match the each component value?
(i.e. If the specified value is '10px 10px 10px lime', the result of getKeyframes() is '10px 10px 10px lime' like gecko.)

I tried to reorder the array like following code:
https://hg.mozilla.org/try/rev/b2dfa59054d57c8d1294d879da5d562fa86aa299#l2.12
(it is Rough WIP code, so it contains the unnecessary code..)
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #3)
> Thanks, Brian.
> 
> (In reply to Brian Birtles (:birtles) from comment #2)
> > I think I made a mistake when we last discussed this bug. The result of
> > getKeyframes() and getComputedStyle() does *not* need to match.
> > getKeyframes() will return *specified* values (except for CSS animations and
> > transitions where the values will happen to be computed values).
> > 
> > However, we still should ensure that the serialization of specified values
> > and getKeyframes() values is consistent and that things like the order of
> > components is consistent between specified values, getKeyframes(), and
> > getComputedStyle().
> 
> Let me clarify my understanding as follows:
> 
> Do you mean that we need to match the order of component? and we don't need
> to match the each component value?
> (i.e. If the specified value is '10px 10px 10px lime', the result of
> getKeyframes() is '10px 10px 10px lime' like gecko.)

Right, those should match. (Whereas getComputedStyle() will presumably give us rgb(0, 255, 0) for 'lime')
This is blocking enabling dom/animation/test/css-animations/test_keyframeeffect-getkeyframes.html so we need to either fix this soon or update the test and mark it failing for Gecko.
Blocks: stylo-css-animations
Priority: P3 → P2
Comment on attachment 8891878 [details]
Bug 1377541 - Change the order of serialized value for mochitest.

https://reviewboard.mozilla.org/r/162902/#review168282
Attachment #8891878 - Flags: review?(bbirtles) → review+
Comment on attachment 8891879 [details]
Bug 1377541 - Reorder shadow array when serializing.

https://reviewboard.mozilla.org/r/162904/#review168280

::: layout/style/nsCSSValue.cpp:1388
(Diff revision 1)
> +    // the order of color value is 5th. This order of computed value is first,
> +    // so we reorder this array when serializing the value.
> +    if (aProperty == eCSSProperty_text_shadow ||
> +        aProperty == eCSSProperty_box_shadow ||
> +        aProperty == eCSSProperty_filter) {
> +      nsCSSValue::Array* reordered = nsCSSValue::Array::Create(6);

This leaks. See the definition of Create:

  http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/layout/style/nsCSSValue.h#1041

This will allocate memory that we assign to a raw pointer and never deallocate.

Furthermore, it's a lot nicer if we allocate the memory inside GetSerializedShadowArray rather than requiring the caller to allocate the correct-sized array and then pass it in.

So how about we:

* Make GetSerializedShadowArray return a  already_AddRefed<nsCSSValue::Array> and drop the aReorderArray parameter
  (You will need to create a local RefPtr and then call .forget() on it as part of the return statement)
* Drop the aProperty parameter since it is only used for a debug-assert and we check the list of properties at its one call site
* Rename the remaining parameter to just aOriginalArray
* Rename it to GetReorderedShadowArrayForSerializing (since it doesn't actually serialize anything)
* Update the comment for that function to something like:

  /**
   * Returns a re-ordered version of an nsCSSValue::Array representing a shadow
   * item (including a drop-shadow() filter function) suitable for serialization.
   */

* At the call site you will want to make the |reordered| local variable a RefPtr and move it outside the if block so that it keeps the pointer alive for as long as |array| is used.
* Update the comment for this call site to something like:

  // CSSParserImpl::ParseShadowItem stores shadow items in a specific order
  // that does not match the order we use when serializing computed shadow
  // items. In order to match the computed value order, we shuffle the items
  // in the shadow array before serializing it.
Attachment #8891879 - Flags: review?(bbirtles)
Comment on attachment 8891879 [details]
Bug 1377541 - Reorder shadow array when serializing.

https://reviewboard.mozilla.org/r/162904/#review168280

> This leaks. See the definition of Create:
> 
>   http://searchfox.org/mozilla-central/rev/09c065976fd4f18d4ad764d7cb4bbc684bf56714/layout/style/nsCSSValue.h#1041
> 
> This will allocate memory that we assign to a raw pointer and never deallocate.
> 
> Furthermore, it's a lot nicer if we allocate the memory inside GetSerializedShadowArray rather than requiring the caller to allocate the correct-sized array and then pass it in.
> 
> So how about we:
> 
> * Make GetSerializedShadowArray return a  already_AddRefed<nsCSSValue::Array> and drop the aReorderArray parameter
>   (You will need to create a local RefPtr and then call .forget() on it as part of the return statement)
> * Drop the aProperty parameter since it is only used for a debug-assert and we check the list of properties at its one call site
> * Rename the remaining parameter to just aOriginalArray
> * Rename it to GetReorderedShadowArrayForSerializing (since it doesn't actually serialize anything)
> * Update the comment for that function to something like:
> 
>   /**
>    * Returns a re-ordered version of an nsCSSValue::Array representing a shadow
>    * item (including a drop-shadow() filter function) suitable for serialization.
>    */
> 
> * At the call site you will want to make the |reordered| local variable a RefPtr and move it outside the if block so that it keeps the pointer alive for as long as |array| is used.
> * Update the comment for this call site to something like:
> 
>   // CSSParserImpl::ParseShadowItem stores shadow items in a specific order
>   // that does not match the order we use when serializing computed shadow
>   // items. In order to match the computed value order, we shuffle the items
>   // in the shadow array before serializing it.

Thanks, Brian.

I fixed it by refering the similar implementation of the StyleAnimationValue(AppendFucntion), and I addressed the commit message as well.
Comment on attachment 8891879 [details]
Bug 1377541 - Reorder shadow array when serializing.

https://reviewboard.mozilla.org/r/162904/#review168646

::: layout/style/nsCSSValue.cpp:1316
(Diff revision 2)
> + * Returns a re-ordered version of an csCSSValue::Array representing a shadow
> + * item (including a drop-shado() filter function) suitable for serialization.

As per comment 9, drop-shadow() not drop-shado()

::: layout/style/nsCSSValue.cpp:1320
(Diff revision 2)
> +/**
> + * Returns a re-ordered version of an csCSSValue::Array representing a shadow
> + * item (including a drop-shado() filter function) suitable for serialization.
> + */
> +static already_AddRefed<nsCSSValue::Array>
> +GetReorderedShadowArrayForSerialize(const nsCSSValue::Array* aOriginalArray)

As per comment 9, GetReorderedShadowArrayForSerializing (or GetReorderedShadowArrayForSerialization if you prefer)

::: layout/style/nsCSSValue.cpp:1323
(Diff revision 2)
> + */
> +static already_AddRefed<nsCSSValue::Array>
> +GetReorderedShadowArrayForSerialize(const nsCSSValue::Array* aOriginalArray)
> +{
> +  MOZ_ASSERT(aOriginalArray, "shadow array expected.");
> +  RefPtr<nsCSSValue::Array> reorderArray = nsCSSValue::Array::Create(6);

Let's add a blank link between where we check the function's pre-conditions (assertions) and the rest of the implementation.

::: layout/style/nsCSSValue.cpp:1326
(Diff revision 2)
> +{
> +  MOZ_ASSERT(aOriginalArray, "shadow array expected.");
> +  RefPtr<nsCSSValue::Array> reorderArray = nsCSSValue::Array::Create(6);
> +
> +  reorderArray->Item(0) = aOriginalArray->Item(4); // Color
> +  for (int i = 0; i < 4; i++) {

uint8_t
Attachment #8891879 - Flags: review?(bbirtles)
Comment on attachment 8891879 [details]
Bug 1377541 - Reorder shadow array when serializing.

https://reviewboard.mozilla.org/r/162904/#review168646

Thank you for taking your time for reviewing.
I fixed these points.
Comment on attachment 8891879 [details]
Bug 1377541 - Reorder shadow array when serializing.

https://reviewboard.mozilla.org/r/162904/#review168782

::: layout/style/nsCSSValue.cpp:1322
(Diff revision 3)
> + * item (including a drop-shadow() filter function) suitable for serialization.
> + */
> +static already_AddRefed<nsCSSValue::Array>
> +GetReorderedShadowArrayForSerialization(const nsCSSValue::Array* aOriginalArray)
> +{
> +  MOZ_ASSERT(aOriginalArray, "shadow array expected.");

Nit: No period (.) at the end of assertion messages.

(In the case of a simple null-check, I'd just drop the message altogether.)
Attachment #8891879 - Flags: review?(bbirtles) → review+
Comment on attachment 8891879 [details]
Bug 1377541 - Reorder shadow array when serializing.

https://reviewboard.mozilla.org/r/162904/#review168782

> Nit: No period (.) at the end of assertion messages.
> 
> (In the case of a simple null-check, I'd just drop the message altogether.)

Thanks.
I removed the message.
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0d42c22c1ec6
Change the order of serialized value for mochitest. r=birtles
https://hg.mozilla.org/integration/autoland/rev/9599a3a41754
Reorder shadow array when serializing. r=birtles
https://hg.mozilla.org/mozilla-central/rev/0d42c22c1ec6
https://hg.mozilla.org/mozilla-central/rev/9599a3a41754
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: