Closed Bug 931845 Opened 6 years ago Closed 6 years ago

Timeout for showing image tooltips is a bit too long

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: rik, Assigned: pbro)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 6 obsolete files)

I know it's tricky to get right. I believe a timeout < 50ms would be nice.
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.
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.
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;
}
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.
Assignee: nobody → pbrosset
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.
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.
EDIT : Ok, Firebug and Chrome devtools show images immediately!
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.
Flags: needinfo?(dhenein)
(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.
Attached video panel-mouseleave.mov
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.
Attachment #827391 - Flags: review?(bgrinstead)
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.
Flags: needinfo?(dhenein)
(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.
As described above, I found a solution for this flickering problem
So this new patch contains that fix too.
Attachment #827391 - Attachment is obsolete: true
Attachment #827391 - Flags: review?(bgrinstead)
Attachment #827481 - Flags: review?(bgrinstead)
Rebased!
Attachment #827481 - Attachment is obsolete: true
Attachment #827481 - Flags: review?(bgrinstead)
Attachment #827515 - Flags: review?(bgrinstead)
Attached video 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
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.
Attachment #827515 - Flags: review?(bgrinstead)
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
Attachment #827515 - Attachment is obsolete: true
Attachment #827931 - Flags: review?(bgrinstead)
This try build should be more relevant: https://tbpl.mozilla.org/?tree=Try&rev=7077e7aea9b7
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.
Attachment #827931 - Flags: review?(bgrinstead) → review+
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.
Try build doesn't look good, there seems to be erratic test timeout on linux.
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.
Attachment #827931 - Flags: review+
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 :(
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").
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.
Blocks: 938097
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
Attachment #827931 - Attachment is obsolete: true
Attachment #831468 - Flags: review?(bgrinstead)
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.
Attachment #831468 - Flags: review?(bgrinstead) → review+
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
Keywords: checkin-needed
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.
Keywords: checkin-needed
Ok, making this depending on bug 889638.
I'm going to rebase this patch on top of bug 889638's patch in a second.
Depends on: 889638
Rebased on top of the patch in bug 889638.
So, to be applied once bug 889638 is applied.
Attachment #831468 - Attachment is obsolete: true
Attachment #832805 - Flags: review+
Keywords: checkin-needed
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.
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!
Ready for check-in now!
Attachment #832805 - Attachment is obsolete: true
Attachment #8333842 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9cbb559661cd
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.