1136918 - Too hard to select/copy commit SHAs from the list of revisions

Status: VERIFIED FIXED

(Tree Management :: Treeherder, defect, P3)

Description

[Ed Morley [:emorley]]

I thought we had a bug for this already, but obviously not. A common use case is wanting to copy the SHA for a commit that isn;t the top one, and as such isn't on the push header.

Currently it requires a very precise mouse selection to get the SHA selected to copy. Whereas on TBPL you could double click to select a SHA easily.

We could hook up the "hover to copy" behaviour, but I suspect it will impact performance, given the number of commits listed on any one page.

Fixing this would mean we could remove the somewhat ugly SHA on the push header row, which was only added because text selection was hard here.

Comment 1

[Jonathan French (:jfrench)]

If we want we could try the hover copy to see, I don't really see any hiccup or lag when I hover over the existing 3 items (logviewer, rawlog, job name) we have hooked up so far.

Comment 2

[Jonathan French (:jfrench)]

So, using the copyValue directive in this case seems non-trivial. It's not straight markup like the logviewer, raw log, or job name elements. The revision column is generated from our revision clone target template. The copy-value attribute is easy enough to inject, but the event handler appears to get munged during interpolate in directives/clonejobs.js and services/main.js.

I also tested this by putting a print into the directive and hovering over each element, you get nothing when hovering over the revisions, the directive is not triggered.

I talked with camd about it and he was of the mind that while we _might be able to add that event handler in that function through jquery, or somehow get the interpolator to check for directives, on the surface it doesn't appear an easy fix. He nominated this approach at least, as a won't fix.

Maybe there will be some other way to solve it. My nominated fix :) is raising priority for all open bugs requesting Firefox include a RMB->Copy context menu, like Chrome. It simply copies the text, and addresses the user problem.

Is that bug 746385 ?, not sure.

Worst case, if we can't get that, I suppose we could make the revisions plain text and stick an fa-external-link beside each revision, but that seems an unsatisfactory choice compared to having improved browser functionality.

Comment 3

[Ed Morley [:emorley]]

Or we could fix this from a layout/CSS point of view and make it work as least as well as it did with TBPL (it was fine there) :-)

Comment 4

[Ed Morley [:emorley]]

(ie triple click then ctrl+c)

Comment 5

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

Attachment: PR 678

Curious what you think of this solution.

Comment 6

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

Attachment: Screenshot

Comment 7

[Jonathan French (:jfrench)]

Fwiw I had spent some time during the mozwww travel day digging around to find bmo references to the TBPL triple click implementation, but came up empty so far. I'd spent a bit of time reading up on triple-click behaviors in general, still ongoing.

Comment 8

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

Attachment: Alternate screenshot

I pushed a followup commit to get this display. The extra padding to the right of the link can be double-clicked to select the SHA's text. As a nice bonus, it adds a bit of space between the SHA and the commit author initials.

Comment 9

[Jonathan French (:jfrench)]

I tried the latest branch, the interaction seems nice (due to an unrelated Nightly bug it copies an extra character after the revision.. not treeherders' fault :) ). I'm curious about the new behavior opening the revision link in a new tab with the change? I remember Ed removed a lot of those 'open in new tab' links in January, including revision
https://github.com/mozilla/treeherder-ui-deprecated/pull/336/files

If we kept that unchanged, maybe we could avoid adding the fa-external link to help our ongoing ui declutter goals, but I defer to Ed and Wes and the sheriffs :)

Comment 10

[Jonathan French (:jfrench)]

I wonder also if we should embed a 'title' in the empty space between the revision SHA and the Author Initials for discoverability? eg. "Double-click here to copy this revision" or something. So if the user is hovering over that area to traditionally drag-select-copy, they might discover this functionality.

Comment 11

[Ed Morley [:emorley]]

Comment on attachment 8626445 [details] [review]
PR 678

I've left some comments on the PR.

TBPL's markup for reference, was:

<li>
<a class="revlink" data-rev="291614a686f1" href="https://hg.mozilla.org/mozilla-central/rev/291614a686f1">291614a686f1</a>
<span class="patchTitle"><span class="author">Carsten "Tomcat" Book</span> – <span class="desc">merge mozilla-inbound to mozilla-central a=merge</span></span>
</li>

Comment 12

[Ed Morley [:emorley]]

(In reply to Jonathan French (:jfrench) from comment #10)
> I wonder also if we should embed a 'title' in the empty space between the
> revision SHA and the Author Initials for discoverability? eg. "Double-click
> here to copy this revision" or something. So if the user is hovering over
> that area to traditionally drag-select-copy, they might discover this
> functionality.

I think the vast majority of users that actually need this feature (ie sheriffs and a couple of others) will either hear about it from us, or know that double clicking selects text (I wasn't aware people didn't know about this browser feature, but maybe it's less common than I thought?), so this may be unnecessary?

Comment 13

[Jonathan French (:jfrench)]

(In reply to Ed Morley [:emorley] from comment #12)
> I think the vast majority of users that actually need this feature (ie
> sheriffs and a couple of others) will either hear about it from us, or know
> that double clicking selects text (I wasn't aware people didn't know about
> this browser feature, but maybe it's less common than I thought?), so this
> may be unnecessary?

Ya, if we will be double clicking directly on the link I agree. I had only been thinking about us providing another title in the right hand empty space, if it acted as the sole target area.

Perhaps we'd want to raise awareness of the TBPL implementation given a double click on a link acts the same as single click ie. it opens a link? So perhaps we'd want to tweak the first revision title in each resultset to something like

"open revision [sha] or double click to copy"

I just recall we sometimes have concerns expressed about feature discoverability.

Not related to this bug, but I wonder if we really need to repeat the "..https://hg.mozilla.org/integration/mozilla-inbound" in each revision title. It seems there's always context given you are in the repo.

Whatever you guys think makes sense though.

Comment 14

[Jonathan French (:jfrench)]

Ed at your leisure, do you know where that data-rev="291614a686f1" in comment 11 gets used in TBPL, or can point to the file or directive? I wonder if that is where the TBPL click magic happens.

Comment 15

[Ed Morley [:emorley]]

There is no click magic, it's simple copying of visible text :-)

Comment 16

[Ed Morley [:emorley]]

To be clear: on TBPL we'd double/triple click in the whitespace to the left of the SHA (not on the link) and the SHA would be selected.

Comment 17

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

So selecting that extra space to the right appears to be controlled by the layout.word_select.eat_space_to_next_word preference (setting it to 'false' makes it not select that trailing space). That pref defaults to 'false' at http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#2141 but is overridden on Windows to 'true' by http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#3065

Unsure where that leaves us...

Comment 18

[Ed Morley [:emorley]]

We could just trim() on classification submit?

Comment 19

[Ed Morley [:emorley]]

Either way, this is no different from TBPL; I'm only after parity :-)

Comment 20

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

Comment on attachment 8626445 [details] [review]
PR 678

So this should in theory get parity with TBPL, regardless of what that preference is set to... 

I wasn't able to get either ng-click or ng-dblclick to do anything, so this is just using the basic ondblclick function in-line to ensure the selection does not include that trailing space.

On platforms where that preference is set to 'true', it will briefly show that trailing space as selected, but it will be unselected because of the dblclick handler. When that preference is set to 'false', there's no noticeable change (the revision is selected without trailing whitespace and stays selected).

Happy to take out this workaround and leave it up to the user to make sure that trailing space gets removed. :)

Comment 21

[Ed Morley [:emorley]]

Comment on attachment 8626445 [details] [review]
PR 678

Looks good; just needs a tweak so the monospaced font etc doesn't apply to the linkified bug comments etc.

Comment 22

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

Comment on attachment 8626445 [details] [review]
PR 678

Applied Ed's fix and rebased to current master. I think this is ready for a final review.

I'd prefer to get this landed as-is for now and maybe work on jfrench's suggestions in a followup.

Comment 23

[Ed Morley [:emorley]]

Comment on attachment 8626445 [details] [review]
PR 678

r=me with the `ondblclick` tweak mentioned on the PR :-)

Comment 24

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/e772f3a30cfb7ceed6aa9d3ea74699365ec28b6c
Bug 1136918 - Make it easier to select revisions from the list

https://github.com/mozilla/treeherder/commit/49cf0fe44df6e98c920e13d0ac62a9db419024f0
Merge pull request #678 from KWierso/1136918

Bug 1136918 - Make it easier to select revisions from the list r=emorley

Comment 25

[Jonathan French (:jfrench)]

(In reply to Wes Kocher (:KWierso) from comment #22)
> 
> I'd prefer to get this landed as-is for now and maybe work on jfrench's
> suggestions in a followup.

Now bug 1187403 :-)

Comment 26

[Jonathan French (:jfrench)]

Verified fixed on production.