1082693 - Show PR/Bug summary tooltip when mousing over of a changeset's bug number (like TBPL)

Status: NEW

(Tree Management :: Treeherder: Frontend, defect, P3)

Description

[Chris Peterson [:cpeterson]]

Many changeset descriptions differ from the original bug summary, especially for patch series. TBPL shows the bug summary as a tooltip when hovering over a changeset's bug number.

Comment 1

[Ed Morley [:emorley]]

TBPL implementation:
https://hg.mozilla.org/webtools/tbpl/file/94d11ce30b8a/js/Data.js#l27

Comment 2

[Jonathan French (:jfrench)]

Maybe I misunderstand the description, but I think this bug is fixed? (excluding known bug 1133021).

I see the bug summary as a tooltip when I hover over a changeset's adjacent bug number, in a resultset.

Comment 3

[Phil Ringnalda (:philor)]

I don't, I see the commit message in a tooltip. https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d81e8b99b284 and hover the "1140806", expected tooltip would be "Bug 1140806 - Fennec debug build crashes on startup with "Can't open /dev/urandom?!" in js/src/jsmath.cpp" but actual is "Bug 1140806 - Initialize JS random seed using arc4random on Android and BSDs. r=fitzgen"

Comment 4

[Jonathan French (:jfrench)]

I see. It must have been the particular ones I looked at, which shared identical values for the commit message and its bug summary.

Comment 5

[Jonathan French (:jfrench)]

A note, for the time being, we will have "github.com" and "bugzilla.mozilla.org" proxy titles as a result of fixing bug 1133021 rather than what we had before, which was nothing and only incidentally worked if the commit message happened to match the Bug summary or PR title.

Comment 8

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

Attachment: PR 764 (First attempt)

This seems to work for me testing with Firefox 39.

Only issue that might be a problem is that the fetch API is relatively new (only enabled in Firefox 39+ and Chrome 42+), so maybe Travis won't like it. No existing functionality should break if it's unsupported, though, it just won't fetch the summaries.

Comment 9

[Ed Morley [:emorley]]

Comment on attachment 8634211 [details] [review]
PR 764 (First attempt)

Have added some comments to the PR. Think we need to cache+add a slight delay, but looking good so far :-)

Comment 10

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

Comment on attachment 8634211 [details] [review]
PR 764 (First attempt)

Okay, this now caches(ish) the bug data in a local object so that we only need to fetch a given bug's information from bugzilla once (so a multi-commit patch for a bug doesn't have to re-fetch the information for each commit).

I switched from fetch() to jquery's getJSON() which cut down on the boilerplate quite a bit.

I haven't added a hover delay as requested, because I think we'd need to use something like http://cherne.net/brian/resources/jquery.hoverIntent.html

Comment 11

[Cameron Dawson [:camd]]

You could just use $timeout: https://docs.angularjs.org/api/ng/service/$timeout

Put the promise in scope on the onmouseover and then cancel it in onmouseout.  Just a thought.  But the hoverIntent thing looks pretty cool, too.

I agree with Ed though.  It should have some kind of delay.

Comment 12

[Ed Morley [:emorley]]

Comment on attachment 8634211 [details] [review]
PR 764 (First attempt)

Almost there; left a comment :-)

Comment 13

[Cameron Dawson [:camd]]

Comment on attachment 8634211 [details] [review]
PR 764 (First attempt)

I left a comment here in the bug, but forgot to clear the feedback request.  So, yeah, just adding some delay would be good.  Thanks!  :)

Comment 15

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

Comment on attachment 8634211 [details] [review]
PR 764 (First attempt)

Alrighty, I think this is as close as it's gonna get.

When you first hover over a bug link, it immediately checks whether we already have this bug's details in the bugsCache. If we find the details in there, we immediately set the bug link's title to show those details, skipping the hover timeout completely. The mouseover and mouseout handlers are removed since we've already dealt with this bug, so future hovers won't do anything special.

If we don't know about the bug, we start a 1000ms hover timeout to make sure we really want to fetch the bug details. As the bug details are being fetched (and until you move the mouse out and back onto the link), the bug's title shows "Fetching bug details... Move cursor away and back again to show information."

Once the bug details are fetched (regardless of whether the user has moused out and back onto the link), the link's title gets updated to show the bug details, and the mouseover/mouseout handlers are removed.

When the user mouses out and back onto the link, it displays the recently-fetched details that are already in the title attribute.



This handles:
1. Non-public bugs (bzapi returns 401 error for these)
2. Non-existent bugs (bzapi returns 404 error for these)
3. Bugs that we've already retrieved while the current page is opened (multi-commit pushes for a single bug will only hit bzapi once)


The bugs cache is not stored locally outside of a single page load, it gets blown away when the page reloads or closes.


The only issue I can see is that the title doesn't automatically change once we fetch the bug details, but that's a platform issue. We could make fake tooltips that handle this much better, but those probably aren't supported by accessibility tools (not that TH is very accessible as it stands).

Comment 16

[Ed Morley [:emorley]]

Comment on attachment 8634211 [details] [review]
PR 764 (First attempt)

Cameron may be a better person to review this :-)

Comment 17

[Cameron Dawson [:camd]]

I just wanted to mention that I've been going over this (with several interruptions) today.  I keep thinking there must be a better approach, but perhaps there's not.  
I thought there might be a way to do this with popovers from here: http://angular-ui.github.io/bootstrap/ instead of normal tooltips.  

You could use the mouseenter trigger like you're doing, but instead of having to re-hover on the link, it could reveal the popover when the ajax call returns.  But I'm still investigating.

Comment 18

[Cameron Dawson [:camd]]

Comment on attachment 8634211 [details] [review]
PR 764 (First attempt)

Clearing my review flag for now.  Please update to create a bugzilla service, like ``treestatus.js``, to make calls to get the info.  It's more consistent with how we make other calls.

Please re-add me when you've done your magic.  Thanks for tackling this!  :)

Comment 19

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

I can make a bugzilla service[1], and I can inject[2] it into the linkifyBugs filter and use it in there, but I can't get it to work in the function called when I mouseover a bug link (or more usefully, in the function that the mouseover function calls, since the mouseover function is only there to add a delay to the hover effect). Any ideas?


1. https://github.com/mozilla/treeherder/pull/764/files#diff-2169e2dcbdc657216334769b6e994de2R1
2. https://github.com/mozilla/treeherder/pull/764/files#diff-f3c6d859d44fcbc59c5c40ce6e7cb037R39

Comment 20

[William Lachance (:wlach)]

(In reply to Wes Kocher (:KWierso) from comment #19)
> I can make a bugzilla service[1], and I can inject[2] it into the
> linkifyBugs filter and use it in there, but I can't get it to work in the
> function called when I mouseover a bug link (or more usefully, in the
> function that the mouseover function calls, since the mouseover function is
> only there to add a delay to the hover effect). Any ideas?
> 
> 
> 1.
> https://github.com/mozilla/treeherder/pull/764/files#diff-
> 2169e2dcbdc657216334769b6e994de2R1
> 2.
> https://github.com/mozilla/treeherder/pull/764/files#diff-
> f3c6d859d44fcbc59c5c40ce6e7cb037R39

:crosscent did something very similar to this in bug 1276354, using angular bootstrap tooltips. The trick is to use ng-mouseenter and ng-mouseleave to trigger the events. You can see a demo of it here (hover over the "downstream" links at the bottom)

https://treeherder.mozilla.org/perf.html#/alerts?id=1328

Comment 21

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #20)
> (In reply to Wes Kocher (:KWierso) from comment #19)
> :crosscent did something very similar to this in bug 1276354, using angular
> bootstrap tooltips. The trick is to use ng-mouseenter and ng-mouseleave to
> trigger the events. You can see a demo of it here (hover over the
> "downstream" links at the bottom)
> 
> https://treeherder.mozilla.org/perf.html#/alerts?id=1328

(as a quick aside, we even added a progress spinner there, but I'm not sure if I'd copy that for bugzilla as I believe getting the bug summary is ~ instantaneous)

Comment 22

[Cameron Dawson [:camd]]

Wes: I like Will's idea of using angular bootstrap tooltips.  Or I think there's one called Popover.  It let's you do things asynchronous like this.  Would you be up for using that technique?

Comment 23

[GitHub Bugzilla PR Linker]

Attachment: PR 3252 (second attempt)

Comment 24

[William Lachance (:wlach)]

Comment on attachment 8952893 [details] [review]
PR 3252 (second attempt)

I decided to have another try at implementing this. It's kind of a horrible hack, but it seems to work pretty well and doesn't have some of the other disadvantages of the previous implementation. What do you think?

Comment 25

[William Lachance (:wlach)]

Comment on attachment 8952893 [details] [review]
PR 3252 (second attempt)

Having slept on it and looked at the bug, I think this probably still needs work:

* There is no timeout, so hovering over a bunch of bugs can really hammer the server (this came up with Wes's original PR as well).
* Creating a javascript function per href was pretty gross, but the only other easy alternative (if we want to use a filter) is to put a function on the global scope which is pretty bad.

I think the proper thing to do here is just to implement this functionality in the react component somewhow (https://github.com/mozilla/treeherder/blob/master/ui/job-view/Revision.jsx I guess) and forget about the silly filter. It would probably be a lot easier to get the right sorts of behaviour without polluting the global scope that way.

I can't really justify spending any more time on this right now though, so i guess I'll just close the PR. I think this is actually a pretty useful piece of functionality IMO for sheriffs, so it does seem like something that should be gotten back to. Probably not a good first bug, but maybe a good second one?

Comment 26

[Erica Aguilar]

Attachment: same problem

Thanks from tavarpuncko1981.