Closed Bug 1259285 Opened 8 years ago Closed 8 years ago

Move CSS/Web Animations-specific visibility handling to StyleAnimationValue

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: birtles, Assigned: mantaroh)

References

Details

Attachments

(2 files, 2 obsolete files)

Summary: Add a utility function to perform the CSS/Web Animations-specific visibility handling to StyleAnimationValue → Move CSS/Web Animations-specific visibility handling to StyleAnimationValue
(In reply to Brian Birtles (:birtles, away 2-10 April) from comment #0)
> See bug 1223658 comment 19, bug 1223658 comment 20, bug 1245748 comment 68

Yoshinaga-san, please see these comments. Why don't we just pass an enum param as suggested?
Comment on attachment 8737679 [details]
MozReview Request: Bug 1259285 - Part1. Move CSS/Web Animations-specific visibility handling. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44087/diff/1-2/
Attachment #8737679 - Attachment description: MozReview Request: Bug 1259285 - Part1.Movve visibility handling function to StyleAnimationValue. r? → MozReview Request: Bug 1259285 - Part1. Move CSS/Web Animations-specific visibility handling. r?birtles
Attachment #8737680 - Attachment description: MozReview Request: Bug 1259285 - Part2.Add web-platform tests for visibility handling. r? → MozReview Request: Bug 1259285 - Part2. Add web-platform test for visibility handling. r?birtles
Attachment #8737679 - Flags: review?(bbirtles)
Attachment #8737680 - Flags: review?(bbirtles)
Comment on attachment 8737680 [details]
MozReview Request: Bug 1259285 - Part2. Add web-platform test for visibility handling. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44089/diff/1-2/
Attachment #8737679 - Flags: review?(bbirtles)
Comment on attachment 8737679 [details]
MozReview Request: Bug 1259285 - Part1. Move CSS/Web Animations-specific visibility handling. r?birtles

https://reviewboard.mozilla.org/r/44087/#review42245

This is getting close but needs a little bit more work. Thanks!

::: dom/animation/KeyframeUtils.cpp:476
(Diff revision 2)
>  
>    for (const Keyframe& frame : aFrames) {
>      nsCSSPropertySet propertiesOnThisKeyframe;
>      for (const PropertyValuePair& pair :
>             PropertyPriorityIterator(frame.mPropertyValues)) {
> +      bool isShorthand = nsCSSProps::IsShorthand(pair.mProperty);

I don't think we need this. nsCSSProps::IsShorthand is very cheap and inlined so I think caching this just complicates the code.

::: dom/animation/KeyframeUtils.cpp:477
(Diff revision 2)
>    for (const Keyframe& frame : aFrames) {
>      nsCSSPropertySet propertiesOnThisKeyframe;
>      for (const PropertyValuePair& pair :
>             PropertyPriorityIterator(frame.mPropertyValues)) {
> +      bool isShorthand = nsCSSProps::IsShorthand(pair.mProperty);
> +      

There's some unnecessary whitespace here.

::: dom/base/nsDOMWindowUtils.cpp
(Diff revision 2)
> -
> -  // This matches TransExtractComputedValue in nsTransitionManager.cpp.
> -  if (aProperty == eCSSProperty_visibility) {
> -    MOZ_ASSERT(aOutput.GetUnit() == StyleAnimationValue::eUnit_Enumerated,
> -               "unexpected unit");
> -    aOutput.SetIntValue(aOutput.GetIntValue(),
> -                        StyleAnimationValue::eUnit_Visibility);
> -  }
> -

It seems like we're removing this line (which implements the clamping behavior) and calling StyleAnimationValue::ComputeValue which does *not* using the clamping behavior.

I think we need to pass the VisibilityHandling flag to ComputeValue too.

::: layout/style/nsCSSProps.h:336
(Diff revision 2)
> +enum VisibilityHandling {
> +  VisibilityClamp,
> +  VisibilityNone
> +};
> +

When I spoke to hiro this morning, he suggested that maybe SMIL would "just work" without this enum, i.e. if we *always* do the special visibility handling.

I had a look at the code and I'm not sure.

From what I can tell:

1. nsSMILAnimationFunction::InterpolateResult calls from->Interpolate
2. from->Interpolate will correspond to nsSMILCSSValueType::Interpolate for animations of 'visibility'
3. nsSMILCSSValueType::Interpolate will call StyleAnimationValue::Interpolate which, for *enumerated* types (other than font-stretch) will fail, i.e. return false[1]
4. If from->Interpolate fails, SMIL will fall back to discrete interpolation which will do the correct SMIL behavior[2]

However, if in step 3 the style animation type is 'visibility' I think the call to Interpolate() may succeed and we will end up getting a different result.

Can you please verify that if we use VisibilityClamp everywhere that some SMIL test fails? Or, alternatively, write a test to prove that the behavior changes?

[1] https://dxr.mozilla.org/mozilla-central/rev/e847cfcb315f511f4928b03fd47dcf57aad05e1e/layout/style/StyleAnimationValue.cpp#2007
[2] https://dxr.mozilla.org/mozilla-central/rev/e847cfcb315f511f4928b03fd47dcf57aad05e1e/dom/smil/nsSMILAnimationFunction.cpp#433

::: layout/style/nsStyleContext.cpp:1286
(Diff revision 2)
>                        nsStyleContext* aStyleContext,
>                        StyleAnimationValue& aResult)
>  {
>    DebugOnly<bool> success =
>      StyleAnimationValue::ExtractComputedValue(aProperty, aStyleContext,
> +                                              VisibilityHandling::VisibilityNone,

It seems like this probably should use the clamping behavior? It's just that we only use it for color at the moment so it doesn't matter?
Comment on attachment 8737680 [details]
MozReview Request: Bug 1259285 - Part2. Add web-platform test for visibility handling. r?birtles

https://reviewboard.mozilla.org/r/44089/#review42251

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:78
(Diff revision 2)
> +  assert_equals(getComputedStyle(div).visibility, 'hidden',
> +                'Before calling animate function, visibility is hidden');

Nit: I don't think these lines are necessary--seems like we're testing that specified style affects computed style which doesn't belong here.

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:80
(Diff revision 2)
> +  var anim = div.animate({visibility:['hidden','visible']}, 100 * 1000);
> +
> +  return anim.ready.then(function(){
> +    assert_equals(getComputedStyle(div).visibility, 'visible',
> +                  'After called animate function, visibility is visible');
> +    anim.currentTime = 100 * 1000;

I'm not sure this is the best test---we don't know how much time has passed between starting the animation and testing the result.

It would be better to just seek the animation using the currentTime and check that it is initially hidden, then set the currentTime to '1' and check that it is visible.

Then we wouldn't need promise_test either.

(Better still, we could add another test with a timing function that goes below zero and check we get 'hidden' while it's below zero.)

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:85
(Diff revision 2)
> +    anim.currentTime = 100 * 1000;
> +
> +    assert_equals(getComputedStyle(div).visibility, 'hidden',
> +                  'After finishing the animation, visibility is hidden');

I'm not sure this is needed?

If you want to test that at the other endpoint we get 'visible' we should set fill: 'forwards' and then call finish().
Attachment #8737680 - Flags: review?(bbirtles)
Actually, thinking about this some more, I think we could probably just do the "clamping" visibility behavior all the time in StyleAnimationValue code and then special-case SMIL instead. e.g. in SMIL code, check if we are animating visibility and then force us to go to 'discrete' animation in that case.
Attached file VisibilityTestcasOfSMIL. (obsolete) —
Thanks Brian!

(In reply to Brian Birtles (:birtles, away 13 April) from comment #10)
> Actually, thinking about this some more, I think we could probably just do
> the "clamping" visibility behavior all the time in StyleAnimationValue code
> and then special-case SMIL instead. e.g. in SMIL code, check if we are
> animating visibility and then force us to go to 'discrete' animation in that
> case.
I tried to apply the "clamping" all the time in StyleAnimationValue, and then failed to smil mochitest.[1]
This attachment file is the simple testcase.
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=292752571b58d631bc58df4471fc1c50b938ceeb

I think that gecko didn't fallback to 'discrete' process in this case. So I think we should ignore SMIL case.
Assignee: nobody → mantaroh
Comment on attachment 8737679 [details]
MozReview Request: Bug 1259285 - Part1. Move CSS/Web Animations-specific visibility handling. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44087/diff/2-3/
Attachment #8737679 - Flags: review?(bbirtles)
Attachment #8737680 - Flags: review?(bbirtles)
Comment on attachment 8737680 [details]
MozReview Request: Bug 1259285 - Part2. Add web-platform test for visibility handling. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44089/diff/2-3/
Comment on attachment 8737679 [details]
MozReview Request: Bug 1259285 - Part1. Move CSS/Web Animations-specific visibility handling. r?birtles

https://reviewboard.mozilla.org/r/44087/#review42895

Getting close!

::: dom/smil/nsSMILAnimationFunction.cpp:389
(Diff revision 3)
> -  if (calcMode != CALC_DISCRETE) {
> +  nsCSSProperty prop = nsSMILCSSValueType::PropertyFromValue(aSMILAttr.GetBaseValue());
> +  if (calcMode != CALC_DISCRETE && prop != eCSSProperty_visibility) {

A few suggestions here:

* Why do we use aSMILAttr.GetBaseValue()? Why can't we just use aValues[0]? We've already asserted above that the length of aValues is at least 1? Then we wouldn't need to pass aSMILAttr?

* Rather than complicate the two if() statements here, why not separate out the test and force calcMode like so:

  if (nsSMILCSSValueType::PropertyFromValue(aValues[0])
      == eCSSProperty_visibility) {
    calcMode = CALC_DISCRETE;
  }

Then the rest of the function can remain untouched?

* This needs a comment to explain why we treat visibility specially (e.g. 'Force discrete calcMode for visibility since StyleAnimationValue will try to interpolate it using the special clamping behavior defined for CSS').

::: dom/smil/nsSMILCSSValueType.h:102
(Diff revision 3)
>     * @return               true on success, false on failure.
>     */
>    static bool ValueToString(const nsSMILValue& aValue, nsAString& aString);
>  
> +  /**
> +   * Return CSS property type of parameter.

Return the CSS property animated by the specified value.

@param aValue The nsSMILValue to examine.
@return The nsCSSProperty enum value of the property animated by |aValue|, or eCSSProperty_UNKNOWN if the type of |aValue| is not nsSMILCSSValueType.

::: dom/smil/nsSMILCSSValueType.cpp:436
(Diff revision 3)
> +  MOZ_ASSERT(aValue.mType == &nsSMILCSSValueType::sSingleton,
> +             "Unexpected SMIL value type");

Why can we assume that aValue is a nsSMILCSSValueType here?

::: layout/style/StyleAnimationValue.cpp:3698
(Diff revision 3)
> +
> +      // 'visibility' requires special handling that is unique to CSS
> +      // transition/CSS animations/Web Animaitons (not SMIL).
> +      // Even if called from SMIL procedure, post processing will skip
> +      // in SMIL.(see nsSMILAnimationFunction)

I think we can drop this comment.
Attachment #8737679 - Flags: review?(bbirtles)
Attachment #8737680 - Flags: review?(bbirtles)
Comment on attachment 8737680 [details]
MozReview Request: Bug 1259285 - Part2. Add web-platform test for visibility handling. r?birtles

https://reviewboard.mozilla.org/r/44089/#review42905

Thanks! Looking good but I want to have one more check with the fixes made.

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:75
(Diff revision 3)
> +  var anim = div.animate({visibility: ['hidden','visible']},
> +                         {duration: 1 * MS_PER_SEC,

Nit: I think standard JS style is to put a space between the { and object members, e.g.

  var anim = div.animate({ visibility: ['hidden', 'visible'] },
                         { duration: 1 * MS_PER_SEC, fill: 'both' });

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:76
(Diff revision 3)
>     + ' side of the overlap point');
>  
> +test(function(t) {
> +  var div = createDiv(t);
> +  var anim = div.animate({visibility: ['hidden','visible']},
> +                         {duration: 1 * MS_PER_SEC,

I think hiro would prefer we use 100 here for consistency.

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:79
(Diff revision 3)
> +  var div = createDiv(t);
> +  var anim = div.animate({visibility: ['hidden','visible']},
> +                         {duration: 1 * MS_PER_SEC,
> +                          fill: 'both'});
> +
> +  anim.pause();

This isn't necessary since we never let the animation run.

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:81
(Diff revision 3)
> +  // visibility behavior defined at 'Animation Model' part.
> +  // http://w3c.github.io/web-animations/#other-animation-behaviors

It's probably best just to drop this comment since it's not actually defined there yet and might never be.

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:85
(Diff revision 3)
> +
> +  // visibility behavior defined at 'Animation Model' part.
> +  // http://w3c.github.io/web-animations/#other-animation-behaviors
> +  anim.currentTime = 0;
> +  assert_equals(getComputedStyle(div).visibility, 'hidden',
> +                'In Before phase, visibility is hidden');

Nit: 'before' (lower case). Also, I think it's more about when the progress is zero than when we are in the before phase. We're not actually in the before phase at this point in any case, we're at the start of the active phase.

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:87
(Diff revision 3)
> +  // http://w3c.github.io/web-animations/#other-animation-behaviors
> +  anim.currentTime = 0;
> +  assert_equals(getComputedStyle(div).visibility, 'hidden',
> +                'In Before phase, visibility is hidden');
> +
> +  anim.currentTime = (1/2) * MS_PER_SEC;

I think just anim.currentTime = 1 would be better?

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:91
(Diff revision 3)
> +
> +  anim.currentTime = (1/2) * MS_PER_SEC;
> +  assert_equals(getComputedStyle(div).visibility, 'visible',
> +                'In Active phase, visibility is visible');
> +
> +  anim.currentTime = 1 * MS_PER_SEC;

anim.finish()?

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:95
(Diff revision 3)
> +
> +  anim.currentTime = 1 * MS_PER_SEC;
> +  assert_equals(getComputedStyle(div).visibility, 'visible',
> +                'In After phase, visibility is visible');
> +
> +}, "Confirm visibility behavior.");

'visibility clamping behavior'

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:100
(Diff revision 3)
> +}, "Confirm visibility behavior.");
> +
> +test(function(t) {
> +  var div = createDiv(t);
> +  var anim = div.animate({visibility: ['hidden', 'visible']},
> +                         {duration: 1 * MS_PER_SEC,

Likewise, 100 here.

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:103
(Diff revision 3)
> +  anim.pause();
> +

Again, this is not needed.

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:114
(Diff revision 3)
> +  // Timing function is below zero. So we expected visibility is hidden.
> +  anim.currentTime = 1;
> +  assert_equals(getComputedStyle(div).visibility, 'hidden',
> +                'In Active phase, visibility is visible');
> +
> +  anim.currentTime = (1/2) * MS_PER_SEC;

0.5 might be easier to read?

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:118
(Diff revision 3)
> +
> +  anim.currentTime = (1/2) * MS_PER_SEC;
> +  assert_equals(getComputedStyle(div).visibility, 'visible',
> +                'In Active phase, visibility is visible');
> +
> +}, "Confirm visibility behavior with cubic-bazier.");

'visibility clamping behavior with an easing that has a negative component'
Comment on attachment 8737679 [details]
MozReview Request: Bug 1259285 - Part1. Move CSS/Web Animations-specific visibility handling. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44087/diff/3-4/
Attachment #8737679 - Flags: review?(bbirtles)
Attachment #8737680 - Flags: review?(bbirtles)
Comment on attachment 8737680 [details]
MozReview Request: Bug 1259285 - Part2. Add web-platform test for visibility handling. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44089/diff/3-4/
Comment on attachment 8737679 [details]
MozReview Request: Bug 1259285 - Part1. Move CSS/Web Animations-specific visibility handling. r?birtles

https://reviewboard.mozilla.org/r/44087/#review43567

r=me with comments addressed

::: dom/smil/nsSMILAnimationFunction.cpp:389
(Diff revision 4)
>    }
>  
>    nsresult rv = NS_OK;
>    nsSMILCalcMode calcMode = GetCalcMode();
> +
> +  // Force discrete calcMode for visibility when StyleAnimationValue will

s/when/since/

::: dom/smil/nsSMILCSSValueType.h:105
(Diff revision 4)
> +   * @return           The nsCSSProperty enum value of the property animated
> +                       by |aValue|, or eCSSProperty_UNKNOWN if the type of
> +                       |aValue| is not nsSMILCSSValueType.

Each of these lines should still start with a '*'.

::: layout/style/StyleAnimationValue.cpp:3698
(Diff revision 4)
>          StyleDataAtOffset(styleStruct, ssOffset)));
>        return true;
>      case eStyleAnimType_EnumU8:
>        aComputedValue.SetIntValue(*static_cast<const uint8_t*>(
>          StyleDataAtOffset(styleStruct, ssOffset)), eUnit_Enumerated);
> +

Nit: I think we could probably drop this blank line--it seems like that might be more consistent with other places in this function.
Attachment #8737679 - Flags: review?(bbirtles) → review+
Comment on attachment 8737680 [details]
MozReview Request: Bug 1259285 - Part2. Add web-platform test for visibility handling. r?birtles

https://reviewboard.mozilla.org/r/44089/#review43569

r=me with comments addressed

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:81
(Diff revisions 3 - 4)
> -
> -  // visibility behavior defined at 'Animation Model' part.
> -  // http://w3c.github.io/web-animations/#other-animation-behaviors
>    anim.currentTime = 0;
>    assert_equals(getComputedStyle(div).visibility, 'hidden',
> -                'In Before phase, visibility is hidden');
> +                'In before phase, visibility is hidden');

I don't really think we want to talk about phases here. The visibility handling is independent of phases--it's purely based on the progress.

So I think it would be better to drop the delay and just change the comment to 'When progress is zero, visibility is hidden'.

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:85
(Diff revisions 3 - 4)
>    assert_equals(getComputedStyle(div).visibility, 'hidden',
> -                'In Before phase, visibility is hidden');
> +                'In before phase, visibility is hidden');
>  
> -  anim.currentTime = (1/2) * MS_PER_SEC;
> +  anim.currentTime = 10 * MS_PER_SEC + 1;
>    assert_equals(getComputedStyle(div).visibility, 'visible',
>                  'In Active phase, visibility is visible');

'When progress value > 0, visibility is visible'

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:89
(Diff revisions 3 - 4)
>    assert_equals(getComputedStyle(div).visibility, 'visible',
>                  'In Active phase, visibility is visible');
>  
> -  anim.currentTime = 1 * MS_PER_SEC;
> +  anim.finish();
>    assert_equals(getComputedStyle(div).visibility, 'visible',
>                  'In After phase, visibility is visible');

When progress = 1, visibility is visible

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:91
(Diff revisions 3 - 4)
>  
> -  anim.currentTime = 1 * MS_PER_SEC;
> +  anim.finish();
>    assert_equals(getComputedStyle(div).visibility, 'visible',
>                  'In After phase, visibility is visible');
>  
> -}, "Confirm visibility behavior.");
> +}, "Confirm visibility clamping behavior.");

As per my previous comment, "Confirm" here is a bit odd. Either "Test visibility clamping behavior" or even just "visibility clamping behavior" would be ok.

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:98
(Diff revisions 3 - 4)
>  test(function(t) {
>    var div = createDiv(t);
> -  var anim = div.animate({visibility: ['hidden', 'visible']},
> -                         {duration: 1 * MS_PER_SEC,
> -                          fill: 'both',
> -                          easing: 'cubic-bezier(0.25, -0.6, 0, 0.5)'});
> +  var anim = div.animate({ visibility: ['hidden', 'visible'] },
> +                         { duration: 100 * MS_PER_SEC, fill: 'both',
> +                           easing: 'cubic-bezier(0.25, -0.6, 0, 0.5)',
> +                           delay: 10 * MS_PER_SEC });

As with the previous test, we should drop the delay.

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:102
(Diff revisions 3 - 4)
> -                          easing: 'cubic-bezier(0.25, -0.6, 0, 0.5)'});
> +                           delay: 10 * MS_PER_SEC });
> -  anim.pause();
>  
>    anim.currentTime = 0;
>    assert_equals(getComputedStyle(div).visibility, 'hidden',
> -                'In Before phase, visibility is hidden');
> +                'In before phase, visibility is hidden');

We should drop mentions of phases here and below.

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:107
(Diff revisions 3 - 4)
> -                'In Before phase, visibility is hidden');
> +                'In before phase, visibility is hidden');
>  
>    // Timing function is below zero. So we expected visibility is hidden.
> -  anim.currentTime = 1;
> +  anim.currentTime = 10 * MS_PER_SEC + 1;
>    assert_equals(getComputedStyle(div).visibility, 'hidden',
>                  'In Active phase, visibility is visible');

When progress < 0 due to easing, visibility is hidden.

Note, the current comment says "visibility is visible"--we could avoid this kind of bug by just making the messages shorter such as:

  'visibility when progress = 0'
  'visibility when progress < 0 due to easing'
  'visibility when progress > 0 due to easing'
  'visibility when progress = 1'

::: testing/web-platform/tests/web-animations/keyframe-effect/keyframe-handling.html:113
(Diff revisions 3 - 4)
>  
> -  anim.currentTime = (1/2) * MS_PER_SEC;
> +  anim.currentTime = 60 * MS_PER_SEC;
>    assert_equals(getComputedStyle(div).visibility, 'visible',
>                  'In Active phase, visibility is visible');
>  
> -}, "Confirm visibility behavior with cubic-bazier.");
> +}, "Confirm visibility clamping behavior with an easing that has a negative component");

s/Confirm/Test/ or s/Confirm//
Attachment #8737680 - Flags: review?(bbirtles) → review+
Comment on attachment 8737679 [details]
MozReview Request: Bug 1259285 - Part1. Move CSS/Web Animations-specific visibility handling. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44087/diff/4-5/
Comment on attachment 8737680 [details]
MozReview Request: Bug 1259285 - Part2. Add web-platform test for visibility handling. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44089/diff/4-5/
https://reviewboard.mozilla.org/r/44089/#review43569

> I don't really think we want to talk about phases here. The visibility handling is independent of phases--it's purely based on the progress.
> 
> So I think it would be better to drop the delay and just change the comment to 'When progress is zero, visibility is hidden'.

Thanks Brian.

I've misunderstood about relation of visibility behavior and phase. So I modified the test description and test parameter(delay).
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91f88ba657d55229d0c8af177e06a901cc1d5160
Try result hit the crash on Mac/Linux. Perhaps, the cause of this phenomenon is memory invalid error.

|PropertyFromValue| casted |nsSMILValue.mU.mPtr| to |ValueWrapper|, however there are several |nsSMILValue| which have not castable |mU.mPtr| value. (e.g. |nsSMILLength|[1] haven't castable |mU.mPtr| value.)

[1] https://dxr.mozilla.org/mozilla-central/rev/1f16d3da9280e40ada252acf8110b91ee1edbb08/dom/svg/nsSVGLength2.cpp#475

So we should consider those case.
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #24)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=91f88ba657d55229d0c8af177e06a901cc1d5160
> Try result hit the crash on Mac/Linux. Perhaps, the cause of this phenomenon
> is memory invalid error.
> 
> |PropertyFromValue| casted |nsSMILValue.mU.mPtr| to |ValueWrapper|, however
> there are several |nsSMILValue| which have not castable |mU.mPtr| value.
> (e.g. |nsSMILLength|[1] haven't castable |mU.mPtr| value.)

We shouldn't be casting at all if the type doesn't match. We need to add a test to the start of nsSMILCSSValueType::PropertyFromValue that checks:

  if (aValue.mType != &nsSMILCSSValueType::sSingleton) {
    return eCSSProperty_UNKNOWN;
  }

We should't need the null check after that (since null is just another type).
Comment on attachment 8737679 [details]
MozReview Request: Bug 1259285 - Part1. Move CSS/Web Animations-specific visibility handling. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44087/diff/5-6/
Comment on attachment 8737680 [details]
MozReview Request: Bug 1259285 - Part2. Add web-platform test for visibility handling. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44089/diff/5-6/
Attachment #8742651 - Flags: review?(bbirtles) → review+
Comment on attachment 8742651 [details]
MozReview Request: Bug 1259285 - Part3. Modified nsSMILCSSValueType::PropertyFromValue for prevent invalid nsSMILValue. r?birtles.

https://reviewboard.mozilla.org/r/47393/#review44077

r=me but please fold this into part 1 before landing.

::: dom/smil/nsSMILCSSValueType.cpp:436
(Diff revision 1)
>  
>  // static
>  nsCSSProperty
>  nsSMILCSSValueType::PropertyFromValue(const nsSMILValue& aValue)
>  {
> +  // We can get the property id only nsSMILCSSValueType.

Nit: I think we don't need this comment.
Comment on attachment 8742651 [details]
MozReview Request: Bug 1259285 - Part3. Modified nsSMILCSSValueType::PropertyFromValue for prevent invalid nsSMILValue. r?birtles.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47393/diff/1-2/
Comment on attachment 8737679 [details]
MozReview Request: Bug 1259285 - Part1. Move CSS/Web Animations-specific visibility handling. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44087/diff/6-7/
Comment on attachment 8737680 [details]
MozReview Request: Bug 1259285 - Part2. Add web-platform test for visibility handling. r?birtles

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44089/diff/6-7/
Attachment #8742651 - Attachment is obsolete: true
Attachment #8740809 - Attachment is obsolete: true
Thanks Brian.

(In reply to Brian Birtles (:birtles) from comment #31)
> Comment on attachment 8742651 [details]
> MozReview Request: Bug 1259285 - Part3. Modified
> nsSMILCSSValueType::PropertyFromValue for prevent invalid nsSMILValue.
> r?birtles.
> 
> https://reviewboard.mozilla.org/r/47393/#review44077
> 
> r=me but please fold this into part 1 before landing.
> 
> ::: dom/smil/nsSMILCSSValueType.cpp:436
> (Diff revision 1)
> >  
> >  // static
> >  nsCSSProperty
> >  nsSMILCSSValueType::PropertyFromValue(const nsSMILValue& aValue)
> >  {
> > +  // We can get the property id only nsSMILCSSValueType.
> 
> Nit: I think we don't need this comment.
I folded the patch of part1 and part3.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eea2efe445bc
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dbe9e023494e
https://hg.mozilla.org/mozilla-central/rev/c56123334629
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.