keyboard shortcuts to move a tab left or right #11313

petemill · 2017-10-06T07:49:15Z

I created this PR as a precursor to some tab enhancements I'm working on with regard to sorting animation and realtime sorting operations during dragging. Following from this PR, which adds the command to change the sort order, I'll be able to submit the PR to add animation to the operation. These same shortcuts are present in other browsers by default (Firefox on all platforms, and Chrome on linux).

Keyboard shortcuts are:

ctrl-shift-pageup : moves the current tab right, and the next tab left (swaps)
ctrl-shift-pagedown: moves the current tab left, and the previous tab right (swaps)

Note that if you're on a mac keyboard with no pageup or pagedown keys, those are accessed via fn-up or fn-down. So the shortcuts become ctrl-shift-fn-up and ctrl-shift-fn-down

Auditors:
@bsclifton
@cezaraugusto

Would be great to hear from you guys if the way this is architected is best practice. Thoughts I've had:

I'm not often a fan of mixing reducers with side-effect causing action dispatching, but I don't see an alternative architecture present at the moment
I'm not 100% clear yet on the signature an action needs in order to be passed to and reduced by the correct window, dispatch-able from both the browser process (from the keyboard shortcut) and the renderer process (from the web driven tests) - but after a bit or trial and error the way it is here works.

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Test Plan:

auto

Run the automated tests for this feature with npm run test -- --bail --grep="tab order index change"

manual

With one tab, one window

Open a single window with a single tab
Observe that ctrl-shift-pageup and ctrl-shift-pagedown do nothing

With multiple tabs, one window

Open many more tabs
Observe that when a tab is active, ctrl-shift-pagedown moves the tab forward, as long as it is not the last tab
Observe that when a tab is active, ctrl-shift-pageup moves the tab backwards, as long as it is not the first tab

With multiple tabs, two windows

Open two windows
Open multiple tabs in each window
Activate a tab in one window
Observe that moving the tab using ctrl-shift-pagedown or ctrl-shift-pageup does not affect any tabs in the second window

With pinned tabs

Open multiple tabs
Pin some of those tabs
Observe that moving the unpinned tabs does not move the tabs to the pinned area
Observe that moving the pinned tabs does not move the tabs to the unpinned area

With multiple tab pages

Open enough tabs to split in to multiple tab pages
Activate a tab on the first page
Observe that pressing ctrl-shift-pagedown enough times will move the tab to the second page

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

codecov-io · 2017-10-06T08:10:26Z

Codecov Report

Merging #11313 into master will decrease coverage by 0.06%.
The diff coverage is 10.71%.

@@            Coverage Diff             @@
##           master   #11313      +/-   ##
==========================================
- Coverage    52.6%   52.54%   -0.07%     
==========================================
  Files         268      268              
  Lines       25237    25273      +36     
  Branches     4024     4031       +7     
==========================================
+ Hits        13277    13280       +3     
- Misses      11960    11993      +33

Flag	Coverage Δ
#unittest	`52.54% <10.71%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
js/constants/windowConstants.js	`100% <ø> (ø)`	⬆️
js/actions/windowActions.js	`4.85% <0%> (-0.05%)`	⬇️
app/localShortcuts.js	`20.68% <11.11%> (-3.76%)`	⬇️
js/stores/windowStore.js	`27.01% <11.76%> (-0.57%)`	⬇️

luixxiul · 2017-10-06T15:43:09Z

Thanks for providing the nice test plan (also automated tests code)!

If automated tests should cover every possible scenarios, would you please add QA/no-qa-needed to #11310? Thanks!

cezaraugusto · 2017-10-13T18:36:18Z

app/localShortcuts.js

+  electronLocalshortcut.register(win, 'Ctrl+Shift+PageDown', () => {
+    const win = BrowserWindow.getFocusedWindow()
+    if (win) {
+      windowActions.tabMoveIncrementalRequested(win.id, true)


if moveNext defaults to true I think we can skip the second argument

cezaraugusto · 2017-10-13T18:37:38Z

js/stores/windowStore.js

+      const sourceFrame = frameStateUtil.getActiveFrame(windowState)
+      const isPinned = sourceFrame.get('pinnedLocation')
+      const framesCanMoveWithin = isPinned ? frameStateUtil.getPinnedFrames(windowState) : frameStateUtil.getNonPinnedFrames(windowState)
+      const frameGroupIdx = framesCanMoveWithin.indexOf(sourceFrame)


nit: I'd prefer the verbose name w/ frameGroupIndex instead

same for other similar named constants

cezaraugusto · 2017-10-13T18:42:14Z

test/lib/selectors.js

@@ -7,7 +7,9 @@ module.exports = {
  activeTab: '[data-test-active-tab]',
  activeTabTitle: '[data-test-active-tab] [data-test-id="tabTitle"]',
  activeTabFavicon: '[data-test-active-tab] [data-test-favicon]',
+  pinnedTabs: '.pinnedTabs',


can we make use of data attr instead? something like data-pinned-tab. We're trying to avoid tests w/ CSS selectors so Aphrodite changes could less likely break tests

sure - I had just copied what was there originally but makes sense to make the positive changes now

cezaraugusto · 2017-10-13T18:42:49Z

test/lib/selectors.js

  pinnedTabsTabs: '.pinnedTabs [data-test-id="tab"]',
+  tabs: '.tabs',


same as above

cezaraugusto

unable to test on macOS, left some comments

cezaraugusto · 2017-10-13T18:50:46Z

@petemill is this supposed to work on mac? if not please put OS specific labels. I guess not, given ctrl+pageUp conflicts w/ another system shortcut but please specify which should.

petemill · 2017-10-13T19:32:32Z

@cezaraugusto definitely for macOS. Works for me with ctrl+shift+pageup (did you miss the shift?)

petemill · 2017-10-13T21:52:28Z

@cezaraugusto addressed your feedback, rebased on master, and pushed replacement commit

@bsclifton

…order up or down one position. Fix brave#11310 Keyboard shortcut is ctrl-shift-pageup and ctrl-shift-pagedown Auditors: @bsclifton @cezaraugusto

cezaraugusto

lgtm. Please just prefer using appConstants instead of windowConstants and move the windowAction to the appAction as well as we're moving in that direction. Please update tests too to reflect that.

I'm going to approve, once you make the changes please merge. great job!

petemill · 2017-10-28T08:04:13Z

@cezaraugusto changes you ask for cause the tests to fail as the window reducer never receives the action. I assume the problem is that the appStore won't forward the action to the window, since the action came from a window in the first place, and is targetted to one specific window? My assumption is because it works fine in the app, where the action is dispatched from the browser process, but in the tests, the action is dispatched from the window process using devTools('appActions'). Perhaps we can wait for the fuller refactor that has the appStore access to window state before trying to hack this in to work? Otherwise I can look at it again with clearer eyes.

keyboard shortcuts to move a tab left or right

cezaraugusto · 2017-10-31T18:04:29Z

master ec3074a
0.21.x bfd9145

petemill added feature/keyboard-shortcuts feature/tabsbar usability labels Oct 6, 2017

petemill self-assigned this Oct 6, 2017

petemill requested review from cezaraugusto and bsclifton October 6, 2017 07:49

luixxiul added the needs-info Another team member needs information from the PR/issue opener. label Oct 6, 2017

petemill force-pushed the keyboard-sort-tabs branch from c7ece9b to 3a8d0f9 Compare October 10, 2017 19:33

cezaraugusto reviewed Oct 13, 2017

View reviewed changes

test/lib/selectors.js Outdated

pinnedTabsTabs: '.pinnedTabs [data-test-id="tab"]',

tabs: '.tabs',

Copy link

Contributor

cezaraugusto Oct 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

cezaraugusto reviewed Oct 13, 2017

View reviewed changes

petemill force-pushed the keyboard-sort-tabs branch from 3a8d0f9 to 29e33d7 Compare October 13, 2017 21:51

cezaraugusto added PR/pending-review and removed needs-info Another team member needs information from the PR/issue opener. labels Oct 14, 2017

Create new commands and keyboard shortcuts to change a tab's display …

fdfb0b5

…order up or down one position. Fix brave#11310 Keyboard shortcut is ctrl-shift-pageup and ctrl-shift-pagedown Auditors: @bsclifton @cezaraugusto

petemill force-pushed the keyboard-sort-tabs branch from 29e33d7 to fdfb0b5 Compare October 16, 2017 23:45

luixxiul added this to the Backlog milestone Oct 26, 2017

petemill changed the title ~~keyboard shortcuts to change a tab's display order up or down~~ keyboard shortcuts to move a tab left or right Oct 28, 2017

cezaraugusto approved these changes Oct 28, 2017

View reviewed changes

luixxiul added the QA/test-plan-specified label Oct 31, 2017

alexwykoff modified the milestones: Backlog, 0.21.x (Developer Channel) Oct 31, 2017

cezaraugusto merged commit ec3074a into brave:master Oct 31, 2017

cezaraugusto added a commit that referenced this pull request Oct 31, 2017

Merge pull request #11313 from petemill/keyboard-sort-tabs

bfd9145

keyboard shortcuts to move a tab left or right

petemill mentioned this pull request Oct 31, 2017

No keyboard shortcuts to re-order tabs #11310

Closed

bsclifton removed the PR/pending-review label Dec 22, 2017

This was referenced Feb 22, 2018

bad session state can result in permanently borked profile #13261

Closed

pinned tabs do not retain order when moved with keyboard shortcut #13264

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

keyboard shortcuts to move a tab left or right #11313

keyboard shortcuts to move a tab left or right #11313

petemill commented Oct 6, 2017 •

edited

Loading

codecov-io commented Oct 6, 2017 •

edited

Loading

luixxiul commented Oct 6, 2017

cezaraugusto Oct 13, 2017

cezaraugusto Oct 13, 2017

cezaraugusto Oct 13, 2017

cezaraugusto Oct 13, 2017

petemill Oct 13, 2017

cezaraugusto Oct 13, 2017

cezaraugusto left a comment

cezaraugusto commented Oct 13, 2017

petemill commented Oct 13, 2017

petemill commented Oct 13, 2017

cezaraugusto left a comment •

edited

Loading

petemill commented Oct 28, 2017

cezaraugusto commented Oct 31, 2017

		pinnedTabsTabs: '.pinnedTabs [data-test-id="tab"]',
		tabs: '.tabs',

keyboard shortcuts to move a tab left or right #11313

keyboard shortcuts to move a tab left or right #11313

Conversation

petemill commented Oct 6, 2017 • edited Loading

auto

manual

With one tab, one window

With multiple tabs, one window

With multiple tabs, two windows

With pinned tabs

With multiple tab pages

codecov-io commented Oct 6, 2017 • edited Loading

Codecov Report

luixxiul commented Oct 6, 2017

cezaraugusto Oct 13, 2017

Choose a reason for hiding this comment

cezaraugusto Oct 13, 2017

Choose a reason for hiding this comment

cezaraugusto Oct 13, 2017

Choose a reason for hiding this comment

cezaraugusto Oct 13, 2017

Choose a reason for hiding this comment

petemill Oct 13, 2017

Choose a reason for hiding this comment

cezaraugusto Oct 13, 2017

Choose a reason for hiding this comment

cezaraugusto left a comment

Choose a reason for hiding this comment

cezaraugusto commented Oct 13, 2017

petemill commented Oct 13, 2017

petemill commented Oct 13, 2017

cezaraugusto left a comment • edited Loading

Choose a reason for hiding this comment

petemill commented Oct 28, 2017

cezaraugusto commented Oct 31, 2017

petemill commented Oct 6, 2017 •

edited

Loading

codecov-io commented Oct 6, 2017 •

edited

Loading

cezaraugusto left a comment •

edited

Loading