1453246 - If we hover the target element, we should not re-select it

Status: VERIFIED FIXED

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

Description

[Gyula Palko[:gpalko]]

[Environment:]
Windows 8.1, Windows 10, Mac OSX 10.13.2
Nightly 61.0a1 2018-04-04

[Steps:]
1. Open Firefox Nightly and load https://rawgit.com/dadaa/3b73f847427025b51ba1ab7333013d0c/raw/77f3f0bb884875a179c3407f73bf8a8dd54751c9/doc_simple_animation.html.
2. Press F12.
3. Select Inspector.
4. Select Animation tab.
5. Select div.ball.multi from the Inspector
6. Hover over the nodes in Animation panel

[Actual Result:]
The element is re-selected on hover.

[Expected Result:]
Already selected elements should not be re-selected

Comment 1

[Brian Birtles (:birtles)]

Hi Gyula, I'm having a hard time understanding the bug here. The behavior seems to be the same to me with both the old and new animation inspector.

Comment 2

[Gyula Palko[:gpalko]]

(In reply to Brian Birtles (:birtles) from comment #1)
> Hi Gyula, I'm having a hard time understanding the bug here. The behavior
> seems to be the same to me with both the old and new animation inspector.

Hi Brian, 
This issue was pointed to us by Daisuke while we were discussing another issue. The point was that in case in which an element has multiple animations like the example in comment 0, the element should not be re selected because what results is a flicker effect with no additional gain.
This is probably more easily observed if, additionally to comment 0, you add an intermediary step before step 6 of clicking the target on an animation node:

5. Select div.ball.multi from the Inspector
5'. Click the target icon on any of the two animations.
6. Hover over the nodes in Animation panel

Comment 3

[Brian Birtles (:birtles)]

Oh, I see. It seems the new inspector allows locking the target element (whereas the old inspector does not appear to work in this case). Once an element is locked, we don't need to re-highlight it.

Comment 4

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1453246 - Part 1: Disable mouseover/mouseover if the target node already highlighted.

Review commit: https://reviewboard.mozilla.org/r/246346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246346/

Comment 5

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1453246 - Part 2: Add test for avoiding double highlight.

Review commit: https://reviewboard.mozilla.org/r/246348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246348/

Comment 6

[Daisuke Akatsuka (:daisuke)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ea2e5cc58335c2c2f85c61894a5fb2c864f7e18

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8980182 [details]
Bug 1453246 - Part 1: Disable mouseover/mouseover if the target node already highlighted.

https://reviewboard.mozilla.org/r/246346/#review253132

::: devtools/client/inspector/animation/components/AnimationTarget.js:137
(Diff revision 1)
>            defaultRep: ElementNode,
>            mode: MODE.TINY,
>            inspectIconTitle: getInspectorStr("inspector.nodePreview.highlightNodeLabel"),
>            object: translateNodeFrontToGrip(nodeFront),
>            onDOMNodeClick: () => this.select(),
> -          onDOMNodeMouseOut: () => onHideBoxModelHighlighter(),
> +          onDOMNodeMouseOut: isHighlighted ? null : () => onHideBoxModelHighlighter(),

I would prefer if we declared a handler function that did the following:

onDOMNodeMouseOver() {
  if (!isHighlighted) {
    onHideBoxModelHighlighter();
  }
}

::: devtools/client/inspector/animation/components/AnimationTarget.js:138
(Diff revision 1)
>            mode: MODE.TINY,
>            inspectIconTitle: getInspectorStr("inspector.nodePreview.highlightNodeLabel"),
>            object: translateNodeFrontToGrip(nodeFront),
>            onDOMNodeClick: () => this.select(),
> -          onDOMNodeMouseOut: () => onHideBoxModelHighlighter(),
> -          onDOMNodeMouseOver: () => this.highlight(),
> +          onDOMNodeMouseOut: isHighlighted ? null : () => onHideBoxModelHighlighter(),
> +          onDOMNodeMouseOver: isHighlighted ? null : () => this.highlight(),

Same as above.

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8980183 [details]
Bug 1453246 - Part 2: Add test for avoiding double highlight.

https://reviewboard.mozilla.org/r/246348/#review253134

Comment 9

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8980182 [details]
Bug 1453246 - Part 1: Disable mouseover/mouseover if the target node already highlighted.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246346/diff/1-2/

Comment 10

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8980183 [details]
Bug 1453246 - Part 2: Add test for avoiding double highlight.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246348/diff/1-2/

Comment 11

[Daisuke Akatsuka (:daisuke)]

Thanks Gabriel!
I have updated the patches.
Will land if the try was green.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb527a959f14d8bc7a24962fb261304124360e2b

Comment 12

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8980182 [details]
Bug 1453246 - Part 1: Disable mouseover/mouseover if the target node already highlighted.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246346/diff/2-3/

Comment 13

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8980183 [details]
Bug 1453246 - Part 2: Add test for avoiding double highlight.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246348/diff/2-3/

Comment 14

[Daisuke Akatsuka (:daisuke)]

Sorry, I have updated again so as to avoid double highlight in case of clicking the icon after mouse over.
Could you check again?

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f0caff209662ac59220da4d6d29eefdf115362d

Comment 15

[Gabriel Luong [:gl] (ΦωΦ)]

looks fine to me

Comment 16

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8980182 [details]
Bug 1453246 - Part 1: Disable mouseover/mouseover if the target node already highlighted.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246346/diff/3-4/

Comment 17

[Daisuke Akatsuka (:daisuke)]

Comment on attachment 8980183 [details]
Bug 1453246 - Part 2: Add test for avoiding double highlight.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246348/diff/3-4/

Comment 18

[Daisuke Akatsuka (:daisuke)]

Rebased the patches.
Will land if the try was green.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5321ba1a4fd7ac7b5b4435953384b64f65114d3

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1c0e5d523456
Part 1: Disable mouseover/mouseover if the target node already highlighted. r=gl
https://hg.mozilla.org/integration/autoland/rev/2f2884150d18
Part 2: Add test for avoiding double highlight. r=gl

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/1c0e5d523456
https://hg.mozilla.org/mozilla-central/rev/2f2884150d18

Comment 21

[Gyula Palko[:gpalko]]

Verified as fixed on Nightly 62.0a1(20180618220118) on Win10, MacOS 10.12 and Ubuntu 16.04