Closed Bug 1270462 Opened 6 years ago Closed 6 years ago

Extract toggleOnHover logic from Tooltip.js

Categories

(DevTools :: Shared Components, enhancement, P1)

enhancement

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Iteration:
49.1 - May 9
Tracking Status
firefox49 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

Details

(Whiteboard: [devtools-html])

Attachments

(2 files, 1 obsolete file)

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.
Blocks: 1266448
Severity: normal → enhancement
Flags: qe-verify-
Whiteboard: [devtools-html]
Attached patch bug1270462.proto.patch (obsolete) — Splinter Review
Jarda: Can you take a look at the patch and let me know what you think?
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Attachment #8749161 - Flags: feedback?(jsnajdr)
Whiteboard: [devtools-html] → [devtools-html] [triage]
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.
Attachment #8749161 - Flags: feedback?(jsnajdr) → feedback+
(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.
(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.
Iteration: --- → 49.1 - May 9
Priority: -- → P1
Whiteboard: [devtools-html] [triage] → [devtools-html]
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/
Attachment #8749333 - Flags: review?(bgrinstead)
Attachment #8749334 - Flags: review?(bgrinstead)
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/
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).
Attachment #8749161 - Attachment is obsolete: true
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!
Attachment #8749334 - Flags: review?(jsnajdr)
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.
Attachment #8749334 - Flags: review?(jsnajdr)
Attachment #8749334 - Flags: review?(bgrinstead)
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/
Attachment #8749334 - Flags: review?(bgrinstead)
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/
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 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/
Attachment #8749334 - Attachment description: MozReview Request: Bug 1270462 - part2: toggle tooltip callback now always expected to resolve;r=bgrins → MozReview Request: Bug 1270462 - part2: tooltip toggle callback can resolve(false);r=bgrins
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/
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!
Attachment #8749333 - Flags: review?(jsnajdr)
Attachment #8749334 - Flags: review?(jsnajdr)
Attachment #8749333 - Flags: review?(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 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.
Attachment #8749334 - Flags: review?(jsnajdr) → review+
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/
Attachment #8749333 - Attachment description: MozReview Request: Bug 1270462 - part1: extract devtools tooltip toggle logic to separate file;r=bgrins → MozReview Request: Bug 1270462 - part1: extract devtools tooltip toggle logic to separate file;r=bgrins,jsnajdr
Attachment #8749334 - Attachment description: MozReview Request: Bug 1270462 - part2: tooltip toggle callback can resolve(false);r=bgrins → MozReview Request: Bug 1270462 - part2: tooltip toggle callback can resolve(false);r=bgrins,jsnajdr
Attachment #8749333 - Flags: review?(jsnajdr)
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/
Attachment #8749333 - Flags: review?(jsnajdr) → review+
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 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
Attachment #8749333 - Flags: review?(bgrinstead) → review+
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
Attachment #8749334 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/fa953819c393
https://hg.mozilla.org/mozilla-central/rev/91faa576bfe4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.