Closed Bug 1450526 Opened 6 years ago Closed 6 years ago

Animation on pseudo element is not shown up in animation inspector

Categories

(DevTools :: Inspector: Animations, defect, P3)

Product:
DevTools
Component:
Inspector: Animations
Type:
defect

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: hiro, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

Attachments

(8 files, 5 obsolete files)

An example
1.15 KB, text/html
Details
Bug 1450526 - Part 1: Make pseudo element to be available in new animation inspector.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1450526 - Part 2: Change the target node of animation on pseudo element in the actor to proper node.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1450526 - Part 4: Show pseudo element in Reps component.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1450526 - Part 5: Add test for pseudo element.
59 bytes, text/x-review-board-request
pbro
: review+
Details
pseudo.html
796 bytes, text/html
Details
Expose the generated pseudo element in KeyframeEffect
1.89 KB, patch
Details | Diff | Splinter Review
Bug 1450526 - Part 3: Update node front when the indicated node was re-generated.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Attached file An example — 
Attaching file has an animation, but the animation isn't shown up in animation inspector at all. (Though the file is a test case for bug 1443358).

I am surprised that our automation test cases haven't failed.   Is the animation in the file is a particular case?
Flags: needinfo?(dakatsuka)
Thanks, Hiro.
I'm sorry, I had not added the test for pseudo element animation yet.
Also, yeah, I had found that the new one does not work well for pseudo element at a few days ago.
Will fix asap.
Flags: needinfo?(dakatsuka)
Summary: Animation is not shown up in animation inspector → Animation on pseudo element is not shown up in animation inspector
Assignee: nobody → dakatsuka
Blocks: 1399830
Comment on attachment 8964606 [details]
Bug 1450526 - Part 1: Make pseudo element to be available in new animatoin inspector.

https://reviewboard.mozilla.org/r/233322/#review238896


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/animation/test/head.js:469
(Diff revision 1)
>   * Wait for all AnimationTarget components to be fully loaded
>   * (fetched their related actor and rendered).
>   *
>   * @param {AnimationInspector} animationInspector
>   */
>  const waitForAllAnimationTargets = async function(animationInspector) {

Error: Expected to return a value at the end of async function. [eslint: consistent-return]
Comment on attachment 8964608 [details]
Bug 1450526 - Part 3: Add test for pseudo element.

https://reviewboard.mozilla.org/r/233326/#review238898


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/inspector/animation/test/browser_animation_pseudo-element.js:21
(Diff revision 1)
> +  },
> +];
> +
> +add_task(async function() {
> +  await addTab(URL_ROOT + "doc_pseudo.html");
> +  const { animationInspector, inspector, panel } = await openAnimationInspector();

Error: 'animationinspector' is assigned a value but never used. [eslint: no-unused-vars]

::: devtools/client/inspector/animation/test/browser_animation_pseudo-element.js:21
(Diff revision 1)
> +  },
> +];
> +
> +add_task(async function() {
> +  await addTab(URL_ROOT + "doc_pseudo.html");
> +  const { animationInspector, inspector, panel } = await openAnimationInspector();

Error: 'inspector' is assigned a value but never used. [eslint: no-unused-vars]
Comment on attachment 8964606 [details]
Bug 1450526 - Part 1: Make pseudo element to be available in new animatoin inspector.

Please let me cancel this review because I found one bug about selecting a node from Rep.
Attachment #8964606 - Flags: review?(pbrosset)
Attachment #8964607 - Flags: review?(pbrosset)
Attachment #8964608 - Flags: review?(pbrosset)
Attachment #8964606 - Attachment is obsolete: true
Attachment #8964607 - Attachment is obsolete: true
Attachment #8964608 - Attachment is obsolete: true
Comment on attachment 8964827 [details]
Bug 1450526 - Part 1: Make pseudo element to be available in new animation inspector.

https://reviewboard.mozilla.org/r/233570/#review239270

::: devtools/client/inspector/animation/animation.js
(Diff revision 1)
>    updateElementPickerEnabled,
>    updateSelectedAnimation,
>    updateSidebarSize
>  } = require("./actions/animations");
>  const {
> -  isAllAnimationEqual,

Is this util used somewhere else now? If this was the only user of it, then we need to remove the util functional.

::: devtools/client/inspector/animation/animation.js:136
(Diff revision 1)
>          }
>        )
>      );
>      this.provider = provider;
>  
> +    this.animationsFront.setWalkerActor(this.inspector.walker);

Oh so we weren't doing this before this change? Good that you're adding this because this makes the panel much more performant.

I have a question: can you explain how setting the walker helps with showing pseudo elements. The commit is about pseudo elements, but I don't understand how this helps. I means it's good to add it anyway, but I want to make sure it goes in a commit that explains why we did this.

::: devtools/client/inspector/animation/animation.js:144
(Diff revision 1)
> +    if (this.inspector.isReady) {
> +      activate();
> +    } else {
> +      // Wait for preparing the markup-view becase the selectable node on the view
> +      // might be not available yet.
> +      this.inspector.once("ready", () => {
> +        activate();
> +        this.onSidebarSelectionChanged();
> +      });
> +    }

Same question here. I don't understand how this change (which seems good by the way) is related to showing pseudo elements in the panel.
Maybe it's just that the commit message needs to be changed to explain what the change is about.
Attachment #8964827 - Flags: review?(pbrosset)
Comment on attachment 8964828 [details]
Bug 1450526 - Part 2: Change the target node of animation on pseudo element in the actor to proper node.

https://reviewboard.mozilla.org/r/233572/#review239274

::: devtools/server/actors/animation.js:109
(Diff revision 1)
>        let treeWalker = this.walker.getDocumentWalker(node.parentElement);
>        while (treeWalker.nextNode()) {
>          let currentNode = treeWalker.currentNode;
> +
> +        if (node.parentElement !== currentNode.parentElement) {
> +          // The DocumentWalker walks even on the parents and other children.

I'm not sure I understand what other elements the walker ends up on in this case. Can you maybe rephrase this line and give an example?
Attachment #8964828 - Flags: review?(pbrosset) → review+
Comment on attachment 8964829 [details]
Bug 1450526 - Part 4: Show pseudo element in Reps component.

https://reviewboard.mozilla.org/r/233574/#review239276

/devtools/client/shared/components/reps/reps.js is actually a file that is maintained in another repository: https://github.com/devtools-html/devtools-core/tree/master/packages/devtools-reps
We don't modify this file in m-c, instead we work on it in github, and then copy the code from github to m-c when we do a release.
You should get in touch with nchevobbe to learn more about this. I believe the file you need to modify is: https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-reps/src/reps/element-node.js
Attachment #8964829 - Flags: review?(pbrosset)
Comment on attachment 8964830 [details]
Bug 1450526 - Part 5: Add test for pseudo element.

https://reviewboard.mozilla.org/r/233576/#review239278

::: devtools/client/inspector/animation/test/head.js:172
(Diff revision 1)
> +  const bounds = buttonEl.getBoundingClientRect();
> +  const x = bounds.width / 2;
> +  const y = bounds.height / 2;
> +  EventUtils.synthesizeMouse(buttonEl, x, y, {}, buttonEl.ownerGlobal);

Isn't there a synthesizeMouseInCenter you can use instead of having to do this yourself?
Attachment #8964830 - Flags: review?(pbrosset) → review+
Comment on attachment 8964829 [details]
Bug 1450526 - Part 4: Show pseudo element in Reps component.

https://reviewboard.mozilla.org/r/233574/#review239390

::: devtools/client/shared/components/reps/reps.js:4428
(Diff revision 1)
>  
>    return span(baseConfig, ...elements, inspectIcon);
>  }
>  
>  function getElements(grip, mode) {
> -  let { attributes, nodeName } = grip.preview;
> +  let {

You can't modify this file. You have to modify the reps upstream https://github.com/devtools-html/devtools-core/tree/master/packages/devtools-reps
Oh sorry, looks like pbro got to this before I did and I failed to see it.
Comment on attachment 8964827 [details]
Bug 1450526 - Part 1: Make pseudo element to be available in new animation inspector.

https://reviewboard.mozilla.org/r/233570/#review239270

> Is this util used somewhere else now? If this was the only user of it, then we need to remove the util functional.

Thank you very much for your quick reviewing!
Yes, I forgot to drop the function.
Thanks.

> Oh so we weren't doing this before this change? Good that you're adding this because this makes the panel much more performant.
> 
> I have a question: can you explain how setting the walker helps with showing pseudo elements. The commit is about pseudo elements, but I don't understand how this helps. I means it's good to add it anyway, but I want to make sure it goes in a commit that explains why we did this.

The walker is needed in animation.js on the server to get actual pseudo node.
https://searchfox.org/mozilla-central/source/devtools/server/actors/animation.js#104

> Same question here. I don't understand how this change (which seems good by the way) is related to showing pseudo elements in the panel.
> Maybe it's just that the commit message needs to be changed to explain what the change is about.

I tested again, this change was no need. Will drop this.
(I got an error from CssLogic.getComputedStyle in the server when the animation inspector is opened at same time to booting the inspector without this changing. But this fixed in patch 2. The reason is that we could not get node correctly.)
Attached file pseudo.html — 

    
    

    
  
(In reply to Patrick Brosset <:pbro> from comment #15)
> Comment on attachment 8964828 [details]
> Bug 1450526 - Part 2: Change the target node of animation on pseudo element
> in the actor to correct node.
> 
> https://reviewboard.mozilla.org/r/233572/#review239274
> 
> ::: devtools/server/actors/animation.js:109
> (Diff revision 1)
> >        let treeWalker = this.walker.getDocumentWalker(node.parentElement);
> >        while (treeWalker.nextNode()) {
> >          let currentNode = treeWalker.currentNode;
> > +
> > +        if (node.parentElement !== currentNode.parentElement) {
> > +          // The DocumentWalker walks even on the parents and other children.
> 
> I'm not sure I understand what other elements the walker ends up on in this
> case. Can you maybe rephrase this line and give an example?

The DocumentWalker (inDeepTreeWalker) finds the next node the following rule:
1. first child
2. if no first child, sibling node
3. if no sibling node, parent node

So, the walker returns a node as like below order by nextNode() in case of attachment 8965204 [details].

body::after case:
1. #div1
2. #div1::after
3. #div2
4. #div3
5. #div3 text
6. #div3::after
7. #div2::after
8. body::after

#div1::after case:
1. #div1::after
2. #div2
3. #div3
4. #div3 text
5. #div3::after
6. #div2::after
7. body::after

#div2::after case:
1. #div3
2. #div3 text
3. #div3::after
4. #div2::after
5. body::after

#div3::after case:
1. #div3 text
2. #div3::after
3. #div2::after
4. body::after

So, always the body::after is chosen in original code.
(We can confirm that if mouse over on the animation target of animation inspector in Firefox release.)

If we `break` the loop immediately when find "::after", the chosen node would be:
body::after -> #div1::after
#div1::after -> #div1::after
#div2::after -> #div3::after
#div3::after -> #div3::after

Therefor, we need to check the parent node.

Also, we might not need to check the nodeName if we introduce the custom filter to the walker instead of standardTreeWalkerFilter[1]. Furthermore,  might not need the loop if we check the parent node in the filter.
I'll try that.

[1] https://searchfox.org/mozilla-central/source/devtools/server/actors/inspector/utils.js#63

    
    

    
  
I had tried the filter, but it did not work well.
The reason is, although we could get the node correctly, the parentNode of that will be null in CssLogic.getComputedStyle.
Also, I tried another one like below:

const treeWalker = this.walker.getDocumentWalker(node.parentElement);
this._node =
 node.type === "::before" ? treeWalker.firstChild() : treeWalker.lastChild();
console.log(this._node.parentNode);

In this case as well, We could get the target node, and see the parentNode in this console.log, but got same error.
And I added the below code before above code:

const treeWalkerDummy = this.walker.getDocumentWalker(node.parentElement);
while (treeWalkerDummy.nextNode()) {}

It works well,,
(I wonder this is a bug of inDeepTreeWalker?)
Do you have any idea?
Or may I use as it is, at least in this bug?
Flags: needinfo?(pbrosset)

    
    

    
  
(In reply to Daisuke Akatsuka (:daisuke) from comment #23)
> Or may I use as it is, at least in this bug?
Sorry. I meant that we use the code which you reviewed.

    
    

    
  
Comment on attachment 8964829 [details]
Bug 1450526 - Part 4: Show pseudo element in Reps component.

https://reviewboard.mozilla.org/r/233574/#review239276

Thanks, I'll pull-request to the repository.

By the way, if possible, I'd like to change the target node name to like div.className::before, not only ::before, to understand easily.

But, if select animated node on markup-view, the parent node of the pseudo element is a highlighting element of DevTools. So the name will be div.highlightXXX::before.
Is there any ways to recognize the element was appended by DevTools?

(But this should be fixed in another bug.)

    
    

    
  
Comment on attachment 8964830 [details]
Bug 1450526 - Part 5: Add test for pseudo element.

https://reviewboard.mozilla.org/r/233576/#review239278

> Isn't there a synthesizeMouseInCenter you can use instead of having to do this yourself?

I'll use this. Thanks.

By the way, I have one question.
If we should change the Reps component in GitHub, how should we test the content of target node label?
In this bug, the label should be _moz_generated_content_before / _moz_generated_content_after.
But, after fix the Reps, the label will be ::before / ::after.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
This test does not pass before Reps component merged to m-c.
https://github.com/devtools-html/devtools-core/pull/1024

    
    

    
  
Comment on attachment 8964828 [details]
Bug 1450526 - Part 2: Change the target node of animation on pseudo element in the actor to proper node.

Patrick, please let me cancel this patch.
Because the same error happen with another html, when open animation inspector and inspector at same time.

The strange point is, if I add/remove the `break`, the behavior will change..
At line 114 in https://reviewboard.mozilla.org/r/233572/diff/2#index_header.
If remove `break`, the parentElement will be null at below:
https://searchfox.org/mozilla-central/source/devtools/server/actors/animation.js#72

Let me investigate more..
Thanks.

        Attachment #8964828 -
        Flags: review+

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
The reason why the parentElement of pseudo element will be null was that _moz_generated_content_before/after was re-generated again.
node.getBoxQuads of nodeHasSize[1] induces that, and nodeHasSize is called from filter[2] in document-walker.  In getBoxQuads[3], if the node is text node, passed through the [4], call RestyleManager to reconstruct the frames[5]. _moz_generated_content_before/after might be generated in here. Therefor the parentElement which originally we are referring might be null.

I tried that added a new function which gets bounds of text node without flush( patch 2 ), then call this instead of getBoxQuads( patch 3 ). Also, likewise regarding to nsIDOMElement as well, use nsDOMWindowUtils.getBoundsWithoutFlush. (It may improve the performance.)
In try server[6], it looks there are no problem for DevTools tasks by this change.

At least, I don't think, we need to flush the style in nodeHasSize. But if we need, it might be better to do explicitly.
Though I have tried like this approach, how do you think, Patrick?
If ok, I’d like to request the review for patch 2 as well.

[1] https://searchfox.org/mozilla-central/source/devtools/server/actors/inspector/utils.js#117
[2] https://searchfox.org/mozilla-central/source/devtools/server/actors/inspector/utils.js#73
[3] https://searchfox.org/mozilla-central/source/layout/base/GeometryUtils.cpp#251
[4] https://searchfox.org/mozilla-central/source/layout/base/GeometryUtils.cpp#40
[5] https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#8292
[6] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9134180c8b5cb919320fd1331bff213545a9d98

    
    

    
  
A dumb question, why do we need to traverse tree to find the pseudo element?  As far as I can tell, animation's effect target is truly pseudo element.  |this.player.effect.target| is different from the animation's effect target?  If so, why do we try to fix it?

    
    

    
  
`this._node` is used to create node actor[1] to highlight or select the node in animation inspector.
Likewise, other inspectors inspect, change the style, highlighting and so on via the actor, then do something to the actor.rawNode as same as normal element (e.g. changing the style [2]). 
However, because the CSSPseudoElement of animation.effect.target is not nsIDOMElement, we can not do directly.
This is why we need to find generated pseudo element again.

[1] https://searchfox.org/mozilla-central/source/devtools/server/actors/inspector/node.js
[2] https://searchfox.org/mozilla-central/source/devtools/server/actors/styles.js#1286

    
    

    
  
Gah, I thought the CSSPSeudoElement inherits Element...  We have to fix 'issue #9' in https://drafts.csswg.org/css-pseudo-4/#issues-index .

    
    

    
  
Or, we should expose the real pseudo element in KeyframeEffect with chrome privilege so that we don't need to traverse the tree.  Anyway it should be done in a follow-up bug.

    
    

    
  
Comment on attachment 8964829 [details]
Bug 1450526 - Part 4: Show pseudo element in Reps component.

https://reviewboard.mozilla.org/r/233574/#review240524

        Attachment #8964829 -
        Flags: review?(pbrosset) → review+

    
    

    
  
Comment on attachment 8964827 [details]
Bug 1450526 - Part 1: Make pseudo element to be available in new animation inspector.

https://reviewboard.mozilla.org/r/233570/#review240526

::: commit-message-00bdc:1
(Diff revision 3)
> +Bug 1450526 - Part 1: Make pseudo element to be available in new animatoin inspector. r?pbro

s/animatoin/animation

        Attachment #8964827 -
        Flags: review?(pbrosset) → review+

    
    

    
  
(In reply to Hiroyuki Ikezoe (:hiro) from comment #42)
> Or, we should expose the real pseudo element in KeyframeEffect with chrome
> privilege so that we don't need to traverse the tree.  Anyway it should be
> done in a follow-up bug.
Yeah, this would be really good indeed.

    
    

    
  
(In reply to Daisuke Akatsuka (:daisuke) from comment #38)
> The reason why the parentElement of pseudo element will be null was that
> _moz_generated_content_before/after was re-generated again.
> node.getBoxQuads of nodeHasSize[1] induces that, and nodeHasSize is called
> from filter[2] in document-walker.  In getBoxQuads[3], if the node is text
> node, passed through the [4], call RestyleManager to reconstruct the
> frames[5]. _moz_generated_content_before/after might be generated in here.
> Therefor the parentElement which originally we are referring might be null.
I'm not sure I follow quite exactly what's going on here. Let me try to explain what I understand so you can tell me if I'm wrong:

While walking the DOM using the document-walker, we call nodeHasSize on every node, but since this forces a layout flush to calculate the size, the pseudo elements are re-generated. And so, the ones we stored previously aren't the right objects anymore (they don't have a parentElement). Is that right?

Sounds like a hard problem to solve. I suppose you could always do stuff while walking that would add/remove elements that end up making the results of your walk incorrect in the end.
If the walker can't easily guaranty the existence of the pseudo before/after nodes while walking, even if we cause a flush (because of nodeHasSize), then we indeed need to fix this in DevTools by calling something else.

Maybe your approach is the right one. Maybe there's another API we could use instead of getBoxQuads that doesn't have the side-effect of reconstructing frames?
In any case, I agree it is something that should be done explicitly, and with a code comment.
Flags: needinfo?(pbrosset)

    
    

    
  
(In reply to Patrick Brosset <:pbro> (away, back April 19) from comment #46)
> (In reply to Daisuke Akatsuka (:daisuke) from comment #38)
> > The reason why the parentElement of pseudo element will be null was that
> > _moz_generated_content_before/after was re-generated again.
> > node.getBoxQuads of nodeHasSize[1] induces that, and nodeHasSize is called
> > from filter[2] in document-walker.  In getBoxQuads[3], if the node is text
> > node, passed through the [4], call RestyleManager to reconstruct the
> > frames[5]. _moz_generated_content_before/after might be generated in here.
> > Therefor the parentElement which originally we are referring might be null.
> I'm not sure I follow quite exactly what's going on here. Let me try to
> explain what I understand so you can tell me if I'm wrong:
> 
> While walking the DOM using the document-walker, we call nodeHasSize on
> every node, but since this forces a layout flush to calculate the size, the
> pseudo elements are re-generated. And so, the ones we stored previously
> aren't the right objects anymore (they don't have a parentElement). Is that
> right?

It might have related to the browsing document.
The pseudo element re-generated just once each timing of opening / closing the inspector. (by nodeHasSize)
While running the inspector, although we call EnsureFrameForTextNodeIsCreatedAfterFlush by nodeHasSize, doesn't re-generate because the node has no NS_CREATE_FRAME_IF_NON_WHITESPACE[1].
(You can confirm the timing of re-generating with printf like below around here[2])

But, once we close the inspector, and reload the browsing document, then open inspector, the pseudo elements on the document will re-generate again.
Or are we changing the document at the opening the inspector??
But, although I don't know when NS_CREATE_FRAME_IF_NON_WHITESPACE is set, if the flag set while running the inspector, we might face the problem.
How should we do?? Should we more investigate the reason why NS_CREATE_FRAME_IF_NON_WHITESPACE was set?

[1] https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#8273
[2] https://searchfox.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#1904

    
    

    
  
I'm sorry to bother you, Patrick.
How should we proceed this?
Flags: needinfo?(pbrosset)

    
    

    
  
Oh I'm sorry Daisuke, I had not seen your response in comment 47. Thanks for needinfo'ing me. It's easy to lose track of bugs since there are so many.

Honestly, I just don't know enough about how frames are constructed to be able to answer your last questions. I wish I could have said this earlier.

Not sure how to help you progress now. The changes you had done in part 3 looked good to me. But also, I think we should investigate comment 42. It seems like it would simplify a lot of things since we wouldn't have to walk the DOM at all.
Flags: needinfo?(pbrosset)

    
    

    
  


  
Does this work as expected?  We might have to modify InspectorUtils::GetCSSStyleRules too, I am totally unsure though.

    
    

    
  
Thanks, Hiro!
I'll try this!

    
    

    
  
Hiro, the patch works well! Should we introduce this in another bug?

However, because same problem occur even after applying this patch, we should fix to avoid regeneration, anyway.

    
    

    
  
I should have noted about the patch that exposes pseudo element in KeyframeEffect.  It works in most cases as Daisuke commented in 52, but the pseudo element is no longer valid once reframe happens for the element (and one of its ancestors).  To fix that we need to fix our frame construction for pseudo element.

Another thing I noticed about pseudo element on devtools is that devtools doesn't show parent element on animation inspector, it just shows '::before' or '::after'.  It would be nice to show something like 'div#parent::before'.

    
    

    
  
(In reply to Hiroyuki Ikezoe (:hiro) from comment #53)
> I should have noted about the patch that exposes pseudo element in
> KeyframeEffect.  It works in most cases as Daisuke commented in 52, but the
> pseudo element is no longer valid once reframe happens for the element (and
> one of its ancestors).  To fix that we need to fix our frame construction
> for pseudo element.

Thanks, Hiro!
Yeah, it seems this problem would resolve if bug 1419651 fixed.

On one hand, I got an advice of another approach from Brian.
That is, the actor refers the CSSPseudoElement which got by animation.effect.target. Then, if necessary, get the generated pseudo element via walker from its parent. In this case, we can get valid element, even if the pseudo element regenerated. I'm thinking I try this till bug 1419651 was fixed.

> Another thing I noticed about pseudo element on devtools is that devtools
> doesn't show parent element on animation inspector, it just shows '::before'
> or '::after'.  It would be nice to show something like 'div#parent::before'.

Yes, I agree with you!
But, could not fix well, as I commented in comment 25.
However, we may be able resolve, if we introduce the Brian's suggestion.

    
    

    
  
I am trying new way with Hiro's patch, It works very well in my local environment.
That does not keep the node reference, just returns animation.effect.target or animation.effect.pseudoTarget directly when needed. (Originally, we kept the node reference in "_node" variable. But, after regenerated, that reference will be invalid.)

Also, I threw the update to try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e23ae5710b631f413aea6bebd9a8e3859a9b76d
If the try is green, I'd like to ask to Hiro to let him to submit the review for his patch.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8966097 -
        Attachment is obsolete: true

    
    

    
  
Hi Patrick, 
As I commented in comment 55, change the logic a bit.
It is depended on Hiro's patch, could you take a look again especially patch 2??
https://reviewboard.mozilla.org/r/233572/diff/4#index_header
Flags: needinfo?(pbrosset)

    
    

    
  
I think you can put the pseudo element target patch along with other patches in this bug.  Brian is the right person to review it.

    
    

    
  
Thanks, Hiro!
Okay, I'll do if Patrick think no problem about this approach.

    
    

    
  
Comment on attachment 8964828 [details]
Bug 1450526 - Part 2: Change the target node of animation on pseudo element in the actor to proper node.

https://reviewboard.mozilla.org/r/233572/#review249546

Great! I love it when we are able to simplify complex logic.

        Attachment #8964828 -
        Flags: review+
Flags: needinfo?(pbrosset)
Status: NEW → ASSIGNED
Priority: -- → P3

    
    

    
  
(In reply to Patrick Brosset <:pbro> from comment #63)
> Comment on attachment 8964828 [details]
> Bug 1450526 - Part 2: Change the target node of animation on pseudo element
> in the actor to proper node.
> 
> https://reviewboard.mozilla.org/r/233572/#review249546
> 
> Great! I love it when we are able to simplify complex logic.

Thanks! I'll proceed this approach!

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8975725 [details]
Bug 1450526 - Part 2: Expose the generated pseudo element itself in KeyframeEffect.

https://reviewboard.mozilla.org/r/243952/#review250176

Two questions:

* What is the performance cost of using DocumentWalker?
* Why add this to KeyframeEffect instead of CSSPseudoElement?

::: commit-message-f8501:3
(Diff revision 1)
> +The animation inspector so far, retrieved the pseudo element which is actually
> +displaying not CSSPseudoElement using DocumentWalker[1], kept the reference,
> +had used that. However, if the pseudo element was re-generated, the reference
> +will be invalid. So as to resolve this, though retrieve the pseudo element if
> +needed, using DocumentWalker takes high cost. Therefor, add a method which get
> +pseudo element directly into KeyframeEfect, use this instead.

"Since generated content elements may be regenerated during layout, for animations on pseudo elements the animation inspector tracks the parent element and pseudo type. In order to highlight the pseudo element, however, it needs to refer to the generated content element. Navigating from the parent element to the generated content element via inIDeepTreeWalker, however, is expensive so this patch adds a member to KeyframeEffect to access the generated content element."

::: dom/animation/KeyframeEffect.h:171
(Diff revision 1)
>    // updating style, we should pass the ComputedStyle into this method and use
>    // that to update the properties rather than calling
>    // GetComputedStyle.
>    void SetTarget(const Nullable<ElementOrCSSPseudoElement>& aTarget);
>  
> +  Element* GetPseudoTarget() const;

Let's add a comment here:

"Chrome-only accessor to allow DevTools to get to the generated content element (if any) without having to go through DocumentWalker."

::: dom/webidl/KeyframeEffect.webidl:57
(Diff revision 1)
>    required sequence<AnimationPropertyValueDetails> values;
>  };
>  
>  partial interface KeyframeEffect {
>    [ChromeOnly, Throws] sequence<AnimationPropertyDetails> getProperties();
> +  [ChromeOnly] readonly attribute Element? pseudoTarget;

This should probably have a comment something like:

// Get the generated content element if this effect targets a pseudo-element. This element may become invalid any time layout is flushed.

(bz can probably provide a more accurate description than "invalid" here.)

    
    

    
  
Also, part 2 will need review from a DOM peer.

    
    

    
  
Comment on attachment 8975725 [details]
Bug 1450526 - Part 2: Expose the generated pseudo element itself in KeyframeEffect.

https://reviewboard.mozilla.org/r/243952/#review250886

Clearing review request for now since (a) it seems like this member should go on CSSPseudoElement and (b) Daisuke mentioned that he was trying another approach using TreeWalker.

        Attachment #8975725 -
        Flags: review?(bbirtles)

    
    

    
      
  


  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  

        Attachment #8975725 -
        Attachment is obsolete: true

    
    

    
  
Thanks Brian.

At first, the original code of server might cause flushing styles while walking the document. So as to avoid this, I wanted to use the Hiro's approach. However I tried same approach of comment 23, it doesn not flush.
Actually, though Hiro's patch would be better performance, we can choose this way since this function is not called not so frequently, I think.

Patrick, sorry again and again.
How do you think this approach?
https://reviewboard.mozilla.org/r/233572/diff/6#index_header

Also, because the node actor of pseudo element will re-generate as well, need to update in client side.
Could you review this patch as well?

    
    

    
  
Comment on attachment 8979166 [details]
Bug 1450526 - Part 3: Update node front when the indicated node was re-generated.

https://reviewboard.mozilla.org/r/245420/#review251326

::: devtools/client/inspector/animation/animation.js:248
(Diff revision 1)
>    }
>  
>    getNodeFromActor(actorID) {
> +    if (!this.inspector) {
> +      // Already destroyed.
> +      return null;

It's better if all branches of a function return the same type. So in this case a promise.
So this should be changed to `return new Promise.resolve(null)` or maybe `reject`? Not sure what's best in this context. But at least, this way, callers of the `getNodeFromActor` function can always assume that the return value is a promise.

        Attachment #8979166 -
        Flags: review?(pbrosset) → review+

    
    

    
  
(In reply to Patrick Brosset <:pbro> from comment #80)
> Comment on attachment 8979166 [details]
> Bug 1450526 - Part 3: Update node front when the indicated node was
> re-generated.
> 
> https://reviewboard.mozilla.org/r/245420/#review251326
> 
> ::: devtools/client/inspector/animation/animation.js:248
> (Diff revision 1)
> >    }
> >  
> >    getNodeFromActor(actorID) {
> > +    if (!this.inspector) {
> > +      // Already destroyed.
> > +      return null;
> 
> It's better if all branches of a function return the same type. So in this
> case a promise.
> So this should be changed to `return new Promise.resolve(null)` or maybe
> `reject`? Not sure what's best in this context. But at least, this way,
> callers of the `getNodeFromActor` function can always assume that the return
> value is a promise.

I'll use Promise.reject(), thanks!

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9458e4e940db
Part 1: Make pseudo element to be available in new animation inspector. r=pbro
https://hg.mozilla.org/integration/autoland/rev/41501920b211
Part 2: Change the target node of animation on pseudo element in the actor to proper node. r=pbro
https://hg.mozilla.org/integration/autoland/rev/59a04541da1a
Part 3: Update node front when the indicated node was re-generated. r=pbro
https://hg.mozilla.org/integration/autoland/rev/9d46d003fec8
Part 4: Show pseudo element in Reps component. r=pbro
https://hg.mozilla.org/integration/autoland/rev/e0a113c3da05
Part 5: Add test for pseudo element. r=pbro

    
  
Product: Firefox → DevTools

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: