1270462 - Extract toggleOnHover logic from Tooltip.js

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement, P1)

Description

[Julian Descottes [:jdescottes]]

All the code used to toggle the tooltip when hovering targets can be shared by the HTMLTooltip developed in Bug 1259834 and the existing Tooltip.

The goal of this bug is to extract this to a separated class, and clean it up from privileged APIs.

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: bug1270462.proto.patch

Jarda: Can you take a look at the patch and let me know what you think?

Comment 2

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8749161 [details] [diff] [review]
bug1270462.proto.patch

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

Looks good except one small bug.

::: devtools/client/shared/widgets/tooltip/TooltipToggle.js
@@ +99,5 @@
> +        this.isValidTarget(event.target).then(target => {
> +          if (target) {
> +            this.tooltip.show(target);
> +          }
> +        }, reason => {

Here you introduced a bug - if the _targetNodeCb returns a promise rejected with "false" (meaning "I don't want to display the tooltip here"), you will log it as error, because you removed the check for "false" value from the original code.

@@ +119,5 @@
> +    // false, true or a DOM node to use as target.
> +    let res = yield this._targetNodeCb(target, this.tooltip);
> +
> +    if (!res) {
> +      return false;

This should be "throw false". Then the return value will be the same as if _targetNodeCb returned a promise rejected with "false".

This logic could use some refactoring: if _targetNodeCb always returned a promise, the code would be simpler.

Comment 3

[Julian Descottes [:jdescottes]]

(In reply to Jarda Snajdr [:jsnajdr] from comment #2)
> Comment on attachment 8749161 [details] [diff] [review]
> bug1270462.proto.patch
> 
> Review of attachment 8749161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good except one small bug.
> 

Urgh! Good catch. I assumed the promise was supposed to resolve(false) if the target was invalid, and not reject(false). I don't think a "return false" should translate as a reject(false). We don't have many consumers for this API, so I will try to submit a cleanup as part 2.

Comment 4

[Jarda Snajdr [:jsnajdr]]

(In reply to Julian Descottes [:jdescottes] from comment #3)
> I don't think a "return false" should translate as a reject(false).

Currently, that's how Tooltip.isValidHoverTarget works (see [1]) - a non-promise falsy value from the callback is translated to a reject(false).

[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/widgets/Tooltip.js#465

Of course, this is not a great API and it should be cleaned up so that a promise is always returned from the callback.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1270462 - part1: extract devtools tooltip toggle logic to separate file;r=bgrins,jsnajdr

The code used to make the tooltip appear/disappear when hovering targets
has been extracted to a separate class that can be shared between the
current Tooltip.js implementation and the upcoming HTMLTooltip.

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

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: MozReview Request: Bug 1270462 - part2: tooltip toggle callback can resolve(false);r=bgrins,jsnajdr

Previously, the targetNodeCb used in TooltipToggle had an inconsistent API. If
returning synchronously, "false" would prevent the tooltip from appearing.
However, if using a promise, resolving "false" would still show the tooltip.
It was needed to reject the promise in this case to prevent the tooltip from
being displayed.

This commit makes TooltipToggle always expect a consistent return value from
this callback, whether it is synchronous, or using promises.
- true -> show the tooltip on the event target
- DOM node -> show the tooltip on the provided node
- false (or falsy value) -> do not show the tooltip

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

Comment 7

[Julian Descottes [:jdescottes]]

Brian: As discussed, let me know if you'd prefer to redirect on someone else (probably pbro or miker according to hg annotate for Tooltip.js).

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8749334 [details]
MozReview Request: Bug 1270462 - part2: tooltip toggle callback can resolve(false);r=bgrins,jsnajdr

Jarda: this is the cleanup we discussed on IRC, let me know what you think!

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8749334 [details]
MozReview Request: Bug 1270462 - part2: tooltip toggle callback can resolve(false);r=bgrins,jsnajdr

Cancelling review request for this one as I am getting test failures.

Comment 10

[Julian Descottes [:jdescottes]]

Comment on attachment 8749333 [details]
MozReview Request: Bug 1270462 - part1: extract devtools tooltip toggle logic to separate file;r=bgrins,jsnajdr

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

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8749334 [details]
MozReview Request: Bug 1270462 - part2: tooltip toggle callback can resolve(false);r=bgrins,jsnajdr

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

Comment 12

[Jarda Snajdr [:jsnajdr]]

https://reviewboard.mozilla.org/r/50873/#review47697

::: devtools/client/debugger/views/variable-bubble-view.js:25
(Diff revision 2)
>    this.DebuggerView = DebuggerView;
>  
>    this._onMouseMove = this._onMouseMove.bind(this);
>    this._onMouseOut = this._onMouseOut.bind(this);
>    this._onPopupHiding = this._onPopupHiding.bind(this);
> +  this._tooltipShowDelay = EDITOR_VARIABLE_HOVER_DELAY;

Nit: You're doing this only to make the EDITOR_VARIABLE_HOVER_DELAY value available in the test? Then I'd consider moving both EDITOR_VARIABLE_* constants to the VariableBubbleView.prototype. Avoids the redundant _tooltipShowDelay property.

::: devtools/client/inspector/shared/test/head.js:210
(Diff revision 2)
>   * panels during test runs, the better).
>   *
>   * @return a promise that resolves when the answer is known
>   */
>  function isHoverTooltipTarget(tooltip, target) {
> -  if (!tooltip._basedNode || !tooltip.panel) {
> +  if (!tooltip._toggle._baseNode || !tooltip.panel) {

This code is duplicated in inspector/rules/test/head.js and in inspector/shared/test/head.js. This bug is a great opportunity to move it up to inspector/test/head.js, which is included by both of them.

::: devtools/client/shared/widgets/Tooltip.js:194
(Diff revision 2)
>    }, options);
>    this.panel = PanelFactory.get(doc, this.options);
>  
> -  // Used for namedTimeouts in the mouseover handling
> -  this.uid = "tooltip-" + Date.now();
> +  // Create tooltip toggle helper and decorate the Tooltip instance with
> +  // shortcut methods.
> +  this._toggle = new TooltipToggle(this);

Just an idea - lazy initialization would be nice here, so that the TooltipToggle instance is created only when I really use the feature of the Tooltip. You can use a property getter for this:

get toggle() { this._toggle = ...; return this._toggle; }
this.startToggleOnHover = (...args) => this.toggle.start(...args);

::: devtools/client/shared/widgets/Tooltip.js:197
(Diff revision 2)
> -  // Used for namedTimeouts in the mouseover handling
> -  this.uid = "tooltip-" + Date.now();
> +  // Create tooltip toggle helper and decorate the Tooltip instance with
> +  // shortcut methods.
> +  this._toggle = new TooltipToggle(this);
> +  this.startTogglingOnHover = this._toggle.start.bind(this._toggle);
> +  this.stopTogglingOnHover = this._toggle.stop.bind(this._toggle);
> +  this.isValidHoverTarget = this._toggle.isValidHoverTarget.bind(this._toggle);

isValidHoverTarget doesn't need to be exposed by the Tooltip class, as it's an internal method of the TooltipToggle. The tests that call it can access it through the _toggle property.

::: devtools/client/shared/widgets/tooltip/TooltipToggle.js:11
(Diff revision 2)
> +
> +"use strict";
> +
> +const DEFAULT_SHOW_DELAY = 50;
> +
> +exports.TooltipToggle = function (tooltip) {

Nit: Devtools code usually defines classes like this:
function TooltipToggle() {}
TooltipToggle.prototype = {}
exports.TooltipToggle = TooltipToggle

Then the constructor has the right "name" property, is shown as "TooltipToggle" in variable inspector (not as "exports.TooltipToggle") etc.

Comment 13

[Julian Descottes [:jdescottes]]

Comment on attachment 8749333 [details]
MozReview Request: Bug 1270462 - part1: extract devtools tooltip toggle logic to separate file;r=bgrins,jsnajdr

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

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8749334 [details]
MozReview Request: Bug 1270462 - part2: tooltip toggle callback can resolve(false);r=bgrins,jsnajdr

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

Comment 15

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/50873/#review47697

> Nit: You're doing this only to make the EDITOR_VARIABLE_HOVER_DELAY value available in the test? Then I'd consider moving both EDITOR_VARIABLE_* constants to the VariableBubbleView.prototype. Avoids the redundant _tooltipShowDelay property.

Correct, was only done for the tests.

> This code is duplicated in inspector/rules/test/head.js and in inspector/shared/test/head.js. This bug is a great opportunity to move it up to inspector/test/head.js, which is included by both of them.

Good point, moved it

> Just an idea - lazy initialization would be nice here, so that the TooltipToggle instance is created only when I really use the feature of the Tooltip. You can use a property getter for this:
> 
> get toggle() { this._toggle = ...; return this._toggle; }
> this.startToggleOnHover = (...args) => this.toggle.start(...args);

I don't feel the extra complexity is worth the performance impact here. I think instantiating the TooltipToggle is pretty lightweight.
Feel free to push back if I am missing something.

> isValidHoverTarget doesn't need to be exposed by the Tooltip class, as it's an internal method of the TooltipToggle. The tests that call it can access it through the _toggle property.

Right, no need for this method to be part of the API of tooltip.

> Nit: Devtools code usually defines classes like this:
> function TooltipToggle() {}
> TooltipToggle.prototype = {}
> exports.TooltipToggle = TooltipToggle
> 
> Then the constructor has the right "name" property, is shown as "TooltipToggle" in variable inspector (not as "exports.TooltipToggle") etc.

Thanks!

Comment 16

[Julian Descottes [:jdescottes]]

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=887832186e8f

Comment 17

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8749333 [details]
MozReview Request: Bug 1270462 - part1: extract devtools tooltip toggle logic to separate file;r=bgrins,jsnajdr

https://reviewboard.mozilla.org/r/50873/#review47717

Looks good except one remaining issue with the changed API of isValidHoverTarget. The lazy initialization was just an idea that I wanted to share, no reason to push it.

::: devtools/client/netmonitor/test/browser_net_image-tooltip.js:67
(Diff revision 3)
>      /**
>       * @return a promise that resolves when the tooltip is shown
>       */
>      function showTooltipOn(tooltip, element) {
>        return Task.spawn(function*() {
> -        let isTarget = yield tooltip.isValidHoverTarget(element);
> +        let isTarget = yield tooltip._toggle.isValidHoverTarget(element);

This test seems to rely on the old behavior when a promise.reject(false) was returned by isValidHoverTarget. Then the task threw an exception and the test failed. Now, "false" return value is assigned to isTarget and nobody cares about it. Let's  add an ok(isTarget) test here.

Or even better, throw an exception from this task and then add a catch() handler at the end of the promise chain in the test() function. Then the test will fail more gracefully in case of failure.

Comment 18

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8749334 [details]
MozReview Request: Bug 1270462 - part2: tooltip toggle callback can resolve(false);r=bgrins,jsnajdr

https://reviewboard.mozilla.org/r/50875/#review47703

Part 2 looks good.

Comment 19

[Julian Descottes [:jdescottes]]

Comment on attachment 8749333 [details]
MozReview Request: Bug 1270462 - part1: extract devtools tooltip toggle logic to separate file;r=bgrins,jsnajdr

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

Comment 20

[Julian Descottes [:jdescottes]]

Comment on attachment 8749334 [details]
MozReview Request: Bug 1270462 - part2: tooltip toggle callback can resolve(false);r=bgrins,jsnajdr

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

Comment 21

[Julian Descottes [:jdescottes]]

https://reviewboard.mozilla.org/r/50873/#review47717

> This test seems to rely on the old behavior when a promise.reject(false) was returned by isValidHoverTarget. Then the task threw an exception and the test failed. Now, "false" return value is assigned to isTarget and nobody cares about it. Let's  add an ok(isTarget) test here.
> 
> Or even better, throw an exception from this task and then add a catch() handler at the end of the promise chain in the test() function. Then the test will fail more gracefully in case of failure.

I added the ok() for now, to avoid making the test more complex. Thanks!

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8749333 [details]
MozReview Request: Bug 1270462 - part1: extract devtools tooltip toggle logic to separate file;r=bgrins,jsnajdr

https://reviewboard.mozilla.org/r/50873/#review47767

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8749334 [details]
MozReview Request: Bug 1270462 - part2: tooltip toggle callback can resolve(false);r=bgrins,jsnajdr

https://reviewboard.mozilla.org/r/50875/#review47769

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/fa953819c393
https://hg.mozilla.org/integration/fx-team/rev/91faa576bfe4

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fa953819c393
https://hg.mozilla.org/mozilla-central/rev/91faa576bfe4