Closed Bug 1254418 Opened 10 years ago Closed 10 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: 10 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: