Closed Bug 1401128 Opened 7 years ago Closed 7 years ago

Can't easily click on the first animation in the animation inspector

Categories

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

Product:
DevTools
Component:
Inspector: Animations
Type:
defect

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: pbro, Assigned: daisuke)

Details

Attachments

(3 files)

cant-click-on-first-animation.gif
3.73 MB, image/gif
Details
Bug 1401128 - Part 1: Change the size of time-body.
59 bytes, text/x-review-board-request
pbro
: review+
Details
Bug 1401128 - Part 2: Modify test to change the way to send mouse event.
59 bytes, text/x-review-board-request
pbro
: review+
Details 
STR:

- open http://msuja.ws/svg.html
- open the inspector and switch to the animation tab in the sidebar
- try clicking on animations in the list

Expected:
You can click on any of the animation and have its details displayed in the panel below

Actual: 
You can click on any animation but the first. The cursor isn't a pointer when hovering over the first animation, and clicking doesn't do anything. Also, the tooltip does not appear on hover.

More details:
In fact, all of the above works if you move your mouse to the few last pixels at the bottom of the first animation.
It's like there's an invisible element covering most of the animation and preventing pointer events from reaching the animation, except at the very bottom.
Looks like a pointer-events:none; on the .time-body.track-container element works. This element is above the animations, and needs to remain there, so the vertical lines are visible, but it doesn't need to respond to events I believe, and should therefore let them through.

Daisuke: do you think setting pointer-events:none on this element could break something?
status-firefox57: --- → fix-optional
Flags: needinfo?(dakatsuka)
Priority: -- → P1
(In reply to Patrick Brosset <:pbro> from comment #1)
> Looks like a pointer-events:none; on the .time-body.track-container element
> works. This element is above the animations, and needs to remain there, so
> the vertical lines are visible, but it doesn't need to respond to events I
> believe, and should therefore let them through.
> 
> Daisuke: do you think setting pointer-events:none on this element could
> break something?

Oh, I could reproduce it,, I take a look.
Oh, failed the try.. I'll take a look again.
Comment on attachment 8910095 [details]
Bug 1401128 - Part 2: Modify test to change the way to send mouse event.

https://reviewboard.mozilla.org/r/181574/#review187002

::: devtools/client/animationinspector/components/animation-timeline.js:319
(Diff revision 2)
>      }, TIMELINE_BACKGROUND_RESIZE_DEBOUNCE_TIMER);
>    },
>  
>    onAnimationSelected: Task.async(function* (animation) {
>      let index = this.animations.indexOf(animation);
> +    console.log("onAnimationSelected:"+index);

Please remove this leftover console statement.

::: devtools/client/animationinspector/components/animation-timeline.js:332
(Diff revision 2)
>        const animationEl = animationEls[i];
>        if (!animationEl.classList.contains("selected")) {
>          continue;
>        }
>        if (i === index) {
> +        console.log("emit animation-already-selected");

here too

::: devtools/client/animationinspector/components/animation-timeline.js:521
(Diff revision 2)
>        yield this.onAnimationSelected(this.selectedAnimation);
>      } else {
>        // Otherwise, close detail pane.
>        this.onDetailCloseButtonClick();
>      }
> +    console.log("animation-timeline-rendering-completed");

and here

::: devtools/client/animationinspector/test/head.js:444
(Diff revision 2)
>                                           : "animation-selected");
>  
>    info("Click on animation " + index + " in the timeline");
>    let timeBlock = timeline.rootWrapperEl.querySelectorAll(".time-block")[index];
> -  EventUtils.sendMouseEvent({type: "click"}, timeBlock,
> -                            timeBlock.ownerDocument.defaultView);
> +  let timeBlockBounds = timeBlock.getBoundingClientRect();
> +  let x = timeBlockBounds.x + timeBlockBounds.width / 2 + 5;

Why do we need the +5 here? If it's really needed, please add a comment that explains why.

::: devtools/client/animationinspector/test/head.js:447
(Diff revision 2)
> +  let utils =
> +    timeBlock.ownerDocument.defaultView.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                       .getInterface(Ci.nsIDOMWindowUtils);
> +  utils.sendMouseEventToWindow("mousedown", x, y, 0, 1, 0, false, 0, 0);
> +  utils.sendMouseEventToWindow("mouseup", x, y, 0, 1, 0, false, 0, 0);

Can't you use EventUtils.synthesizeMouse instead of having to get windowUtils here? synthesizeMouse accept x, y coordinates too.
Attachment #8910095 - Flags: review?(pbrosset)
Comment on attachment 8910094 [details]
Bug 1401128 - Part 1: Change the size of time-body.

https://reviewboard.mozilla.org/r/181572/#review187006

::: devtools/client/themes/animationinspector.css:267
(Diff revision 1)
>  .animation-detail .track-container {
>    height: var(--detail-animation-height);
>  }
>  
> +.time-body.track-container {
> +  height: 0;

Oh good, that works too!
By the way, maybe this is useless:

In this rule:
.progress-tick-container .progress-tick, .animation-timeline .time-body .time-tick

we do height: 100%

but then, the ::before pseudo-elements inside the time-ticks actually have height:100vh, so setting 100% height on the time-tick doesn't seem like it's needed.
Attachment #8910094 - Flags: review?(pbrosset) → review+
Comment on attachment 8910095 [details]
Bug 1401128 - Part 2: Modify test to change the way to send mouse event.

https://reviewboard.mozilla.org/r/181574/#review187002

> Please remove this leftover console statement.

Ohhh, I'm so sorry!

> Why do we need the +5 here? If it's really needed, please add a comment that explains why.

Yeah, since this is center of the time line block, so sent the event to time-tick line.
But I will append pointer-events: none; to where I modified in patch 1.

> Can't you use EventUtils.synthesizeMouse instead of having to get windowUtils here? synthesizeMouse accept x, y coordinates too.

Thanks, I'll use that!
Assignee: nobody → dakatsuka
Comment on attachment 8910095 [details]
Bug 1401128 - Part 2: Modify test to change the way to send mouse event.

https://reviewboard.mozilla.org/r/181574/#review187130
Attachment #8910095 - Flags: review?(pbrosset) → review+
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/01ff67e4eeb8
Part 1: Change the size of time-body. r=pbro
https://hg.mozilla.org/integration/autoland/rev/bac4881560bf
Part 2: Modify test to change the way to send mouse event. r=pbro
I have reproduced the bug according to (2017-09-19)

Fixing bug is verified on Latest Beta--
Build ID    :20171002181526
User Agent  :Mozilla/5.0 (Windows NT 6.1; rv:57.0) Gecko/20100101 Firefox/57.0

Tested OS-- Windows7 32bit
QA Whiteboard: [bugday-20170927]
QA Whiteboard: [bugday-20170927] → [bugday-20171004]
(In reply to Saheda Reza [:Antora] from comment #18)
> I have reproduced the bug according to (2017-09-19)
> 
> Fixing bug is verified on Latest Beta--
> Build ID    :20171002181526
> User Agent  :Mozilla/5.0 (Windows NT 6.1; rv:57.0) Gecko/20100101
> Firefox/57.0
> 
> Tested OS-- Windows7 32bit

Thank you for the report!
I'll take a look.
Hi Antora,

I have tested using Beta ( 57.0b5 ) on Windows 7 ( but 64 bit ).
However, unfortunately, I could not reproduce it..
Could you inform me how did you reproduce it since the problem which you got might be new bug.

Thanks!
Flags: needinfo?(saheda.antora)
Hey Daisuke,
(In reply to Daisuke Akatsuka (:daisuke) from comment #19)
> (In reply to Saheda Reza [:Antora] from comment #18)
> > I have reproduced the bug according to (2017-09-19)
The tester meant the Buggy Nightly build of 2017-09-19 
> > Fixing bug is verified on Latest Beta--
> > Build ID    :20171002181526
> > User Agent  :Mozilla/5.0 (Windows NT 6.1; rv:57.0) Gecko/20100101
> > Firefox/57.0
And the fix is verified on the above beta.
Which means There is no new bug and the fix is verified for this bug.
You can change the status to VERIFIED FIXED if you want.
Cheers !
Flags: needinfo?(saheda.antora)
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: