Summary

[WIP] Keyboard shortcuts for expand & collapse diff

Review Request #10951 — Created March 12, 2020 and updated April 5, 2020, 12:30 p.m.

Information

Owner

hxqlin

Repository

Review Board

Branch

master

Bugs

Depends On

10925

Commit

a4627d4...

Reviewers

Groups

reviewboard

People

Description

This patch adds keyboard shortcuts to expand & collapse the diff in a review
request and in a diff fragment in a review. Expand is triggered by the e
key and collapse is triggered by the w key (the c key may seem more
natural, but it's taken already by another shortcut so w was chosen for its
proximity to the e key for ease of use between them).

For the diff viewer, expand and collapse buttons are now tracked as anchors
in RB.DiffViewerPageView. This allows us to find the next button on the
page from the currently selected anchor and trigger it when a keyboard
shortcut is used. The e and w keys are added to the existing key
bindings mixin object to track those key presses and trigger the right
button.

For diff fragments, there can only be one set of expand buttons and one
collapse button in any given fragment. If the fragment is focused, the e
and w keys will trigger their respective expand and collapse buttons. This
is implemented by using the key bindings mixin in RB.DiffFragmentView. The
shortcut to expand the fragment defaults to expanding the diff by 20 lines,
which is achieved by adding a modifier to desired expand button.

Testing Done

Manual testing & unit tests for the e and w key events in both the diff
fragment and diff viewer.

Issues

Description	From	Last Updated
W291 trailing whitespace	reviewbot	March 20, 2020, 5:32 p.m.
Can you update this to describe what this means? "Next" from what viewpoint? Same below.	chipx86	April 3, 2020, 12:17 p.m.
Arguments should align.	chipx86	April 3, 2020, 12:17 p.m.
Arguments should align.	chipx86	April 3, 2020, 12:17 p.m.
Blank line between a statement and a block.	chipx86	April 3, 2020, 12:18 p.m.
Function doc summaries must be a single unwrapped line. Always good to expand on meanings in subsequent paragraphs.	chipx86	April 3, 2020, 12:21 p.m.
Querying based on attributes is slow, and that's very important here, because such a query could end up taking an …	chipx86	April 1, 2020, 2:12 p.m.
This can be: label = `'${key}'`;	chipx86	April 3, 2020, 12:27 p.m.

flake8 passed.

JSHint passed.

Change Summary:

Implement keyboard shortcuts in diff fragments

Commit:

ff882a74102785befef7974d3701cb615ac294aa

ae16814debfad9675f35e5b3fc73f8db50dc09ad

Diff:

Revision 2 (+87 -18)

Show changes

	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/views/diffFragmentView.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html
	reviewboard/templates/diffviewer/expand_link.html
	reviewboard/templates/reviews/diff_comment_fragment.html
	reviewboard/templates/reviews/expand_link.html
	1 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/templatetags/reviewtags.py (Diff revision 2)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```

Change Summary:

Add unit tests

Commit:

ae16814debfad9675f35e5b3fc73f8db50dc09ad

b3b0589377ddddd40fc0571df4543d70be6f96d8

Diff:

Revision 3 (+133 -18)

Show changes

	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js
	reviewboard/static/rb/js/views/diffFragmentView.es6.js
	reviewboard/static/rb/js/views/tests/diffFragmentViewTests.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html
	reviewboard/templates/diffviewer/expand_link.html
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Fix it!

reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you update this to describe what this means? "Next" from what viewpoint?
Same below.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Arguments should align.
```
reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Arguments should align.
```
reviewboard/static/rb/js/views/diffFragmentView.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line between a statement and a block.
```

reviewboard/static/rb/js/views/diffFragmentView.es6.js (Diff revision 3)

The issue has been resolved. Show all issues

Function doc summaries must be a single unwrapped line.

Always good to expand on meanings in subsequent paragraphs.

reviewboard/static/rb/js/views/diffFragmentView.es6.js (Diff revision 3)

An issue was opened. Show all issues

Querying based on attributes is slow, and that's very important here, because such a query could end up taking an actual noticeable amount of time for larger diffs. It'd be better to specifically look for and cache any such thing up-front, if we can, or to at least narrow down first by something more specific.

Classes are also faster to query.

Also, how is this finding the "next" one? This looks like it'd find all of them and click all of them. (Same with others.)

hxqlin April 3, 2020, 12:55 p.m.

https://reviews.reviewboard.org/r/10951/#comment50645 - updated the selector to use a class instead of an attribute, but the reasoning is the same

hxqlin April 5, 2020, 12:30 p.m.

Sorry this is a bit hard to follow - this is the primary comment I meant to reply to. I'll re-explain everything here. So after all the updated changes, here's why this would only click one button. You said that there will only be one collapse button or set of expand buttons in a diff fragment, so if we look for buttons within a diff fragment, we know we'll have selected a button corresponding to the current fragment. We also only trigger the shortcut if a particular diff fragment is focused so the buttons won't be triggered in the wrong fragment.

For expand buttons, there will be four buttons in a diff fragment in total but we only want to trigger one of them for the shortcut. I added a new modifier class -is-default to the expand button that I chose to be the default for this shortcut, which is the one that will expand the diff by 20 lines. I chose this one since I thought it would be the most common use case out of the expand buttons, but feel free to correct me. So when we look for an expand button with that modifier within a diff fragment, we'll only find one.

For the collapse button, there will only be one of them in a diff fragment if there's one at all. Again, since we only trigger the shortcut for the current fragment if it's focused and we search for the button within the focused fragment, it will only find the collapse button for the current fragment if it exists and trigger it or do nothing at all.

Hope that made more sense!

reviewboard/static/rb/js/views/tests/diffFragmentViewTests.es6.js (Diff revision 3)
The issue has been resolved. Show all issues
```
This can be:

label = `'${key}'`;
```

chipx86 April 4, 2020, 10:39 p.m.

I think you accidentally posted a new review, instead of replying to mine, so I'm not sure exactly what everything pertains to. When you comment in the diff viewer, you're creating a new review, not replying.

reviewboard/static/rb/js/views/diffFragmentView.es6.js (Diff revision 3)

I added a comment at the bit of code that adds this [data-default-expand-btn="true"] attribute to the button explaining how this button will be the only one. The reasoning behind how these will only affect one button is because you said there will only be one collapse button or set of expand buttons in a diff fragment, so finding and clicking one within the element will only affect the current fragment.

reviewboard/static/rb/js/views/tests/diffFragmentViewTests.es6.js (Diff revision 3)

I forgot to leave some comments regarding this function - it's actually copied over from diffViewerPageViewTests.es6.js. Would it be better to pull this out into a util file somewhere? I wasn't sure if that was the right move or where to put it.

chipx86 April 4, 2020, 10:39 p.m.

It's fine to duplicate it right now. It would be ideal for us to have some common function someplace, but that'd be a more standalone task, and it isn't worth spending time on right now.

reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 3)

part of the dependent change

reviewboard/templates/reviews/diff_comment_fragment.html (Diff revision 3)

part of the dependent change

reviewboard/templates/reviews/diff_comment_fragment.html (Diff revision 3)

part of the dependent change

reviewboard/templates/reviews/diff_comment_fragment.html (Diff revision 3)

This is the only expand button that will have the data-default-expand-btn="true" in a particular diff fragment, so it will be the only button that's affected by the keyboard shortcut.

Change Summary:

Fix issues from review and update description.

Description:

		This patch adds keyboard shortcuts to expand & collapse the diff in a review
~		request. Expand is triggered by the `e` key and collapse is triggered by the `w`
~		key (the `c` key may seem more natural, but it's taken already by another shortcut
~		so `w` was chosen for its proximity to the `e` key for ease of use between them).
	~	request and in a diff fragment in a review. Expand is triggered by the `e`
	~	key and collapse is triggered by the `w` key (the `c` key may seem more
	~	natural, but it's taken already by another shortcut so `w` was chosen for its
	+	proximity to the `e` key for ease of use between them).

~		This change is built on top of the changes in https://reviews.reviewboard.org/r/10925/,
~		which make the collapse diff button tabbable.
	~	For the diff viewer, expand and collapse buttons are now tracked as anchors
	~	in `RB.DiffViewerPageView`. This allows us to find the next button on the
	+	page from the currently selected anchor and trigger it when a keyboard
	+	shortcut is used. The `e` and `w` keys are added to the existing key
	+	bindings mixin object to track those key presses and trigger the right
	+	button.
	+
	+	For diff fragments, there can only be one set of expand buttons and one
	+	collapse button in any given fragment. If the fragment is focused, the `e`
	+	and `w` keys will trigger their respective expand and collapse buttons. This
	+	is implemented by using the key bindings mixin in `RB.DiffFragmentView`. The
	+	shortcut to expand the fragment defaults to expanding the diff by 20 lines,
	+	which is achieved by adding a modifier to desired expand button.

Testing Done:

~		Manual testing
	~	Manual testing & unit tests for the `e` and `w` key events in both the diff
	+	fragment and diff viewer.

Commit:

b3b0589377ddddd40fc0571df4543d70be6f96d8

a4627d4e91fcfef95358e9af7b7da926c82b7680

Diff:

Revision 4 (+135 -19)

Show changes

	reviewboard/reviews/templatetags/reviewtags.py
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
	reviewboard/static/rb/js/pages/views/tests/diffViewerPageViewTests.es6.js
	reviewboard/static/rb/js/views/diffFragmentView.es6.js
	reviewboard/static/rb/js/views/tests/diffFragmentViewTests.es6.js
	reviewboard/templates/diffviewer/diff_file_fragment.html
	reviewboard/templates/diffviewer/expand_link.html
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.