Closed Bug 1270111 Opened 4 years ago Closed 4 years ago

Tooltip.js: Improve the behavior of startTogglingOnHover

Categories

(DevTools :: Shared Components, defect)

defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox49 --- fixed

People

(Reporter: jsnajdr, Assigned: jsnajdr)

References

Details

Attachments

(3 files, 4 obsolete files)

In bug 1134073, I need to display a tooltip when user hovers over a "Cause" field in the network request list, and this tooltip is interactive, i.e., the user can click on the items inside it.

When I wanted to use Tooltip.js and its startTogglingOnHover helper method, I discovered I need to make several changes to the Tooltip.js class in order to make it work. These changes should be incorporated into Tooltip.js or into its HTML successor.

They are:
1. When I leave the hovered element, don't close the tooltip immediately. Instead, close it only after some delay, and give me time to move the mouse inside the tooltip element.
2. When I mouseover inside the tooltip element, don't hide it or move it until the mouse leaves. This allows the tooltip to be interactive.

These two changes are implemented in this patch: https://bugzilla.mozilla.org/show_bug.cgi?id=1134073#attach_8747764

There are other improvements that I didn't implement yet because they are not so pressing, but would be very nice anyway:
3. The tooltip arrow is not aligned to the center of the anchor element - it's offset by a few pixels to the right (at least on OS X)
4. The startTogglingOnHover logic uses a this._lastHovered property to track movement from one element to another. It's always set to the event.target of the "mousemove" element. It would be nice to be able to select some ancestor of the event.target as the _lastHovered element. For example, I can have markup like this:

<div class="request-url">
  <img class="request-image-preview"/>
  <span class="request-url-text"></div>
</div>

When I move the mouse from the <img> to the <span> and back, startTogglingOnHover forces the tooltip to be hidden and then redisplayed over the other element. I need a way to tell the tooltip to not care about this move, and track movement between the "request-url" parent <div>s instead.

All these improvements would improve the UX of tooltips, and probably also simplify the code for variable bubble view in debugger, and the various swatches in the inspector rule view.
Adding ni? for Julian - how does this fit into the new Tooltip HTML API?
Blocks: 1134073
Flags: needinfo?(jdescottes)
The HTMLTooltip aims to share the basic API of Tooltip.js (hide/show/isVisible etc ...). See Bug 1259834 and Bug 1266448. In theory, all the changes you make to this part of Tooltip.js should remain compatible with the HTMLTooltip.

I wanted to extract the "toggleOnHover" logic to a separate file shared by both HTMLTooltip and Tooltip, to avoid dealing with duplicated code during the migration. I looked at your patch and it seems like the changes are compatible with this approach. 

I wanted to do this as part of Bug 1266448, but since we are both making changes to Tooltip.js, it could be easier to have a separate bug to extract the behavior and land it.
Flags: needinfo?(jdescottes)
(In reply to Jarda Snajdr [:jsnajdr] from comment #0)
> 
> There are other improvements that I didn't implement yet because they are
> not so pressing, but would be very nice anyway:
> 3. The tooltip arrow is not aligned to the center of the anchor element -
> it's offset by a few pixels to the right (at least on OS X)

There are lot of issues with the "arrow" type XUL panel. The corresponding bug for the HTML tooltip is 1267401. We will generate the arrow in HTML. I think we should be able to position it precisely to point to the anchor.
This patch improves the behavior of Tooltip and TooltipToggle widgets: don't hide the tooltip when mouse leaves the hovered element and enters the tooltip. Allows the tooltip to be interactive. Also, adds a slight delay between the mouse event and the tooltip showing/hiding. Gives the user some time to move the mouse from the hovered element into the tooltip.

There are 3 parts:

Part 1: The main thing, changes the behavior of TooltipToggle.
Part 2: A small drive-by change for the netmonitor mochitests do disable excessively verbose logging of all network events. I find myself disabling this log message every time I debug any netmonitor mochitest.
Part 3: To provide some test coverage to the TooltipToggle, I refactored, fixed and improved the browser_net_image-tooltip.js mochitest in netmonitor:
- first, refactored it to a Task.js form
- second, fixed the errors that caused it to be disabled (see my comments in bug 1252641 for details), and reenabled it in browser.ini
- third, instead by showing the tooltip by tooltip.show(), I just synthesize a "mousemove" event over the right element and let TooltipToggle do the actual work of showing it.

The interactivity of the tooltip will be used in bug 1134073.
Assignee: nobody → jsnajdr
Comment on attachment 8756322 [details] [diff] [review]
Part 1: TooltipToggle doesn't hide the tooltip when mouse is over, shows and hides after a short delay

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

Looks good Jarda, thanks!

An additional comment: the "showDelay" is no longer just about showing the tooltip. Let's rename it (toggleDelay ? delay?).
The JSDoc also needs updating in this file.

Keeping the review flag to discuss 2 items:
- HTML Tooltip compatiblity
- darker shadow issue

Let's also ni? Helen about the change in behavior this patch introduces.

Helen: With this patch, the tooltips toggled on hover are no longer hidden when moving the mouse from the anchor to the tooltip content. This will allow the user to interact with the tooltip content. We have 4 tooltips impacted by this change : image & font previews in ruleview, image previews in markup view, image previews in netmonitor, line gutter errors in shader editor.
Are we ok with this, or should we keep the old behavior for some tooltips?

::: devtools/client/shared/widgets/tooltip/TooltipToggle.js
@@ +77,5 @@
>  
>      baseNode.addEventListener("mousemove", this._onMouseMove, false);
>      baseNode.addEventListener("mouseleave", this._onMouseLeave, false);
> +
> +    this.tooltip.panel.addEventListener("mouseover", this._onPanelMouseOver);

This is an issue with the HTML Tooltip :/

It uses an iframe as a container. When I added the panel() getter on the HTML tooltip I decided to make it point to the container inside the iframe, so that querying the content would still work (tooltip.panel.querySelector(...) used in several tests)

So first of all the HTML Tooltip's panel does not contain the border/padding/arrow. Also it is not ready right away (need to wait for the iframe load event). For the second point, we can modify the HTML tooltip so that the start/stopTogglingOnHover methods wait for the containerReady promise to resolve.

But in this context I think you need in any case to get the tooltip's container including the arrow. On the HTML tooltip, this would be tooltip.container. 

What about adding a container() getter on the XUL tooltip?

The semantics would be:
- panel: the parent of the tooltip content
- container: the parent of the tooltip element

For the XUL tooltip, both would actually point to the XUL panel.

@@ +93,5 @@
>      if (!this._baseNode) {
>        return;
>      }
>  
>      this._baseNode.removeEventListener("mousemove", this._onMouseMove, false);

The useCapture=false is not useful here (it is the default value). To be consistent with the new listeners you added, we could remove them.

@@ +110,5 @@
>        this._lastHovered = event.target;
>  
>        this.win.clearTimeout(this.toggleTimer);
>        this.toggleTimer = this.win.setTimeout(() => {
> +        this.tooltip.hide();

There seems to be a weird issue with the XUL tooltip when hiding it and showing it right after.

If you open 

data:text/html,<div style="font-family:Arial;font-family:Monospace;">

go to the rule view, hover "Arial" and then move the mouse to "Monospace", you will see the Tooltip shadow become much darker, as if it was rendered twice. We should just try to confirm this is only a rendering issue.

(you can increase the show delay to make to reproduce it easily, but I got it with the default one originally)
Attachment #8756322 - Flags: review?(jdescottes) → feedback+
(In reply to Julian Descottes [:jdescottes] from comment #9)
> Comment on attachment 8756322 [details] [diff] [review]
> 
> Helen: With this patch, the tooltips toggled on hover are no longer hidden
> when moving the mouse from the anchor to the tooltip content. This will
> allow the user to interact with the tooltip content. We have 4 tooltips
> impacted by this change : image & font previews in ruleview, image previews
> in markup view, image previews in netmonitor, line gutter errors in shader
> editor.
> Are we ok with this, or should we keep the old behavior for some tooltips?
> 

Helen: question for you in my previous comment
Flags: needinfo?(hholmes)
Comment on attachment 8756323 [details] [diff] [review]
Part 2: Netmonitor mochitest logging is too verbose, disable the detailed progress info

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

This log is indeed very noisy! Thanks for cleaning this up.

::: devtools/client/netmonitor/test/head.js
@@ +252,5 @@
>        panel.NetMonitorController.webConsoleClient.getNetworkRequest(actor);
>      let url = networkInfo.request.url;
>      updateProgressForURL(url, event);
>  
> +    // info("> Current state: " + JSON.stringify(progress, null, 2));

Let's just add a small comment above explaining that this can be uncommented for additional debug logs while developing, otherwise it might be deleted by the next person.
Attachment #8756323 - Flags: review?(jdescottes) → review+
- renamed showDelay to toggleDelay, updated jsdocs, too
- added a container() getter to Tooltip.js, use it in TooltipToggle.js
- no third argument to add/removeEventListener (uses the 90-char line limit already, which is not yet in m-c)
Attachment #8756834 - Flags: review?(jdescottes)
Attachment #8756322 - Attachment is obsolete: true
Added comment explaining why the verbose message is commented out.
Attachment #8756835 - Flags: review+
Attachment #8756323 - Attachment is obsolete: true
Just a few comments:

- Now we can hover over image preview tooltips but doesn't appear that I can right-click > save them, which I would suspect would be the case if I can interact/hover over them. That feels a little odd.
- The delay feels natural in the Network Monitor but slow in the Inspector (I think because the Inspector doesn't have other image previews popping up as often to overtake the last popup). Not the biggest deal, but does feel a little strange.

> There are other improvements that I didn't implement yet because they are
> not so pressing, but would be very nice anyway:
> 3. The tooltip arrow is not aligned to the center of the anchor element -
> it's offset by a few pixels to the right (at least on OS X)
If improving the tooltip stylistically is interesting to you, there are some new designs for the tooltips attached to Bug 1258365.
Flags: needinfo?(hholmes)
(In reply to Julian Descottes [:jdescottes] from comment #9)
> Helen: With this patch, the tooltips toggled on hover are no longer hidden
> when moving the mouse from the anchor to the tooltip content. This will
> allow the user to interact with the tooltip content. We have 4 tooltips
> impacted by this change : image & font previews in ruleview, image previews
> in markup view, image previews in netmonitor, line gutter errors in shader
> editor.
> Are we ok with this, or should we keep the old behavior for some tooltips?

There's one UX thing that I'm personally not happy with: if you open this:

data:text/html,<div style="font-family:Arial;font-family:Monospace;">

and go to the rule view, you can hover over "Arial", see the tooltip, then move down to "Monospace" and see the tooltip moving with you. This way you can quickly preview any list.

However, after the patch, it now doesn't work when you want to walk the list in the opposite direction: if you move the mouse cursor up, the tooltip is in your way and you "stumble" over it. This is a bit annoying.

I don't know about any nice solution here, so if you have any idea...

> There seems to be a weird issue with the XUL tooltip when hiding it and
> showing it right after.
> [...]
> you will see the Tooltip shadow become much darker, as if it was rendered
> twice. We should just try to confirm this is only a rendering issue.

I debugged this for a while and it seems like some rendering bug. There is only one tooltip element shown, not two identical elements aligned over each other.
Comment on attachment 8756332 [details] [diff] [review]
Part 3: Fixed and refactored the browser_net_image-tooltip.js test

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

Thanks for fixing this test! 
r+ granted we have a green try (including windows 8).

::: devtools/client/netmonitor/test/browser.ini
@@ -77,5 @@
>  [browser_net_footer-summary.js]
>  [browser_net_html-preview.js]
>  [browser_net_icon-preview.js]
>  [browser_net_image-tooltip.js]
> -skip-if = true # Bug 1234341, bug 1252641

In Bug 1234341, this test was only explicitly disabled for Windows 8 only, using:

skip-if = (os == "win" && os_version == "6.2" && bits == 64)

Before re-enabling completely let's make sure to have at least one try push checking Windows 8.

::: devtools/client/netmonitor/test/browser_net_image-tooltip.js
@@ +3,5 @@
>  
>  /**
>   * Tests if image responses show a popup in the requests menu when hovered.
>   */
> +"use strict";

nit: in most tests, use strict is after the license header and before the test description.

@@ +18,3 @@
>  
> +  let wait1 = promise.all([
> +    waitForNetworkEvents(monitor, 7),

Just a suggestion: instead of the promise all, we could do :

let onEvents = waitForNetworkEvents(monitor, 7);
let onThumbnail = waitFor(monitor.panelWin, EVENTS.RESPONSE_IMAGE_THUMBNAIL_DISPLAYED);
debuggee.performRequests();

yield onEvents;
yield onThumbnail;
Attachment #8756332 - Flags: review?(jdescottes) → review+
Comment on attachment 8756834 [details] [diff] [review]
Part 1: TooltipToggle doesn't hide the tooltip when mouse is over, shows and hides after a short delay

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

Code change is good, no further nits from me :) And thanks for investigating the drop shadow glitch. 

Reading Helen's feedback and your UX comment, I think this new behavior would be better opt-in.
We should enable it only if the tooltip can be interacted with, otherwise it could be misleading for the user.
What do you think?

(just keeping the review flag until we find a solution about the topic above, otherwise r+ for the current changeset)
Attachment #8756834 - Flags: review?(jdescottes)
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #14)
> - Now we can hover over image preview tooltips but doesn't appear that I can
> right-click > save them, which I would suspect would be the case if I can
> interact/hover over them. That feels a little odd.

This could be a nice follow-up bug - make the tooltips interactive where it's useful, now that the interactivity is enabled.

> - The delay feels natural in the Network Monitor but slow in the Inspector
> (I think because the Inspector doesn't have other image previews popping up
> as often to overtake the last popup). Not the biggest deal, but does feel a
> little strange.

Which tooltip exactly do you mean? The default delay is just 50ms - when I try out the popups in inspector, the delay is indistinguishable. It's set to 500ms only in netmonitor.
Flags: needinfo?(hholmes)
(In reply to Jarda Snajdr [:jsnajdr] from comment #18)
> This could be a nice follow-up bug - make the tooltips interactive where
> it's useful, now that the interactivity is enabled.
+1

> > - The delay feels natural in the Network Monitor but slow in the Inspector
> > (I think because the Inspector doesn't have other image previews popping up
> > as often to overtake the last popup). Not the biggest deal, but does feel a
> > little strange.
> 
> Which tooltip exactly do you mean? The default delay is just 50ms - when I
> try out the popups in inspector, the delay is indistinguishable. It's set to
> 500ms only in netmonitor.
I meant the image previews. It could just be in my head since I was paying much more attention to them than I ever have to review the patches—since that seems to be the case, please disregard what I said before and don't consider it a blocker.
Flags: needinfo?(hholmes)
TooltipToggle.start now has multiple options: the old toggleDelay, and new boolean "interactive" one. If set to true (going to happen in netmonitor stack trace), the tooltip keeps being displayed when mouse is over it. Otherwise (the default) it will be hidden as before.
Attachment #8757277 - Flags: review?(jdescottes)
Attachment #8756834 - Attachment is obsolete: true
Addressed the nits from Julian: better "use strict" placement, remove "promise.all" in favor of more readable two yields.

Also, submitted a new try run with mochitests on Win8.
Attachment #8757278 - Flags: review+
Attachment #8756332 - Attachment is obsolete: true
Comment on attachment 8757277 [details] [diff] [review]
Part 1: TooltipToggle doesn't hide the tooltip when mouse is over, shows and hides after a short delay

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

Thank you! This looks good.
Attachment #8757277 - Flags: review?(jdescottes) → review+
Keywords: checkin-needed
Blocks: 1252641
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.