1057338 - Next/previous keyboard shortcuts stop working after giving job details panel focus

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P1)

Description

[Ed Morley [:emorley]]

1) Open a repo with >1 unclassified failure
2) Press the 'j' key to select the next unclassified failure
3) Middle (or ctrl) mouse click the structured log viewer link bottom left of the UI (thereby giving the job details panel focus)
4) Repeat step #2

Expected:
'j' key still works.

Actual:
Nothing happens until focus is given back to where the results/pushes are displayed.

Comment 1

[Jonathan French (:jfrench)]

Interestingly this doesn't happen on Google Chrome, but does on Firefox - at least on Windows.

It also appears to be the act giving the job panels' top navbar focus, rather than the job panel content panes below it. For example if you click in bottom left or bottom right job content panes and use 'j' or 'k' it works.

Comment 2

[Ed Morley [:emorley]]

P1 since as part of classifying failures, the job details panel is given focus, so this breaks the classification workflow UX in a big way (a la bug 1059368).

Comment 3

[William Lachance (:wlach)]

I'll take this one as well.

Comment 4

[William Lachance (:wlach)]

Attachment: failure-summary-reduced-padding.png

Updated the pull request to reduce padding on failure request lines. What do you think now?

Comment 5

[Ed Morley [:emorley]]

Comment on attachment 8480792 [details]
failure-summary-reduced-padding.png

That's great - thank you :-)

Comment 6

[Ed Morley [:emorley]]

Comment on attachment 8480792 [details]
failure-summary-reduced-padding.png

(wrong bug)

Comment 7

[William Lachance (:wlach)]

Attachment: PR to fix issue

Attached a pull request that fixes the issue. We should only ignore the key events inside input fields, I think.

Comment 8

[Jonathan Eads ( :jeads )]

Comment on attachment 8480802 [details] [review]
PR to fix issue

This looks good. The "(FIXME: explain?)" comment should be removed. The only html elements where the conditional makes sense are INPUT. Will clean up in a different PR so we can push this change a long with several others to dev/stage/production.

Comment 9

[William Lachance (:wlach)]

Attachment: A few more updates

This builds on the previous work, ignoring keypresses in other kinds of input fields and if the element has editable content. I don't think any of those cases currently apply, but it doesn't hurt to be forward thinking. Ideas from the mousetrap library (https://github.com/ccampbell/mousetrap)

Comment 10

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/7d9c6d3c3bfb7dbc2d92a8105d61e5fa02117731
Bug 1057338 - Only ignore key events when user is inside an input field

https://github.com/mozilla/treeherder/commit/92f17dee805fb8b6030a8fee6a0b951063296945
Merge pull request #135 from wlach/1057338

 	Bug 1057338 - Only ignore key events when user is inside an input field

https://github.com/mozilla/treeherder/commit/2ca8384107730f90e7e3f659d3e2efde12deb1da
Bug 1057338 - Update keyboard event ignoring code

use heuristics from mousetrap library (https://github.com/ccampbell/mousetrap):
* Also ignore keyboard events if in other types of input elements
* Ignore keyboard events if element's content is editable