Retain keyboard cursor state across more inline edit operations in Differential

Summary:
Ref T12634. Fixes T8131. Currently, most edit operations (edit, reply, collapse, mark done) lose the keyboard cursor state.

Instead, bind the state more tighlty to the inline object itself (instead of the rows which happen to be in the document), and then do a bit of recalculation to try to keep it selected across edits.

Test Plan:
  - Used "n" to select an inline.
  - Clicked "Done" checkbox.
  - Pressed "n".
  - Went to the next inline (previously: lost position in document).
  - Behavior is also better for: edit, reply, collapse/expand.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12634, T8131

Differential Revision: https://secure.phabricator.com/D17905

resources/celerity/map.php

@@ -13,7 +13,7 @@ return array(
     'core.pkg.js' => '8c5f913d',
     'darkconsole.pkg.js' => '1f9a31bc',
     'differential.pkg.css' => '58712637',
-    'differential.pkg.js' => '6ee9a850',
+    'differential.pkg.js' => '0bfd141c',
     'diffusion.pkg.css' => 'b93d9b8c',
     'diffusion.pkg.js' => '84c8f8fd',
     'favicon.ico' => '30672e08',
@@ -390,9 +390,9 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '408bf173',
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
-    'rsrc/js/application/diff/DiffChangeset.js' => 'f7100923',
-    'rsrc/js/application/diff/DiffChangesetList.js' => 'e5c5e171',
-    'rsrc/js/application/diff/DiffInline.js' => '4bbefc49',
+    'rsrc/js/application/diff/DiffChangeset.js' => '145c34e2',
+    'rsrc/js/application/diff/DiffChangesetList.js' => 'ca3b6387',
+    'rsrc/js/application/diff/DiffInline.js' => 'b5b1f167',
     'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832',
     'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d',
     'rsrc/js/application/differential/behavior-comment-preview.js' => 'b064af76',
@@ -781,9 +781,9 @@ return array(
     'phabricator-darklog' => 'c8e1ffe3',
     'phabricator-darkmessage' => 'c48cccdd',
     'phabricator-dashboard-css' => 'fe5b1869',
-    'phabricator-diff-changeset' => 'f7100923',
-    'phabricator-diff-changeset-list' => 'e5c5e171',
-    'phabricator-diff-inline' => '4bbefc49',
+    'phabricator-diff-changeset' => '145c34e2',
+    'phabricator-diff-changeset-list' => 'ca3b6387',
+    'phabricator-diff-inline' => 'b5b1f167',
     'phabricator-drag-and-drop-file-upload' => '58dea2fa',
     'phabricator-draggable-list' => 'bea6e7f4',
     'phabricator-fatal-config-template-css' => '8f18fa41',
@@ -999,6 +999,17 @@ return array(
       'javelin-dom',
       'javelin-typeahead-normalizer',
     ),
+    '145c34e2' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+      'phabricator-diff-inline',
+    ),
     '1499a8cb' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -1259,9 +1270,6 @@ return array(
       'javelin-util',
       'phabricator-shaped-request',
     ),
-    '4bbefc49' => array(
-      'javelin-dom',
-    ),
     '4c193c96' => array(
       'javelin-behavior',
       'javelin-uri',
@@ -1834,6 +1842,9 @@ return array(
       'javelin-dom',
       'phabricator-draggable-list',
     ),
+    'b5b1f167' => array(
+      'javelin-dom',
+    ),
     'b5c256b8' => array(
       'javelin-install',
       'javelin-dom',
@@ -1973,6 +1984,9 @@ return array(
       'phabricator-shaped-request',
       'conpherence-thread-manager',
     ),
+    'ca3b6387' => array(
+      'javelin-install',
+    ),
     'ca3f91eb' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -2137,9 +2151,6 @@ return array(
       'javelin-workflow',
       'javelin-magical-init',
     ),
-    'e5c5e171' => array(
-      'javelin-install',
-    ),
     'e9581f08' => array(
       'javelin-behavior',
       'javelin-stratcom',
@@ -2188,17 +2199,6 @@ return array(
       'phuix-icon-view',
       'phabricator-prefab',
     ),
-    'f7100923' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-      'phabricator-diff-inline',
-    ),
     'f7fc67ec' => array(
       'javelin-install',
       'javelin-typeahead',

webroot/rsrc/js/application/diff/DiffChangeset.js

@@ -369,17 +369,11 @@ JX.install('DiffChangeset', {
           for (var jj = 0; jj < block.items.length; jj++) {
             var inline = this.getInlineForRow(block.items[jj]);
 
-            // If this inline has been collapsed, don't select it with the
-            // keyboard cursor.
-            if (inline.isHidden()) {
-              continue;
-            }
-
             items.push({
               type: block.type,
               changeset: this,
-              target: block.items[jj],
-              inline: inline,
+              target: inline,
+              hidden: inline.isHidden(),
               nodes: {
                 begin: block.items[jj],
                 end: block.items[jj]

webroot/rsrc/js/application/diff/DiffChangesetList.js

@@ -206,7 +206,7 @@ JX.install('DiffChangesetList', {
 
       if (cursor) {
         if (cursor.type == 'comment') {
-          var inline = cursor.inline;
+          var inline = cursor.target;
           if (inline.canReply()) {
             this.setFocus(null);
 
@@ -225,7 +225,7 @@ JX.install('DiffChangesetList', {
 
       if (cursor) {
         if (cursor.type == 'comment') {
-          var inline = cursor.inline;
+          var inline = cursor.target;
           if (inline.canEdit()) {
             this.setFocus(null);
 
@@ -284,6 +284,12 @@ JX.install('DiffChangesetList', {
           }
         }
 
+        // If the item is hidden, don't select it when iterating with jump
+        // keys. It can still potentially be selected in other ways.
+        if (items[cursor].hidden) {
+          continue;
+        }
+
         // Otherwise, we've found a valid item to select.
         break;
       }
@@ -328,13 +334,28 @@ JX.install('DiffChangesetList', {
 
       this.setFocus(cursor.nodes.begin, cursor.nodes.end);
 
-      if (scroll) {
+      if (manager && scroll) {
         manager.scrollTo(cursor.nodes.begin);
       }
 
       return this;
     },
 
+    redrawCursor: function() {
+      // NOTE: This is setting the cursor to the current cursor. Usually, this
+      // would have no effect.
+
+      // However, if the old cursor pointed at an inline and the inline has
+      // been edited so the rows have changed, this updates the cursor to point
+      // at the new inline with the proper rows for the current state, and
+      // redraws the reticle correctly.
+
+      var state = this._getSelectionState();
+      if (state.cursor !== null) {
+        this._setSelectionState(state.items[state.cursor]);
+      }
+    },
+
     _getSelectableItems: function() {
       var result = [];
 

webroot/rsrc/js/application/diff/DiffInline.js

@@ -519,6 +519,8 @@ JX.install('DiffInline', {
       // preview at the bottom of the page.
       this.getChangeset().getChangesetList().redrawPreview();
 
+      this.getChangeset().getChangesetList().redrawCursor();
+
       // Emit a resize event so that UI elements like the keyboad focus
       // reticle can redraw properly.
       JX.Stratcom.invoke('resize');