Incremental improvement to GitTab focus

I've done a pass through the GitTab components and improved the focus
management code somewhat. It's still ref soup, imperative, and pretty
verbose, but at least now it's a bit more internally consistent.

GitTab components that may receive focus implement four methods:

* `getFocus(element)` returns the logical focus symbol corresponding to
  a DOM element, or null if the element is unrecognized.
* `setFocus(symbol)` brings focus to the DOM element corresponding to a
  logical focus symbol. It returns true if an element was found and
  focused successfully and false otherwise.
* `advanceFocusFrom(lastFocus)` returns a Promise that resolves to the
  logical focus symbol after a given symbol.
* `retreatFocusFrom(lastFocus)` returns a Promise that resolves to the
  logical focus symbol before a given symbol.

keymaps/git.cson

@@ -39,6 +39,7 @@
 '.github-CommitView-editor atom-text-editor:not([mini])':
   'cmd-enter': 'github:commit'
   'ctrl-enter': 'github:commit'
+  'tab': 'core:focus-next'
   'shift-tab': 'core:focus-previous'
 
 '.github-CommitView-commitPreview':

lib/controllers/commit-controller.js

@@ -76,7 +76,7 @@ export default class CommitController extends React.Component {
       }),
       this.props.workspace.onDidDestroyPaneItem(async ({item}) => {
         if (this.props.repository.isPresent() && item.getPath && item.getPath() === this.getCommitMessagePath() &&
-            this.getCommitMessageEditors().length === 0) {
+          this.getCommitMessageEditors().length === 0) {
           // we closed the last editor pointing to the commit message file
           try {
             this.commitMessageBuffer.setText(await fs.readFile(this.getCommitMessagePath(), {encoding: 'utf8'}));
@@ -252,24 +252,20 @@ export default class CommitController extends React.Component {
     this.grammarSubscription.dispose();
   }
 
-  rememberFocus(event) {
-    return this.refCommitView.map(view => view.rememberFocus(event)).getOr(null);
+  getFocus(element) {
+    return this.refCommitView.map(view => view.getFocus(element)).getOr(null);
   }
 
   setFocus(focus) {
     return this.refCommitView.map(view => view.setFocus(focus)).getOr(false);
   }
 
-  advanceFocus(...args) {
-    return this.refCommitView.map(view => view.advanceFocus(...args)).getOr(false);
+  advanceFocusFrom(...args) {
+    return this.refCommitView.map(view => view.advanceFocusFrom(...args)).getOr(false);
   }
 
-  retreatFocus(...args) {
-    return this.refCommitView.map(view => view.retreatFocus(...args)).getOr(false);
-  }
-
-  hasFocusAtBeginning() {
-    return this.refCommitView.map(view => view.hasFocusAtBeginning()).getOr(false);
+  retreatFocusFrom(...args) {
+    return this.refCommitView.map(view => view.retreatFocusFrom(...args)).getOr(false);
   }
 
   toggleCommitPreview() {

lib/controllers/git-tab-controller.js

@@ -338,7 +338,7 @@ export default class GitTabController extends React.Component {
   }
 
   rememberLastFocus(event) {
-    this.lastFocus = this.refView.map(view => view.rememberFocus(event)).getOr(null) || GitTabView.focus.STAGING;
+    this.lastFocus = this.refView.map(view => view.getFocus(event.target)).getOr(null) || GitTabView.focus.STAGING;
   }
 
   restoreFocus() {

lib/controllers/recent-commits-controller.js

@@ -94,11 +94,19 @@ export default class RecentCommitsController extends React.Component {
     }
   }
 
-  rememberFocus(event) {
-    return this.refView.map(view => view.rememberFocus(event)).getOr(null);
+  getFocus(element) {
+    return this.refView.map(view => view.getFocus(element)).getOr(null);
   }
 
   setFocus(focus) {
     return this.refView.map(view => view.setFocus(focus)).getOr(false);
   }
+
+  advanceFocusFrom(focus) {
+    return this.refView.map(view => view.advanceFocusFrom(focus)).getOr(Promise.resolve(false));
+  }
+
+  retreatFocusFrom(focus) {
+    return this.refView.map(view => view.retreatFocusFrom(focus)).getOr(Promise.resolve(false));
+  }
 }

lib/views/commit-view.js

@@ -7,6 +7,8 @@ import Select from 'react-select';
 import Tooltip from '../atom/tooltip';
 import AtomTextEditor from '../atom/atom-text-editor';
 import CoAuthorForm from './co-author-form';
+import RecentCommitsView from './recent-commits-view';
+import StagingView from './staging-view';
 import Commands, {Command} from '../atom/commands';
 import RefHolder from '../models/ref-holder';
 import Author from '../models/author';
@@ -30,6 +32,10 @@ export default class CommitView extends React.Component {
     COMMIT_BUTTON: Symbol('commit-button'),
   };
 
+  static firstFocus = CommitView.focus.COMMIT_PREVIEW_BUTTON;
+
+  static lastFocus = CommitView.focus.COMMIT_BUTTON;
+
   static propTypes = {
     workspace: PropTypes.object.isRequired,
     config: PropTypes.object.isRequired,
@@ -569,15 +575,7 @@ export default class CommitView extends React.Component {
     return this.refRoot.map(element => element.contains(document.activeElement)).getOr(false);
   }
 
-  hasFocusEditor() {
-    return this.refEditorComponent.map(editor => editor.contains(document.activeElement)).getOr(false);
-  }
-
-  hasFocusAtBeginning() {
-    return this.refCommitPreviewButton.map(button => button.contains(document.activeElement)).getOr(false);
-  }
-
-  getFocus(element = document.activeElement) {
+  getFocus(element) {
     if (this.refCommitPreviewButton.map(button => button.contains(element)).getOr(false)) {
       return CommitView.focus.COMMIT_PREVIEW_BUTTON;
     }
@@ -601,10 +599,6 @@ export default class CommitView extends React.Component {
     return null;
   }
 
-  rememberFocus(event) {
-    return this.getFocus(event.target);
-  }
-
   setFocus(focus) {
     let fallback = false;
     const focusElement = element => {
@@ -657,77 +651,65 @@ export default class CommitView extends React.Component {
     return false;
   }
 
-  advanceFocus(event) {
+  advanceFocusFrom(focus) {
     const f = this.constructor.focus;
-    const current = this.getFocus();
-    if (current === f.EDITOR) {
-      // Let the editor handle it
-      return true;
-    }
 
     let next = null;
-    switch (current) {
+    switch (focus) {
     case f.COMMIT_PREVIEW_BUTTON:
       next = f.EDITOR;
       break;
+    case f.EDITOR:
+      if (this.state.showCoAuthorInput) {
+        next = f.COAUTHOR_INPUT;
+      } else if (this.props.isMerging) {
+        next = f.ABORT_MERGE_BUTTON;
+      } else {
+        next = f.COMMIT_BUTTON;
+      }
+      break;
     case f.COAUTHOR_INPUT:
       next = this.props.isMerging ? f.ABORT_MERGE_BUTTON : f.COMMIT_BUTTON;
       break;
     case f.ABORT_MERGE_BUTTON:
       next = f.COMMIT_BUTTON;
       break;
     case f.COMMIT_BUTTON:
-      // End of tab navigation. Prevent cycling.
-      event.stopPropagation();
-      return true;
+      next = RecentCommitsView.firstFocus;
+      break;
     }
 
-    if (next !== null) {
-      this.setFocus(next);
-      event.stopPropagation();
-
-      return true;
-    } else {
-      return false;
-    }
+    return Promise.resolve(next);
   }
 
-  retreatFocus(event) {
+  retreatFocusFrom(focus) {
     const f = this.constructor.focus;
-    const current = this.getFocus();
 
-    let next = null;
-    switch (current) {
+    let previous = null;
+    switch (focus) {
     case f.COMMIT_BUTTON:
       if (this.props.isMerging) {
-        next = f.ABORT_MERGE_BUTTON;
+        previous = f.ABORT_MERGE_BUTTON;
       } else if (this.state.showCoAuthorInput) {
-        next = f.COAUTHOR_INPUT;
+        previous = f.COAUTHOR_INPUT;
       } else {
-        next = f.EDITOR;
+        previous = f.EDITOR;
       }
       break;
     case f.ABORT_MERGE_BUTTON:
-      next = this.state.showCoAuthorInput ? f.COAUTHOR_INPUT : f.EDITOR;
+      previous = this.state.showCoAuthorInput ? f.COAUTHOR_INPUT : f.EDITOR;
       break;
     case f.COAUTHOR_INPUT:
-      next = f.EDITOR;
+      previous = f.EDITOR;
       break;
     case f.EDITOR:
-      next = f.COMMIT_PREVIEW_BUTTON;
+      previous = f.COMMIT_PREVIEW_BUTTON;
       break;
     case f.COMMIT_PREVIEW_BUTTON:
-      // Allow the GitTabView to retreat focus back to the last StagingView list.
-      return false;
+      previous = StagingView.lastFocus;
+      break;
     }
 
-    if (next !== null) {
-      this.setFocus(next);
-      event.stopPropagation();
-
-      return true;
-    } else {
-      return false;
-    }
+    return Promise.resolve(previous);
   }
 }

lib/views/git-tab-view.js

@@ -224,35 +224,22 @@ export default class GitTabView extends React.Component {
     this.props.initializeRepo(initPath);
   }
 
-  rememberFocus(event) {
-    let currentFocus = null;
-
-    currentFocus = this.props.refStagingView.map(view => view.rememberFocus(event)).getOr(null);
-
-    if (!currentFocus) {
-      currentFocus = this.refCommitController.map(controller => controller.rememberFocus(event)).getOr(null);
-    }
-
-    if (!currentFocus) {
-      currentFocus = this.refRecentCommitsController.map(controller => controller.rememberFocus(event)).getOr(null);
+  getFocus(element) {
+    for (const ref of [this.props.refStagingView, this.refCommitController, this.refRecentCommitsController]) {
+      const focus = ref.map(sub => sub.getFocus(element)).getOr(null);
+      if (focus !== null) {
+        return focus;
+      }
     }
-
-    return currentFocus;
+    return null;
   }
 
   setFocus(focus) {
-    if (this.props.refStagingView.map(view => view.setFocus(focus)).getOr(false)) {
-      return true;
-    }
-
-    if (this.refCommitController.map(controller => controller.setFocus(focus)).getOr(false)) {
-      return true;
-    }
-
-    if (this.refRecentCommitsController.map(controller => controller.setFocus(focus)).getOr(false)) {
-      return true;
+    for (const ref of [this.props.refStagingView, this.refCommitController, this.refRecentCommitsController]) {
+      if (ref.map(sub => sub.setFocus(focus)).getOr(false)) {
+        return true;
+      }
     }
-
     return false;
   }
 
@@ -261,39 +248,34 @@ export default class GitTabView extends React.Component {
   }
 
   async advanceFocus(evt) {
-    // Advance focus within the CommitView if it's there
-    if (this.refCommitController.map(c => c.advanceFocus(evt)).getOr(false)) {
-      return;
-    }
+    const currentFocus = this.getFocus(document.activeElement);
+    let nextSeen = false;
 
-    // Advance focus to the next staging view list, if it's there
-    if (await this.props.refStagingView.map(view => view.activateNextList()).getOr(false)) {
-      evt.stopPropagation();
-      return;
-    }
-
-    // Advance focus from the staging view lists to the CommitView
-    if (this.refCommitController.map(c => c.setFocus(GitTabView.focus.COMMIT_PREVIEW_BUTTON)).getOr(false)) {
-      evt.stopPropagation();
+    for (const subHolder of [this.props.refStagingView, this.refCommitController, this.refRecentCommitsController]) {
+      const next = await subHolder.map(sub => sub.advanceFocusFrom(currentFocus)).getOr(null);
+      if (next !== null && !nextSeen) {
+        nextSeen = true;
+        evt.stopPropagation();
+        if (next !== currentFocus) {
+          this.setFocus(next);
+        }
+      }
     }
   }
 
   async retreatFocus(evt) {
-    // Retreat focus within the CommitView if it's there
-    if (this.refCommitController.map(c => c.retreatFocus(evt)).getOr(false)) {
-      return;
-    }
+    const currentFocus = this.getFocus(document.activeElement);
+    let previousSeen = false;
 
-    if (this.refCommitController.map(c => c.hasFocusAtBeginning()).getOr(false)) {
-      // Retreat focus from the beginning of the CommitView to the end of the StagingView
-      if (await this.props.refStagingView.map(view => view.activateLastList()).getOr(null)) {
-        this.setFocus(GitTabView.focus.STAGING);
+    for (const subHolder of [this.refRecentCommitsController, this.refCommitController, this.props.refStagingView]) {
+      const previous = await subHolder.map(sub => sub.retreatFocusFrom(currentFocus)).getOr(null);
+      if (previous !== null && !previousSeen) {
+        previousSeen = true;
         evt.stopPropagation();
+        if (previous !== currentFocus) {
+          this.setFocus(previous);
+        }
       }
-    } else if (await this.props.refStagingView.map(c => c.activatePreviousList()).getOr(null)) {
-      // Retreat focus within the StagingView
-      this.setFocus(GitTabView.focus.STAGING);
-      evt.stopPropagation();
     }
   }
 

lib/views/recent-commits-view.js

@@ -7,6 +7,7 @@ import {emojify} from 'node-emoji';
 import Commands, {Command} from '../atom/commands';
 import RefHolder from '../models/ref-holder';
 
+import CommitView from './commit-view';
 import Timeago from './timeago';
 
 class RecentCommitView extends React.Component {
@@ -130,6 +131,10 @@ export default class RecentCommitsView extends React.Component {
     RECENT_COMMIT: Symbol('recent_commit'),
   };
 
+  static firstFocus = RecentCommitsView.focus.RECENT_COMMIT;
+
+  static lastFocus = RecentCommitsView.focus.RECENT_COMMIT;
+
   constructor(props) {
     super(props);
     this.refRoot = new RefHolder();
@@ -143,8 +148,8 @@ export default class RecentCommitsView extends React.Component {
     return false;
   }
 
-  rememberFocus(event) {
-    return this.refRoot.map(element => element.contains(event.target)).getOr(false)
+  getFocus(element) {
+    return this.refRoot.map(e => e.contains(element)).getOr(false)
       ? this.constructor.focus.RECENT_COMMIT
       : null;
   }
@@ -198,4 +203,20 @@ export default class RecentCommitsView extends React.Component {
   }
 
   openSelectedCommit = () => this.props.openCommit({sha: this.props.selectedCommitSha, preserveFocus: false})
+
+  advanceFocusFrom(focus) {
+    if (focus === this.constructor.focus.RECENT_COMMIT) {
+      return Promise.resolve(this.constructor.focus.RECENT_COMMIT);
+    }
+
+    return Promise.resolve(null);
+  }
+
+  retreatFocusFrom(focus) {
+    if (focus === this.constructor.focus.RECENT_COMMIT) {
+      return Promise.resolve(CommitView.lastFocus);
+    }
+
+    return Promise.resolve(null);
+  }
 }