1222937 - Show a dedicated error message in the animation-inspector when an animated pseudo-element is selected

Status: RESOLVED FIXED

(DevTools :: Inspector: Animations, defect)

Description

[Patrick Brosset <:pbro>]

Bug 1206420 is still not resolved, and it's going to take a while before it is.
So in the meantime, instead of showing the "no animations" message when pseudos are selected, we should show a specific message like "animations on pseudo-elements aren't supported yet". Maybe we can detect there's an animation by looking at the styles and only show the message then.

Comment 1

[Sanyam Khurana [:CuriousLearner]]

Hello,

I would like to take this up. Please let me know how to proceed. I've the build ready.

Comment 2

[Patrick Brosset <:pbro>]

The first step for this bug is to alter the way the animation-panel reacts to new nodes being selected in the inspector.

There's an event callback in devtools/client/animationinspector/animation-controller.js: onNewNodeFront that gets called whenever a new node is selected.

In this callback, you can access gInspector.selection.nodeFront to know what is the currently selected node.
You can then use the NodeFront's properties and methods determine if this node is a pseudo element or not. In particular, you could do nodeFront.isPseudoElement to know whether it's a pseudo element.

Using this information, in onNewNodeFront, you should display a message in the animation-panel that says something like: "animated pseudo elements aren't supported yet in the timeline, please select another element".
For info, we have an info message already in the panel with id "error-message" (see animation-inspector.xhtml).
This message is shown when the currently selected node isn't animated. So, I think the right course of action is to show this same message panel but update the label on it.
Right now, the label displayed in this panel comes from here: devtools\client\locales\en-US\animationinspector.dtd

I hope this is enough to get started. Let me know how you get along. Don't hesitate to ask questions on irc #devtools.

Comment 3

[Sanyam Khurana [:CuriousLearner]]

Attachment: bug-1222937-fix.patch

Hi,

As discussed over IRC, during debugging, either selection of pseudo or non-pseudo animated elements don't reach inside either of the if conditions. Earlier, the console was showing error on this line:

if (gInspector.selection.nodeFront.isPseudoElement)

related to chaining which demanded that there should be return. (I don't remember the exact error) and now it's not being shown anymore in console.

Here I'm attaching a patch of whatever I've done till now. Kindly have a look. Thanks.

Comment 4

[Patrick Brosset <:pbro>]

Comment on attachment 8716260 [details] [diff] [review]
bug-1222937-fix.patch

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

I couldn't reproduce the problem we discussed about on IRC today. Your patch worked almost fine for me. So I did a quick review.
I'm suggesting below a change of approach, but essentially, it'll be the same sort of code in the end.

::: devtools/client/animationinspector/animation-controller.js
@@ +221,5 @@
>      }
>  
>      let done = gInspector.updating("animationscontroller");
>  
> +    document.getElementById("error-type").textContent = L10N.getStr("invalidElement")

The role of the animation-controller.js is to serve as a way for the animation panel to access data. It shouldn't deal with updating the UI. Updating the UI is the role of animation-panel.js.

So, while this seems to work fine, I suggest refactoring this code a little bit.
Instead of getting the error-type element here and setting its text content, please do this instead:

Remove the code you added, but just add one more condition to the if (!gInspector....) block, to be like this:

if (!gInspector.selection.isConnected() ||
    !gInspector.selection.isElementNode() ||
    gInspector.selection.isPseudoElementNode()) {
  // ... same as before 
}

This way, the controller stays simple, and just emits the PLAYERS_UPDATED_EVENT a before whenever a node is either not an element or is a pseudo-element.

Then, in animation-panel.js, in the refreshAnimationsUI method, there's already some code to show the error message. The way this happens is that when there are no animations to be shown, refreshAniationsUI then calls this.togglePlayers(false)
If you look at togglePlayers, you'll see that this does this:

      document.body.setAttribute("empty", "true");
      document.body.removeAttribute("timeline");

Adding the "empty" attribute on the body has the consequence of showing the error message (you'll see this in animationinspector.css).
So, I suggest doing this in togglePlayers, when isVisible is false:

$("#error-type").textContent = gInspector.selection.isPseudoElementNode()
                               ? L10N.getStr("pseudoElement")
                               : L10N.getStr("invalidElement");

Comment 5

[Sanyam Khurana [:CuriousLearner]]

Attachment: bug-1222937-fix-v2.patch

Although I've made the above said changes, it's showing the message corresponding to animated pseudo elements for every element as we discussed over IRC.

It crashes the build sometimes running browser console. I've tried to debug this, but till now haven't really found anything except what we discussed last over IRC.

I'm attaching here the latest patch that was made for this bug.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8716918 [details] [diff] [review]
bug-1222937-fix-v2.patch

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

You got pretty close. You only got confused by the isPseudoElementNode thing.
There are 2 similar things you can call to know if the current selection is a pseudo-element:

1) gInspector.selection.isPseudoElementNode()
2) gInspector.selection.nodeFront.isPseudoElement

In fact (1) just calls (2). The difference is that (1) is a function, so you *must* call it with () at the end, while (2) is a property, so you just access it with its name.
Looking at the source code makes this pretty clear:

1) https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/selection.js#269
2) https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/inspector.js#917

I will upload a corrected patch in a minute.
Now, we only miss one thing for this patch to be complete, and that is the test.
We must add a new test for each new feature we add, so that if someone changes the code later on, they know if they broke something, just by running the tests.
It turns out that, luckily, we already have a test for checking that the message appears on invalid nodes. This test is:
\devtools\client\animationinspector\test\browser_animation_empty_on_invalid_nodes.js

Therefore, my suggestion is to add one more case to this test that will select an animated pseudo-element and check that the new message appears.

Please read through the test file, the logic should be pretty straighforward. The test logic is in add_task. What the test does is, adding a new tab in the browser with the page doc_simple_animation.html in it. It then opens the animation inspector and tests selecting various nodes in the page to make sure the animation inspector shows the right thing.

To run this test locally: make sure you built first, and then enter './mach test devtools/client/animationinspector/test/browser_animation_empty_on_invalid_nodes.js'
2)

Comment 7

[Patrick Brosset <:pbro>]

Attachment: Bug_1222937__Showing_dedicated_error_message_for_a.diff

Comment 8

[Sanyam Khurana [:CuriousLearner]]

Hi pbro,

I applied the patch you posted, but as we discussed last over IRC, I'm unable to reproduce the changes manually testing the patch when I select any pseudo animated elements. Can you please check if it works for you? I tried over many different examples, but now I'm stuck because this doesn't seem to work on my build.

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8717396 [details] [diff] [review]
Bug_1222937__Showing_dedicated_error_message_for_a.diff

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

::: devtools/client/animationinspector/animation-controller.js
@@ +240,5 @@
>        done();
>        return;
>      }
>  
>      this.nodeFront = gInspector.selection.nodeFront;

Ah, this needs to be done before the `if` otherwise when a pseudo element is selected, then `this.nodeFront` won't be changed and this will make the next selection early return.
So, please move this assignment to just after the first `if` in this function.

With this fixed, I have no problems with the patch applied. Everything seems to work fine for me.

Comment 10

[Patrick Brosset <:pbro>]

Attachment: pseudo-anim.html

I used this to test the patch.
You can open this HTML page and then try to select <head> (the normal error message should be shown), or <div> (the animation should be shown), or ::before (the pseudo error message should be shown).

Comment 11

[Sanyam Khurana [:CuriousLearner]]

Attachment: bug-1222937-fix-v3.patch

Hi pbro,

I'm attaching another patch here. Please let me know your thoughts.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8726372 [details] [diff] [review]
bug-1222937-fix-v3.patch

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

::: devtools/client/animationinspector/test/browser_animation_empty_on_invalid_nodes.js
@@ +40,5 @@
> +
> +  info("Select the pseudo element node and check that the panel is empty")
> +  let pseudoAnimatedNode = yield getNodeFront(".pseudo", inspector);
> +  onUpdated = panel.once(panel.UI_UPDATED_EVENT);
> +  yield selectNode(pseudoAnimatedNode, inspector);

So here you are actually *not* selecting the pseudo-element node, but its parent (the <div class"pseudo">).
In order to select the pseudo-element, you should do something like this:

let pseudoElParent = yield getNodeFront(".pseudo", inspector);
let {nodes} = yield inspector.walker.children(pseudoElParent);
let pseudoEl = nodes[0];

onUpdated = panel.once(panel.UI_UPDATED_EVENT);
yield selectNode(pseudoEl , inspector);
yield onUpdated;

::: devtools/client/animationinspector/test/doc_simple_animation.html
@@ +99,5 @@
>          margin-right: 600px;
>        }
>      }
> +
> +    .pseudo-ball {

Please move these 2 new css rules before the list of @keyframes rules above.

@@ +102,5 @@
> +
> +    .pseudo-ball {
> +      top: 800px;
> +      left: 10px;
> +      position: relative;

Why relative? Elements with the class "ball" are already absolutely positioned, and I think that's fine here.

@@ +103,5 @@
> +    .pseudo-ball {
> +      top: 800px;
> +      left: 10px;
> +      position: relative;
> +      overflow: hidden;

Also, why overflow hidden?

@@ +107,5 @@
> +      overflow: hidden;
> +    }
> +
> +    .pseudo::before {
> +      content: "";

It would be good to actually animate the pseudo element. Even if in practice it doesn't make any difference with the code in this patch. And in the future when we'll be able to show animations for pseudo elements, we will be able to keep this test.

So maybe add some properties to see the pseudo element and to animate it:

.pseudo::before {
  content: "";
  width: 50%;
  height: 50%;
  background: black;
  position: absolute;
  animation: simple-animation 1s infinite alternate;
}

@@ +123,5 @@
>    <div class="ball short"></div>
>    <div class="ball long"></div>
>    <div class="ball negative-delay"></div>
>    <div class="ball no-compositor"></div>
> +  <div class="ball pseudo-ball pseudo"></div>

No need for both pseudo-ball and pseudo classes. "ball pseudo" would be enough.

Comment 13

[Sanyam Khurana [:CuriousLearner]]

Attachment: bug-1222937-v4.patch

Hi pbro,

You're right about the position: relative and overflow: hidden thing. I was testing things and it remained there. Apologies for that.

I've done the required changes. Also I just moved the element to bottom of the page. Please check and let me know if any further changes are required.

Comment 14

[Patrick Brosset <:pbro>]

Comment on attachment 8727093 [details] [diff] [review]
bug-1222937-v4.patch

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

This looks good thanks.
I just have a few nit comments.
Also, the patch doesn't apply cleanly anymore, because there has been changes to the same files in the meantime. So you need to rebase it.

Because I did rebase it locally in order to test it, I will just attach my patch instead, save you the trouble of doing it (I've also fixed the various nits).

::: devtools/client/animationinspector/test/browser_animation_empty_on_invalid_nodes.js
@@ +31,5 @@
>    let commentNode = yield inspector.walker.previousSibling(stillNode);
>    onUpdated = panel.once(panel.UI_UPDATED_EVENT);
>    yield selectNode(commentNode, inspector);
>    yield onUpdated;
> +  

nit: trailing whitespaces here.

@@ +46,5 @@
> +  yield selectNode(pseudoEl, inspector);
> +  yield onUpdated;
> +
> +  is(panel.animationsTimelineComponent.animations.length, 0,
> +     "No animation players stored in the timeline component for a pseudo Animated Node");

nit: This line (and the similar one below) is more than 80 characters long. We have ESLint rules that check our code for consistent formatting, so this needs to be fixed.
Find out more about that here: https://wiki.mozilla.org/DevTools/CodingStandards

@@ +47,5 @@
> +  yield onUpdated;
> +
> +  is(panel.animationsTimelineComponent.animations.length, 0,
> +     "No animation players stored in the timeline component for a pseudo Animated Node");
> +  is(panel.animationsTimelineComponent.animationsEl.childNodes.length, 0,

It would be good to also test the content of the error message. Because we display a different message whether the node is invalid, or a pseudo-element.

@@ +49,5 @@
> +  is(panel.animationsTimelineComponent.animations.length, 0,
> +     "No animation players stored in the timeline component for a pseudo Animated Node");
> +  is(panel.animationsTimelineComponent.animationsEl.childNodes.length, 0,
> +     "No animation displayed in the timeline component for a pseudo Animated Node");
> +

nit: no need for an empty line here.

::: devtools/client/animationinspector/test/doc_simple_animation.html
@@ +113,5 @@
>        100% {
>          margin-right: 600px;
>        }
>      }
> +

nit: no need for these 2 empty lines.

Comment 15

[Patrick Brosset <:pbro>]

Attachment: Bug_1222937_-_Show_a_dedicated_error_message_for_a.diff

I've pushed this patch to TRY to make sure all tests still pass:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f6003ebb23c

Comment 16

[Sanyam Khurana [:CuriousLearner]]

Hi pbro,

Thanks for this. I'll keep these things in mind from next time positively, and reach you out on IRC for more questions. I'll also go through the ESLint Coding standards. Thanks again.

Comment 17

[Patrick Brosset <:pbro>]

Ok, try is green, I'm pushing this to fx-team: https://hg.mozilla.org/integration/fx-team/rev/3827e69a54ef
Thanks for your help Sanyam! Feel free to ping me/us on IRC for more bugs.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3827e69a54ef