Closed Bug 1254418 Opened 8 years ago Closed 8 years ago

Make Element::GetAnimations work when called on generated-content representing a pseudo element

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: birtles, Assigned: boris)

References

Details

Attachments

(2 files, 4 obsolete files)

Part 1: Support generated-content element for Element.getAnimations (v3)
1.99 KB, patch
birtles
: review+
Details | Diff | Splinter Review
Part 2: Test getAnimations for generated-content elements. (v3, carry birtles' r+)
3.70 KB, patch
boris
: review+
Details | Diff | Splinter Review 
I haven't look into this very deeply, but DevTools currently uses inIDeepTreeWalker to walk the tree including generated-content such as pseudo elements. In order to be able to look up animations on pseudo-elements using this mechanism, we probably need to make Element::GetAnimations() work when |this| is a generated-content element for a pseudo.
Assignee: nobody → boris.chiou
Blocks: 1206420
Attachment #8728876 - Attachment is obsolete: true
Attachment #8728929 - Flags: review?(bbirtles)
Attachment #8728930 - Flags: review?(bbirtles)
Status: NEW → ASSIGNED
Comment on attachment 8728929 [details] [diff] [review]
Part 1: Support generated-content element for Element.getAnimations (v2)

Review of attachment 8728929 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/Element.cpp
@@ +3389,5 @@
>  
> +  // If |this| is a generated-content element, we check the pseudo type, and
> +  // if it has ::before or ::after generated content, get the animations.
> +  nsIFrame* frame = GetPrimaryFrame();
> +  if (frame && frame->IsGeneratedContentFrame()) {

Why do we need this? Could we just do something like:

  nsElement* elem = this;
  nsPseudoType pseudoType = CSSPseudoElementType::NotPseudo;

  // For animations on generated-content elements, the animations are stored
  // on the parent element.
  nsIAtom* name = mNodeInfo->NameAtom();
  if (name == nsGkAtoms::mozgeneratedcontentbefore) {
    elem = GetParentElement();
    pseudoType = CSSPseudoElementType::before;
  } else if (name == nsGkAtoms::mozgeneratedcontentafter) {
    elem = GetParentElement();
    pseudoType = CSSPseudoElementType::after;
  }

  if (!elem) {
    return;
  }

  GetAnimationsUnsorted(elem, pseudoType, aAnimations);
  aAnimations.Sort(AnimationPtrComparator<RefPtr<Animation>>());

That would mean that for other types of generated content, we'd still end up calling GetAnimationsUnsorted, but that's what we do already and it should return an empty list, right?

@@ +3403,5 @@
> +  } else {
> +    GetAnimationsUnsorted(this, CSSPseudoElementType::NotPseudo, aAnimations);
> +  }
> +
> +  if (!aAnimations.IsEmpty()) {

I don't think we need this check, right?

@@ +3418,5 @@
>               aPseudoType == CSSPseudoElementType::after ||
>               aPseudoType == CSSPseudoElementType::before,
>               "Unsupported pseudo type");
>  
> +  if (!aElement) {

Rather than make GetAnimationsUnsorted support a null |aElement| argument, I think we should make the caller check that aElement is non-null before calling this and add a MOZ_ASSERT(aElement) here.
Attachment #8728929 - Flags: review?(bbirtles)
Comment on attachment 8728930 [details] [diff] [review]
Part 2: Test getAnimations for generated-content elements

Review of attachment 8728930 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly good but I'd like to have another look after tweaking it to use testharness.js. Also, I need to check if we should be writing this as a browser-chrome test.

::: dom/animation/test/chrome/test_animation_generated-content-get-animations.html
@@ +1,1 @@
> +<!DOCTYPE html>

Nit: Maybe we can just name this test_generated_content_getAnimations.html

(i.e. make it a bit shorter. Also we only use '-' to separate words in tests that are destined for web-platform-tests since that's the style there.)

@@ +1,4 @@
> +<!DOCTYPE html>
> +<meta charset=utf-8>
> +<title>Test getAnimations() for generated-content elements</title>
> +<script src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>

Any chance you could write this using testharness.js (like test_running_on_compositor.html, test_animate_xrays.html, test_animation_property_state.html)?

We're trying to standardize on testharness.js for animation tests where possible.

@@ +8,5 @@
> +@keyframes anim { }
> +@keyframes anim2 { }
> +.before::before {
> +  content: '';
> +  animation: anim 10s;

This animation (and the ones below) is too short! Let's make it 100s.

@@ +68,5 @@
> +  }
> +
> +  is(animations.length, document.getAnimations().length,
> +     'The number of animations got by DeepTreeWalker and ' +
> +     ' document.getAniamtions() should be the same');

document.getAnimations() (spelling)

(Minor minor nit: We have a space at the end of the first line and at the start of the second line so we have two spaces)

@@ +76,5 @@
> +
> +SimpleTest.waitForExplicitFinish();
> +addLoadEvent(function() {
> +  SpecialPowers.pushPrefEnv(
> +    { "set": [["dom.animations-api.core.enabled", true]] },

We don't need this. This API is always enabled for chrome code.
Attachment #8728930 - Flags: review?(bbirtles)
Boris, if we're using SpecialPowers.unwrap() here for use with inIDeepTreeWalker, should we be writing this as a browser-chrome test instead? I don't know what the criteria is for making tests browser-chrome tests so we can continue testing under e10s.
Flags: needinfo?(bzbarsky)
(In reply to Brian Birtles (:birtles) from comment #4)
> That would mean that for other types of generated content, we'd still end up
> calling GetAnimationsUnsorted, but that's what we do already and it should
> return an empty list, right?

Yes. You're right. I think it too much. Checking the NodeInfo is enough. Thanks.

> @@ +3418,5 @@
> >               aPseudoType == CSSPseudoElementType::after ||
> >               aPseudoType == CSSPseudoElementType::before,
> >               "Unsupported pseudo type");
> >  
> > +  if (!aElement) {
> 
> Rather than make GetAnimationsUnsorted support a null |aElement| argument, I
> think we should make the caller check that aElement is non-null before
> calling this and add a MOZ_ASSERT(aElement) here.

OK. Thanks. I'd like to add the MOZ_ASSERT.
(In reply to Brian Birtles (:birtles) from comment #5)
> Any chance you could write this using testharness.js (like
> test_running_on_compositor.html, test_animate_xrays.html,
> test_animation_property_state.html)?
> 
> We're trying to standardize on testharness.js for animation tests where
> possible.

OK. I will try to use testharness.js.
(In reply to Brian Birtles (:birtles) from comment #5)
> This looks mostly good but I'd like to have another look after tweaking it
> to use testharness.js. Also, I need to check if we should be writing this as
> a browser-chrome test.

BTW, it still works after I remove the SpecialPowers.unwrap(), so I will remove them.
Attachment #8728929 - Attachment is obsolete: true
Attachment #8728930 - Attachment is obsolete: true
Attachment #8729351 - Flags: review?(bbirtles)
Attachment #8729351 - Flags: review?(bbirtles) → review+
Comment on attachment 8729352 [details] [diff] [review]
Part 2: Test getAnimations for generated-content elements (v2)

Review of attachment 8729352 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/animation/test/chrome/test_generated_content_getAnimations.html
@@ +78,5 @@
> +                'The number of animations got by DeepTreeWalker and ' +
> +                'document.getAnimations() should be the same');
> +}, 'Element.getAnimations() used by traversing DeepTreeWalker');
> +
> +done();

Do we need this line? I thought we only needed this when we ran tests in an iframe (after setting a pref from the parent).
Attachment #8729352 - Flags: review+
Flags: needinfo?(bzbarsky)
(In reply to Brian Birtles (:birtles) from comment #12)
> Comment on attachment 8729352 [details] [diff] [review]
> Part 2: Test getAnimations for generated-content elements (v2)
> 
> Review of attachment 8729352 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/test/chrome/test_generated_content_getAnimations.html
> @@ +78,5 @@
> > +                'The number of animations got by DeepTreeWalker and ' +
> > +                'document.getAnimations() should be the same');
> > +}, 'Element.getAnimations() used by traversing DeepTreeWalker');
> > +
> > +done();
> 
> Do we need this line? I thought we only needed this when we ran tests in an
> iframe (after setting a pref from the parent).

I can remove it because it still works. Thanks for your review.
Updated: remove redundant code, 'done()'.
Attachment #8729361 - Flags: review+
Attachment #8729352 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7212aefa6d71
https://hg.mozilla.org/mozilla-central/rev/29ad7ce0f6c3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: