Closed Bug 1121631 Opened 9 years ago Closed 9 years ago

Refactor shortcuts with a known shortcut lib to make them more maintainable

Categories

(Tree Management :: Treeherder, defect, P5)

Product:
Tree Management
Component:
Treeherder
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: jfrench, Assigned: jfrench)

References

(
URL
)

Details

Attachments

(1 file, 3 obsolete files)

treeherder-ui-PR#368
49 bytes, text/x-github-pull-request
camd
: review+
Details | Review 
This was raised in the mtg last week by Mauro. We'd like to have clearer code for shortcuts, rather than extensively relying on ev.keyCode and the complex unused modifier key suppressions required.

As a by-product this will also be used to fix bug 1118342.

Some of the candidates were:

Angular (syntax seemed less clear to me)
https://github.com/chieffancypants/angular-hotkeys

Shortcuts.js (unclear to me if supported some treeherder functionality)
http://www.openjs.com/scripts/events/keyboard_shortcuts/

jKey (same as above)
https://github.com/OscarGodson/jKey

Mousetrap.js (clear and seems like it does what we need, apache2.0 license)
http://craig.is/killing/mice
https://github.com/ccampbell/mousetrap

I went with mousetrap.
Assignee: nobody → tojonmz
Status: NEW → ASSIGNED
Blocks: 1118342
Attached file treeherder-ui-PR#338 (obsolete) — 
Please see above for PR and review.
Attachment #8549120 - Flags: review?(cdawson)
Attachment #8549120 - Flags: review?(cdawson) → review+
Marking fixed per above merge. Will re-test on stage/prod.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
I've reverted this since Ryan was having issues too (alongside philor). I'll let them add STR, since we were unable to repro with the prior STR, so there may be a step missing.
Status: RESOLVED → REOPENED
Flags: needinfo?(ryanvm)
Flags: needinfo?(philringnalda)
Resolution: FIXED → ---
1. Load https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound
2. Click absolutely any job of any color.
3. Hit space to pin and open the pinboard (this is apparently the one critical step, you need to not open it by either typing c or by clicking the pin icon)
4. Click in the comment field.
5. Hit space, notice that you don't get a space in the comment field.
(6. Bonus fun, then hit u and notice that you both get a u in the comment field and also toggle unstarred.)
Flags: needinfo?(philringnalda)
Also, the lag when hitting 'u' to go into onlyunstarred mode was very noticeable :(
Flags: needinfo?(ryanvm)
That's helpful, thank you :-)
(In reply to Phil Ringnalda (:philor) from comment #6)

> 4. Click in the comment field.
> 5. Hit space, notice that you don't get a space in the comment field.

I assume the shortcut keys(like space) need a similar Mousetrap.unbind on click, or similar.

I don't have any explanation on the lag hitting 'u' at this time. It always seemed slow to me, prior to the change.
I now have the shortcut keys being unbound on click in a local branch. So philor's workflow seems to work fine.

I can see the lag in 'u' now. It was also slow on production prior, due to an unrelated change. I'm looking into that 'u' lag now.
Still tracing the 'u' shortcut slowdown. Attaching a wip just for safekeeping, which contains everything else. The patch also includes the mousetrap refactor for dewgeek's bug 1089737, which I anticipate will land before this work.
Latest wip patch for safekeeping, latest rebase, minor tweaks. 'u' now seems a reasonable speed. But still an issue to be sorted, an ambiguous toggle state after idle/load.
Attachment #8554218 - Attachment is obsolete: true
I think I have everything sorted now. PR coming up after I've re-tested everything a final time.
Attached file treeherder-ui-PR#368 — 
Please see above PR for status and review.
Attachment #8549120 - Attachment is obsolete: true
Attachment #8561089 - Attachment is obsolete: true
Attachment #8565082 - Flags: review?(cdawson)
So I've re-tested this on dev, and the other workflows (ctrl+enter to save while mid-comment, ctrl+shift+u to clear-all while mid-comment) and the responsiveness and performance seems normal. If any of the Sheriffs could have a bash at all their favorite shortcuts and workflows on dev, that would be awesome before we push to stage.
Flags: needinfo?(wkocher)
Flags: needinfo?(ryanvm)
Flags: needinfo?(philringnalda)
Will do.
Flags: needinfo?(philringnalda)
Seems fine for the couple minutes I played with it. Seems more likely that any issues are more likely to shake out after it lands in production, though.
Flags: needinfo?(ryanvm)
Seems to work as expected, but I agree with Ryan.
Flags: needinfo?(wkocher)
We have pushed to stage, and all the shortcuts also appear to be working correctly there. We should be pushing to prod within a few hours I believe.
We pushed to prod about a half hour ago, and all shortcuts still appear to be working correctly and consistent with stage.
We seem to have positive reviews from sheriffs on production, so I'm tentatively marking this fixed for now :) I will mark as verified once it has lived on production for a few more days.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Attachment #8565082 - Flags: review?(cdawson) → review+
The refactor has lived on production for about three days with heavy sheriff use, so I'm willing to mark this bug verified fixed :)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: