Closed Bug 1199712 Opened 9 years ago Closed 9 years ago

Pressing space inside the animation panel shifts the contents of the panel down and hides the pause/play button

Categories

(DevTools :: Inspector: Animations, defect)

defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: bgrins, Assigned: george.sanchez, Mentored)

References

Details

(Whiteboard: [good first bug][lang=css][polish-backlog])

Attachments

(5 files, 3 obsolete files)

STR:

Open bgrins.github.io/devtools-demos/inspector/animation-timing.html
Open animations panel and focus the toolbar with the pause/play button
Press space

Expected:

Animations pause

Actual:

Contents of panel shift down and pause/play button disappears.  See screenshot
Patrick, I'm not sure which bug to block this on.  Can you set it?

Also, is there a way I can test with the 'new' animations editor to see if it's still and issue there, or is that already on by default?
Flags: needinfo?(pbrosset)
Let's make this block the animation-inspector v3 bug.

(In reply to Brian Grinstead [:bgrins] from comment #1)
> Also, is there a way I can test with the 'new' animations editor to see if
> it's still and issue there, or is that already on by default?
Looking at the screenshot, it looks like you already have the latest version.

I quickly tested on windows/dev-edition and couldn't reproduce. Maybe this was introduced by one of the recent patches that landed on fx-team/m-c. I'll test there.
Blocks: 1153271
Flags: needinfo?(pbrosset)
Can't reproduce on Windows 10 with today's fx-team either. This might be a Mac-only thing.
Ok, I did reproduce after all. Here are my STR:

Open bgrins.github.io/devtools-demos/inspector/animation-timing.html
Open animations panel
Click in the toolbar that contains the pause/play button, but not on the button itself
Press space
Maybe there is some overflow on a container element that shouldn't be there?
I *think* this is a simple bug to get started, so flagging it as such.
Mentor: pbrosset
Whiteboard: [good first bug][lang=css][polish-backlog]
I was not able to reproduce this on fx-team on Arch Linux.
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #6)
> I *think* this is a simple bug to get started, so flagging it as such.

I would like to get my feet wet in firefox and would like to work on this bug..will you be able to mentor me?
Attached image Animations.PNG
I tried recreating it and the screen shot above is what i get. 
I am on Windows10/Firefox 40.0.3
(In reply to George Sanchez from comment #8)
> I would like to get my feet wet in firefox and would like to work on this
> bug..will you be able to mentor me?
Thanks for your interest! Yes I can mentor you.
Make sure you start with getting the environment ready (unless you've done so before):
https://wiki.mozilla.org/DevTools/Hacking
And make sure you know how to generate and submit patches:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Once that's done, you'll need some pointers to the code:

- The front-end code that handles the animation panel UI is located in
/browser/devtools/animationinspector/animation-panel.js
(there are also other UI widgets/components in /browser/devtools/animationinspector/components.js)
There's a corresponding HTML file too:
/browser/devtools/animationinspector/animation-inspector.xhtml
The toolbar mentioned in comment 4 is the element with id toolbar in this file

- The CSS that goes with this:
/browser/themes/shared/devtools/animationinspector.css

As for the problem itself:
I guess there may be ways to avoid automatically scrolling the container when space is paused, but I have never looked into that myself. Please do start to investigate and come back with questions if you need to. I can look at how to solve this in more details if needed.
(In reply to George Sanchez from comment #9)
> Created attachment 8656014 [details]
> Animations.PNG
> 
> I tried recreating it and the screen shot above is what i get. 
> I am on Windows10/Firefox 40.0.3
When Brian filed the bug, he had the pref devtools.inspector.animationInspectorV3 turned ON (in about:config), so you should try with this too, it might be related (this pref enables a new version of the UI).
Also make sure you use a recent build (if you follow my instructions in comment 10, you'll have a local build of the code from the  fx-team repo, that should do it).
Once this is in place, click on the toolbar, then press space, you should be able to reproduce the issue.
Attached image press-space.gif
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #4)
> Ok, I did reproduce after all. Here are my STR:
> 
> Open bgrins.github.io/devtools-demos/inspector/animation-timing.html
> Open animations panel
> Click in the toolbar that contains the pause/play button, but not on the
> button itself
> Press space

I tried this and the whole box just scrolls down to the middle
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #10)
> (In reply to George Sanchez from comment #8)
> > I would like to get my feet wet in firefox and would like to work on this
> > bug..will you be able to mentor me?
> Thanks for your interest! Yes I can mentor you.
> Make sure you start with getting the environment ready (unless you've done
> so before):
> https://wiki.mozilla.org/DevTools/Hacking
> And make sure you know how to generate and submit patches:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> How_to_Submit_a_Patch
> https://developer.mozilla.org/en-US/docs/Mercurial/
> Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-
> in_for_me.3F
> 
> Once that's done, you'll need some pointers to the code:
> 
> - The front-end code that handles the animation panel UI is located in
> /browser/devtools/animationinspector/animation-panel.js
> (there are also other UI widgets/components in
> /browser/devtools/animationinspector/components.js)
> There's a corresponding HTML file too:
> /browser/devtools/animationinspector/animation-inspector.xhtml
> The toolbar mentioned in comment 4 is the element with id toolbar in this
> file
> 
> - The CSS that goes with this:
> /browser/themes/shared/devtools/animationinspector.css
> 
> As for the problem itself:
> I guess there may be ways to avoid automatically scrolling the container
> when space is paused, but I have never looked into that myself. Please do
> start to investigate and come back with questions if you need to. I can look
> at how to solve this in more details if needed.

Ok will take a look.
Just guessing - maybe there is overflow: hidden set on a container that is overflowing and pressing space is scrolling it?
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #12)
> Created attachment 8656018 [details]
> press-space.gif

Ok Got it .. I was not using the nightly. will try to recreate it.
Thanks!
Attached image animation.PNG
I am able to recreate it now.
No longer blocks: 1153271
Blocks: 1201278
Attached patch animationinspector.patch (obsolete) — Splinter Review
Attached is the css changes for this. Inherited the scroll-y from the body and kept scroll-x hidden.
Pressing space still scrolls down but this is the same behavior with Firefox version- I guess this is a browser thing.

Let me know what you think!
Attachment #8656957 - Flags: ui-review+
Attachment #8656957 - Flags: review+
Attachment #8656957 - Flags: feedback+
Attachment #8656957 - Flags: checkin+
Comment on attachment 8656957 [details] [diff] [review]
animationinspector.patch

Thanks for the patch, I'll a look at it soon.
I'm just going to reset some of the flags you've set.
Typically, when you're ready to get your code review, you just need to set the R flag to ? as explained here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Submitting_the_patch
Attachment #8656957 - Flags: ui-review+
Attachment #8656957 - Flags: review?(pbrosset)
Attachment #8656957 - Flags: review+
Attachment #8656957 - Flags: feedback+
Attachment #8656957 - Flags: checkin+
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #19)
> Comment on attachment 8656957 [details] [diff] [review]
> animationinspector.patch
> 
> Thanks for the patch, I'll a look at it soon.
> I'm just going to reset some of the flags you've set.
> Typically, when you're ready to get your code review, you just need to set
> the R flag to ? as explained here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> How_to_Submit_a_Patch#Submitting_the_patch

Ok. Will take a note of that.
(In reply to George Sanchez from comment #18)
> Created attachment 8656957 [details] [diff] [review]
> animationinspector.patch
> 
> Attached is the css changes for this. Inherited the scroll-y from the body
> and kept scroll-x hidden.
> Pressing space still scrolls down but this is the same behavior with Firefox
> version- I guess this is a browser thing.
Hm, what do you mean by "this is a browser thing"?
We need the patch to make sure space does not scroll down the container anymore, so in that respect, the patch is incorrect for now.
I suggest using the Browser Toolbox [1] to inspect the HTML and CSS structure of the animation panel.
I suspect there is something wrong with the way the scrubber element (the vertical red line) is displayed. Maybe it overflows the container somehow and pressing space forces the iframe to scroll down. Maybe just making sure the scrubber element's parent is positioned would fix it.

[1] https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox
Attachment #8656957 - Flags: review?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #21)
> (In reply to George Sanchez from comment #18)
> > Created attachment 8656957 [details] [diff] [review]
> > animationinspector.patch
> > 
> > Attached is the css changes for this. Inherited the scroll-y from the body
> > and kept scroll-x hidden.
> > Pressing space still scrolls down but this is the same behavior with Firefox
> > version- I guess this is a browser thing.
> Hm, what do you mean by "this is a browser thing"?
> We need the patch to make sure space does not scroll down the container
> anymore, so in that respect, the patch is incorrect for now.
> I suggest using the Browser Toolbox [1] to inspect the HTML and CSS
> structure of the animation panel.
> I suspect there is something wrong with the way the scrubber element (the
> vertical red line) is displayed. Maybe it overflows the container somehow
> and pressing space forces the iframe to scroll down. Maybe just making sure
> the scrubber element's parent is positioned would fix it.
> 
> [1] https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

Ok will take a look at the scrubber element
Attached patch animations.patch (obsolete) — Splinter Review
Reverted back the overflow inherit on animations. Made the scrubber element the same size as the #players.
Attachment #8658795 - Flags: review?(pbrosset)
Comment on attachment 8658795 [details] [diff] [review]
animations.patch

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

I've made some comments about the fix you made.
But there's a more important problem with this patch: it doesn't apply cleanly on fx-team. It looks like it comes from a commit that you made on top of the first commit you made earlier (with the overflow approach).
If you're using MQ (mercurial queues), you should fold the 2 patches into one. In fact, when you do changes on top of a patch in MQ, you can do 'hg qref' to refresh this patch with the new changes.
If you're using Mercurial alone, then you can use the histedit extension to fold the 2 commits into one.

::: browser/themes/shared/devtools/animationinspector.css
@@ +137,5 @@
>  }
>  
>  .animation-timeline .scrubber {
>    position: absolute;
> +  height: calc(100% - 20px);

I've recently introduced a css variable to replace the hard-coded 20px value in several places in this css file. So could you please change this to:

height: calc(100% - var(--toolbar-height));

Although this looks like the right solution, it still scrolls down by 1px when I press space. I've tested this locally on mac.
So the problem is mostly gone, but we should really try to make sure it doesn't scroll at all.

Of course we could do:
calc(100% - var(--toolbar-height) - 1px)
but this looks like an ugly fix.

In fact, keeping height:100% and adding position:relative to .animation-timeline {} instead seems to work fine. Can you try this?
Attachment #8658795 - Flags: review?(pbrosset)
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #24)
> Comment on attachment 8658795 [details] [diff] [review]
> animations.patch
> 
> Review of attachment 8658795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've made some comments about the fix you made.
> But there's a more important problem with this patch: it doesn't apply
> cleanly on fx-team. It looks like it comes from a commit that you made on
> top of the first commit you made earlier (with the overflow approach).
> If you're using MQ (mercurial queues), you should fold the 2 patches into
> one. In fact, when you do changes on top of a patch in MQ, you can do 'hg
> qref' to refresh this patch with the new changes.
> If you're using Mercurial alone, then you can use the histedit extension to
> fold the 2 commits into one.
> 
> ::: browser/themes/shared/devtools/animationinspector.css
> @@ +137,5 @@
> >  }
> >  
> >  .animation-timeline .scrubber {
> >    position: absolute;
> > +  height: calc(100% - 20px);
> 
> I've recently introduced a css variable to replace the hard-coded 20px value
> in several places in this css file. So could you please change this to:
> 
> height: calc(100% - var(--toolbar-height));
> 
> Although this looks like the right solution, it still scrolls down by 1px
> when I press space. I've tested this locally on mac.
> So the problem is mostly gone, but we should really try to make sure it
> doesn't scroll at all.
> 
> Of course we could do:
> calc(100% - var(--toolbar-height) - 1px)
> but this looks like an ugly fix.
> 
> In fact, keeping height:100% and adding position:relative to
> .animation-timeline {} instead seems to work fine. Can you try this?

Ok will try position relative.
Also, this will still scroll down if you select one of the timeline and press space.

As for the patch, I did not commit the first patch locally. I pulled latest and saw that it has the changes in my patch.
Attached patch animationinspectorv2.patch (obsolete) — Splinter Review
Added the toolbar-height variable and position relative in .animation-timeline
Attachment #8660396 - Flags: review?(pbrosset)
Comment on attachment 8656957 [details] [diff] [review]
animationinspector.patch

Marking older patches as obsolete.
Attachment #8656957 - Attachment is obsolete: true
Comment on attachment 8658795 [details] [diff] [review]
animations.patch

Marking older patches as obsolete.
Comment on attachment 8658795 [details] [diff] [review]
animations.patch

Marking older patches as obsolete.
Attachment #8658795 - Attachment is obsolete: true
(In reply to George Sanchez from comment #25) 
> As for the patch, I did not commit the first patch locally. I pulled latest
> and saw that it has the changes in my patch.
There might be something wrong with your repo, I still can't apply the last patch you uploaded locally. I looks to me like you probably committed your first patch locally and have built the newer one on top of it.
Make sure you unapply all patches, then pull the latest, and then only re-apply them (not sure if you're using mercurial MQ or just mercurial alone, don't hesitate to ask questions in the #introduction channel of the IRC Mozilla server if you need more help with this).
I'll upload a cleaned-up patch that should apply well in a second.
Here's your patch cleaned-up, it applies ok on the repo now.
Anyway, the fix looks good, so I'm R+'ing this.
Attachment #8660396 - Attachment is obsolete: true
Attachment #8660396 - Flags: review?(pbrosset)
Attachment #8662243 - Flags: review+
Assignee: nobody → george.sanchez
Pending try build (to make sure tests still pass with this patch applied, but it should be fine, this is only a very minor css change): https://treeherder.mozilla.org/#/jobs?repo=try&revision=419d359fb9d2
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #30)
> (In reply to George Sanchez from comment #25) 
> > As for the patch, I did not commit the first patch locally. I pulled latest
> > and saw that it has the changes in my patch.
> There might be something wrong with your repo, I still can't apply the last
> patch you uploaded locally. I looks to me like you probably committed your
> first patch locally and have built the newer one on top of it.
> Make sure you unapply all patches, then pull the latest, and then only
> re-apply them (not sure if you're using mercurial MQ or just mercurial
> alone, don't hesitate to ask questions in the #introduction channel of the
> IRC Mozilla server if you need more help with this).
> I'll upload a cleaned-up patch that should apply well in a second.

Ok will check my local repo and ask for help. I think because I am using mozilla-central instead of fx-team. Will switch over to fx-team.
https://hg.mozilla.org/mozilla-central/rev/6e4be9895481
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Component: Developer Tools: Inspector → Developer Tools: Animation Inspector
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.