931845 - Timeout for showing image tooltips is a bit too long

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Anthony Ricaud (:rik)]

I know it's tricky to get right. I believe a timeout < 50ms would be nice.

Comment 1

[Brian Grinstead [:bgrins]]

I believe right now it is 750ms.  Agree that it could be shorter, especially since mousemoves seem to clear and restart the timeout.  So if you move left to right across a background image link in the rule view, it won't show up until you hold the mouse still for the timeout.  This may not matter if the timeout was shorter, but that could be another thing to consider changing.

Comment 2

[Patrick Brosset <:pbro>]

I agree the timeout is a bit long, let's try something shorter.
Also, as long as the mouse hovers over the image link, even if it moves from left to right, the tooltip should stay. At least it works this way in the markup-view and used to work too in the rule-view, for some reason I must have broke that.

Comment 3

[Patrick Brosset <:pbro>]

It might be also good to get rid of the CSS animations that is used when the panel shows.
To be tested, but it seems that it's a bit too obstructive.

The animation can be removed by overriding the XUL panel's CSS rule:
.panel-arrowcontainer {
  transition: none;
}

Comment 4

[Patrick Brosset <:pbro>]

So, 3 things to be fixed here:

- Lowering the show delay a little bit (I've played with it, and anything below 500ms seems really too fast, considering that tooltips should not get in the way).
- Making sure that when moving the mouse over an image src in the markup-view or image background in the rule-view, the tooltip delay is not reset.
- Getting rid of the show animation to make it feel snappier.

Comment 5

[Anthony Ricaud (:rik)]

What is your concern about "getting in the way"? There is no action possible on the tooltip so showing it as soon as possible shouldn't get in the way.

Comment 6

[Patrick Brosset <:pbro>]

My point is I think any preview/informational tooltip should come with a delay. If we show it immediately, simply quickly moving your mouse over the markup or rule views will make them pop-up when all you perhaps wanted was to aim a something else, they just happened to be in the way.

This is just my immediate reaction though, I haven't really thought about it in terms of UX a lot and I haven't yet looked at other devtools. I'm happy to change my mind if it makes more sense.

Comment 7

[Patrick Brosset <:pbro>]

EDIT : Ok, Firebug and Chrome devtools show images immediately!

Comment 8

[Patrick Brosset <:pbro>]

CCing Darrin. 
@Darrin: what's your point of view on tooltip show timeouts? I originally started with 750ms, so that they only appear if you hover over the image URL for 750ms. Anthony is basically saying there shouldn't be any delay, and other devtools agree. After playing with this a little bit, I tend to agree too, but would like your input on this.

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset from comment #4)
> So, 3 things to be fixed here:
> 
> - Lowering the show delay a little bit (I've played with it, and anything
> below 500ms seems really too fast, considering that tooltips should not get
> in the way).
> - Making sure that when moving the mouse over an image src in the
> markup-view or image background in the rule-view, the tooltip delay is not
> reset.
> - Getting rid of the show animation to make it feel snappier.

About the second point here, I think this is related to the way XUL panels are inserted and shown in the UI.
Sometimes, when you start hovering over an image URL in the css rule-view sidebar, you may notice a flickering and the panel may not appear. This happens when your mouse is near the area where the arrow of the panel is pointing to.
I'll upload a video attachment to illustrate this point, but I'm pretty sure this is due to the arrow of the panel itself. It somehow covers partly the target element, and therefore causes mouseleave events, preventing the panel from showing (because the tooltip widget listens for mouseleave events to hide it).
That doesn't happen at all with non arrow type panels.

I think I would like to fix this in bug 889638 where I'm working on panels' CSS styles.

Comment 10

[Patrick Brosset <:pbro>]

Attachment: panel-mouseleave.mov

Comment 11

[Patrick Brosset <:pbro>]

Attachment: bug931845-tooltip-timeout-too-long.patch

This patch basically does the 2 following things:
- reduces the default show timeout to 50
- removes the show css animation

On top of this, I refactored a little bit the tooltip toggling mechanism. Now tooltips always get hidden on baseNodee mouseleave.
In the rule view, for instance, when an image tooltip is shown, and the mouse leaves the rule-view HTML container, the tooltip should be closed. It wasn't so far.

This however makes the flickering problem described in an earlier comment more visible. I would to land this small patch anyway and work on fixing this flickering issue in a bug of its own (or as part of bug 889638) as there is more work involved.

Comment 12

[Darrin Henein [:darrin]]

I would tend to agree that 750ms is probably a bit high for a delay, but it shouldn't be 0ms (to avoid the flickering when you are moving your mouse past a trigger). I'd say lets try it with 50ms and see how that feels, it will really just come down to what feels natural, quick enough but not obstructive. It should 'feel' like it's instant... even if there is actually an imperceptible delay.

Comment 13

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset from comment #9)
> (In reply to Patrick Brosset from comment #4)
> > So, 3 things to be fixed here:
> > 
> > - Lowering the show delay a little bit (I've played with it, and anything
> > below 500ms seems really too fast, considering that tooltips should not get
> > in the way).
> > - Making sure that when moving the mouse over an image src in the
> > markup-view or image background in the rule-view, the tooltip delay is not
> > reset.
> > - Getting rid of the show animation to make it feel snappier.
> 
> About the second point here, I think this is related to the way XUL panels
> are inserted and shown in the UI.
> Sometimes, when you start hovering over an image URL in the css rule-view
> sidebar, you may notice a flickering and the panel may not appear. This
> happens when your mouse is near the area where the arrow of the panel is
> pointing to.
> I'll upload a video attachment to illustrate this point, but I'm pretty sure
> this is due to the arrow of the panel itself. It somehow covers partly the
> target element, and therefore causes mouseleave events, preventing the panel
> from showing (because the tooltip widget listens for mouseleave events to
> hide it).
> That doesn't happen at all with non arrow type panels.
> 
> I think I would like to fix this in bug 889638 where I'm working on panels'
> CSS styles.

I found a simple explanation after all. Arrow panels have a 'transform: translateX(-20px);` declaration (depending on their orientation) that is used in the appearing `transition`. So resetting this `transform` property means that the panel is not going to first appear over the target, and therefore is not going to cause mouseleave events.

Comment 14

[Patrick Brosset <:pbro>]

Attachment: bug931845-tooltip-timeout-too-long.patch

As described above, I found a solution for this flickering problem
So this new patch contains that fix too.

Comment 15

[Patrick Brosset <:pbro>]

Attachment: bug931845-tooltip-timeout-too-long.patch

Rebased!

Comment 16

[Brian Grinstead [:bgrins]]

Attachment: tooltip.mp4

A couple quick things I've noticed:

1) The tooltip doesn't disappear when scrolling (see video)
2) After changing a property in the ruleview, the tooltip stops showing up until you reselect the node again.  Is this already filed under another bug?
3) The timeout seems good to me - seems to show up pretty much instantly

Comment 17

[Patrick Brosset <:pbro>]

Thanks for spotting this Brian
> 1) The tooltip doesn't disappear when scrolling (see video)
-> I've fixed this, will attach a new patch
> 2) After changing a property in the ruleview, the tooltip stops showing up
> until you reselect the node again.  Is this already filed under another bug?
-> In fact it's not showing up anymore because after you changed a property, you probably had another one in edit mode, and so far, the tooltip was not showing as long as something was being edited.
This was because whenever the tooltip would appear, it would steal the focus from the currently edited field.
I've managed to fix this by using the panel's "noautofocus" attribute, so now, tooltips always show up, whether you are in edit mode or not, and if there's an edit field, it keeps its focus.
> 3) The timeout seems good to me - seems to show up pretty much instantly
-> Great, let's go with this then.

I'll try and add some tests for 1 and 2.

Comment 18

[Patrick Brosset <:pbro>]

Attachment: bug931845-tooltip-timeout-too-long.patch

ok, new version of the patch!
- fixes the 2 first points raised by Brian
- adds new test cases
- fixes failing tests

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=ec8a2849afd4

Comment 19

[Patrick Brosset <:pbro>]

This try build should be more relevant: https://tbpl.mozilla.org/?tree=Try&rev=7077e7aea9b7

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 827931 [details] [diff] [review]
bug931845-tooltip-timeout-too-long.patch

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

Looks good.  r+ once we have a green try build.

Comment 21

[Patrick Brosset <:pbro>]

The try build is still ongoing, mostly green so far, but there are unstable things. There's even an orange bc with no logs! And failing unrelated tests.
I've requested a few more test run. We'll see what happens.

Comment 22

[Patrick Brosset <:pbro>]

Try build doesn't look good, there seems to be erratic test timeout on linux.

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 827931 [details] [diff] [review]
bug931845-tooltip-timeout-too-long.patch

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

Waiting until the intermittent test failures are resolved.

Comment 24

[Patrick Brosset <:pbro>]

All timeouts on linux are cause by a bug I introduced when adding an event listener to "scroll" that I'm using to hide the tooltip. This just doesn't seem to work at all on linux as when the panel is shown, the mousewheel has no effect anymore :(

Comment 25

[Patrick Brosset <:pbro>]

I'm a bit at a loss with this, the patch is fine, reviewed, and tests pass on most platforms.
The problem seems to happen mainly on Linux and with debug builds only.

After a more thorough investigation, I was able to find out that even if there is this weird scroll issue on linux (for which I've filed bug 935874), this is not what causes the test failure. 
The test shows the tooltip, and then simulates a scroll event, which gets listened correctly by the tooltip widget which uses it to hide itself.
That code is executed. But in the test, to assert that it was hidden I do this:

    panel.addEventListener("popuphidden", function hidden() {
      panel.removeEventListener("popuphidden", hidden, true);
      ok(true, "Scrolling down closed the tooltip");
    }, true);

    executeSoon(function() {
      EventUtils.synthesizeWheel(uriSpan, 10, 10,
        {deltaY: 50, deltaMode: WheelEvent.DOM_DELTA_PIXEL},
        ruleView.doc.defaultView);
    });

And for some reasons, the "popuphidden" event listener is never called (I've also tried with "popuphiding").

Comment 26

[Patrick Brosset <:pbro>]

I haven't yet been able to find a proper solution to this and since it's holding up a few other things that I'd like to merge this one with, I'd like to propose that I open a new bug for the scroll testing issue and try and land this one without this.

Comment 27

[Patrick Brosset <:pbro>]

Attachment: bug931845-tooltip-timeout-too-long-V2.patch

As explained in my last comment, here's a patch for the main problem raised in this bug but without a fix or test for hiding the tooltip on scroll.
For that particular problem, see bug 938097.

@Brian: are you ok to R+ this new, simpler, patch?

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=70ccd65e6bbd

Comment 28

[Brian Grinstead [:bgrins]]

Comment on attachment 831468 [details] [diff] [review]
bug931845-tooltip-timeout-too-long-V2.patch

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

(In reply to Patrick Brosset from comment #26)
> I haven't yet been able to find a proper solution to this and since it's
> holding up a few other things that I'd like to merge this one with, I'd like
> to propose that I open a new bug for the scroll testing issue and try and
> land this one without this.

Yes, let's get these other changes in and deal with the scrolling separately.  r+ with a green try.

Comment 29

[Patrick Brosset <:pbro>]

Ok, try build is finally green. Some debug builds must have suffered from a higher load at some stage cause there's some oranges coming from unrelated tests, but extra run were green: https://tbpl.mozilla.org/?tree=Try&rev=70ccd65e6bbd

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Sorry to do this to you, but can you please rebase this on top of the patch in bug 889638? It has some conflicts and this one looks easier to adjust than that one.

Comment 31

[Patrick Brosset <:pbro>]

Ok, making this depending on bug 889638.
I'm going to rebase this patch on top of bug 889638's patch in a second.

Comment 32

[Patrick Brosset <:pbro>]

Attachment: bug931845-tooltip-timeout-too-long-V3.patch

Rebased on top of the patch in bug 889638.
So, to be applied once bug 889638 is applied.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

This has test failures.
https://tbpl.mozilla.org/php/getParsedLog.php?id=30599415&tree=Try

Comment 34

[Patrick Brosset <:pbro>]

Sorry, that was a rebase mistake. Let me pull the patch, fix this issue and run tests. I'll attach the new patch and ask for check-in when try build is green.

Comment 35

[Patrick Brosset <:pbro>]

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=331be3c042b0

Comment 36

[Patrick Brosset <:pbro>]

Try build is green. The linux debug build tests failed a few times on several different unrelated tests, but finally passed. I'm assuming this is due to heavy load on the machine.
Attaching the new patch!

Comment 37

[Patrick Brosset <:pbro>]

Attachment: bug931845-tooltip-timeout-too-long-V4.patch

Ready for check-in now!

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/9cbb559661cd

Comment 39

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

https://hg.mozilla.org/mozilla-central/rev/9cbb559661cd