From 921164aab7bdf1ced6ccfea020cbeb5d6100fc6c Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 10 Jun 2011 08:34:17 -0700 Subject: [PATCH] Allow keyboard navigation between individual changes Summary: Permit "j" and "k" to cycle through individual changeblocks, similar to how this feature works in ReviewBoard. This still needs a bunch of refinement but it's getting closer to being useful. Also moved reticle underneath the table so you can click links through it (derp derp). Test Plan: Used "j" and "k" to cycle through individual changes. Reviewed By: aran Reviewers: aran, jungejason, tuomaspelkonen CC: moskov, aran, epriestley Differential Revision: 426 --- src/__celerity_resource_map__.php | 142 +++++++++--------- .../application/base/standard-page-view.css | 9 +- .../differential/changeset-view.css | 7 +- .../core/KeyboardShortcutManager.js | 29 +++- .../differential/behavior-keyboard-nav.js | 98 +++++++++++- 5 files changed, 196 insertions(+), 89 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 7cdef0e7aa..35acdc782c 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -63,7 +63,7 @@ celerity_register_resource_map(array( ), 'aphront-headsup-action-list-view-css' => array( - 'uri' => '/res/b7c6bbc2/rsrc/css/aphront/headsup-action-list-view.css', + 'uri' => '/res/c0ef93b6/rsrc/css/aphront/headsup-action-list-view.css', 'type' => 'css', 'requires' => array( @@ -145,7 +145,7 @@ celerity_register_resource_map(array( ), 'differential-changeset-view-css' => array( - 'uri' => '/res/2a59525c/rsrc/css/application/differential/changeset-view.css', + 'uri' => '/res/d2923406/rsrc/css/application/differential/changeset-view.css', 'type' => 'css', 'requires' => array( @@ -451,7 +451,7 @@ celerity_register_resource_map(array( ), 'javelin-behavior-differential-keyboard-navigation' => array( - 'uri' => '/res/dfa0f979/rsrc/js/application/differential/behavior-keyboard-nav.js', + 'uri' => '/res/08ad535c/rsrc/js/application/differential/behavior-keyboard-nav.js', 'type' => 'js', 'requires' => array( @@ -1024,7 +1024,7 @@ celerity_register_resource_map(array( ), 'phabricator-keyboard-shortcut-manager' => array( - 'uri' => '/res/14f11fd3/rsrc/js/application/core/KeyboardShortcutManager.js', + 'uri' => '/res/0454fd16/rsrc/js/application/core/KeyboardShortcutManager.js', 'type' => 'js', 'requires' => array( @@ -1038,7 +1038,7 @@ celerity_register_resource_map(array( ), 'phabricator-object-selector-css' => array( - 'uri' => '/res/62a8fd92/rsrc/css/application/objectselector/object-selector.css', + 'uri' => '/res/608461d2/rsrc/css/application/objectselector/object-selector.css', 'type' => 'css', 'requires' => array( @@ -1078,7 +1078,7 @@ celerity_register_resource_map(array( ), 'phabricator-standard-page-view' => array( - 'uri' => '/res/453bd261/rsrc/css/application/base/standard-page-view.css', + 'uri' => '/res/f0022c27/rsrc/css/application/base/standard-page-view.css', 'type' => 'css', 'requires' => array( @@ -1132,40 +1132,7 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/2892314d/typeahead.pkg.js', 'type' => 'js', ), - '861d7db0' => - array ( - 'name' => 'workflow.pkg.js', - 'symbols' => - array ( - 0 => 'javelin-mask', - 1 => 'javelin-workflow', - 2 => 'javelin-behavior-workflow', - 3 => 'javelin-behavior-aphront-form-disable-on-submit', - 4 => 'phabricator-keyboard-shortcut-manager', - 5 => 'phabricator-keyboard-shortcut', - 6 => 'javelin-behavior-phabricator-keyboard-shortcuts', - ), - 'uri' => '/res/pkg/861d7db0/workflow.pkg.js', - 'type' => 'js', - ), - '8f3a55d1' => - array ( - 'name' => 'differential.pkg.css', - 'symbols' => - array ( - 0 => 'differential-core-view-css', - 1 => 'differential-changeset-view-css', - 2 => 'differential-revision-detail-css', - 3 => 'differential-revision-history-css', - 4 => 'differential-table-of-contents-css', - 5 => 'differential-revision-comment-css', - 6 => 'differential-revision-add-comment-css', - 7 => 'differential-revision-comment-list-css', - ), - 'uri' => '/res/pkg/8f3a55d1/differential.pkg.css', - 'type' => 'css', - ), - 'acfddc38' => + 'a452c449' => array ( 'name' => 'core.pkg.css', 'symbols' => @@ -1186,7 +1153,24 @@ celerity_register_resource_map(array( 13 => 'phabricator-remarkup-css', 14 => 'syntax-highlighting-css', ), - 'uri' => '/res/pkg/acfddc38/core.pkg.css', + 'uri' => '/res/pkg/a452c449/core.pkg.css', + 'type' => 'css', + ), + 'c9226a80' => + array ( + 'name' => 'differential.pkg.css', + 'symbols' => + array ( + 0 => 'differential-core-view-css', + 1 => 'differential-changeset-view-css', + 2 => 'differential-revision-detail-css', + 3 => 'differential-revision-history-css', + 4 => 'differential-table-of-contents-css', + 5 => 'differential-revision-comment-css', + 6 => 'differential-revision-add-comment-css', + 7 => 'differential-revision-comment-list-css', + ), + 'uri' => '/res/pkg/c9226a80/differential.pkg.css', 'type' => 'css', ), 'da416e1c' => @@ -1222,42 +1206,58 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/db95a6d0/javelin.pkg.js', 'type' => 'js', ), + 'df91920b' => + array ( + 'name' => 'workflow.pkg.js', + 'symbols' => + array ( + 0 => 'javelin-mask', + 1 => 'javelin-workflow', + 2 => 'javelin-behavior-workflow', + 3 => 'javelin-behavior-aphront-form-disable-on-submit', + 4 => 'phabricator-keyboard-shortcut-manager', + 5 => 'phabricator-keyboard-shortcut', + 6 => 'javelin-behavior-phabricator-keyboard-shortcuts', + ), + 'uri' => '/res/pkg/df91920b/workflow.pkg.js', + 'type' => 'js', + ), ), 'reverse' => array ( - 'aphront-crumbs-view-css' => 'acfddc38', - 'aphront-dialog-view-css' => 'acfddc38', - 'aphront-form-view-css' => 'acfddc38', - 'aphront-list-filter-view-css' => 'acfddc38', - 'aphront-panel-view-css' => 'acfddc38', - 'aphront-side-nav-view-css' => 'acfddc38', - 'aphront-table-view-css' => 'acfddc38', - 'aphront-tokenizer-control-css' => 'acfddc38', - 'aphront-typeahead-control-css' => 'acfddc38', - 'differential-changeset-view-css' => '8f3a55d1', - 'differential-core-view-css' => '8f3a55d1', - 'differential-revision-add-comment-css' => '8f3a55d1', - 'differential-revision-comment-css' => '8f3a55d1', - 'differential-revision-comment-list-css' => '8f3a55d1', - 'differential-revision-detail-css' => '8f3a55d1', - 'differential-revision-history-css' => '8f3a55d1', - 'differential-table-of-contents-css' => '8f3a55d1', + 'aphront-crumbs-view-css' => 'a452c449', + 'aphront-dialog-view-css' => 'a452c449', + 'aphront-form-view-css' => 'a452c449', + 'aphront-list-filter-view-css' => 'a452c449', + 'aphront-panel-view-css' => 'a452c449', + 'aphront-side-nav-view-css' => 'a452c449', + 'aphront-table-view-css' => 'a452c449', + 'aphront-tokenizer-control-css' => 'a452c449', + 'aphront-typeahead-control-css' => 'a452c449', + 'differential-changeset-view-css' => 'c9226a80', + 'differential-core-view-css' => 'c9226a80', + 'differential-revision-add-comment-css' => 'c9226a80', + 'differential-revision-comment-css' => 'c9226a80', + 'differential-revision-comment-list-css' => 'c9226a80', + 'differential-revision-detail-css' => 'c9226a80', + 'differential-revision-history-css' => 'c9226a80', + 'differential-table-of-contents-css' => 'c9226a80', 'diffusion-commit-view-css' => '03ef179e', 'javelin-behavior' => 'db95a6d0', 'javelin-behavior-aphront-basic-tokenizer' => '2892314d', - 'javelin-behavior-aphront-form-disable-on-submit' => '861d7db0', + 'javelin-behavior-aphront-form-disable-on-submit' => 'df91920b', 'javelin-behavior-differential-diff-radios' => 'da416e1c', 'javelin-behavior-differential-edit-inline-comments' => 'da416e1c', 'javelin-behavior-differential-feedback-preview' => 'da416e1c', 'javelin-behavior-differential-populate' => 'da416e1c', 'javelin-behavior-differential-show-more' => 'da416e1c', - 'javelin-behavior-phabricator-keyboard-shortcuts' => '861d7db0', - 'javelin-behavior-workflow' => '861d7db0', + 'javelin-behavior-phabricator-keyboard-shortcuts' => 'df91920b', + 'javelin-behavior-workflow' => 'df91920b', 'javelin-dom' => 'db95a6d0', 'javelin-event' => 'db95a6d0', 'javelin-install' => 'db95a6d0', 'javelin-json' => 'db95a6d0', - 'javelin-mask' => '861d7db0', + 'javelin-mask' => 'df91920b', 'javelin-request' => 'db95a6d0', 'javelin-stratcom' => 'db95a6d0', 'javelin-tokenizer' => '2892314d', @@ -1269,14 +1269,14 @@ celerity_register_resource_map(array( 'javelin-uri' => 'db95a6d0', 'javelin-util' => 'db95a6d0', 'javelin-vector' => 'db95a6d0', - 'javelin-workflow' => '861d7db0', - 'phabricator-core-buttons-css' => 'acfddc38', - 'phabricator-core-css' => 'acfddc38', - 'phabricator-directory-css' => 'acfddc38', - 'phabricator-keyboard-shortcut' => '861d7db0', - 'phabricator-keyboard-shortcut-manager' => '861d7db0', - 'phabricator-remarkup-css' => 'acfddc38', - 'phabricator-standard-page-view' => 'acfddc38', - 'syntax-highlighting-css' => 'acfddc38', + 'javelin-workflow' => 'df91920b', + 'phabricator-core-buttons-css' => 'a452c449', + 'phabricator-core-css' => 'a452c449', + 'phabricator-directory-css' => 'a452c449', + 'phabricator-keyboard-shortcut' => 'df91920b', + 'phabricator-keyboard-shortcut-manager' => 'df91920b', + 'phabricator-remarkup-css' => 'a452c449', + 'phabricator-standard-page-view' => 'a452c449', + 'syntax-highlighting-css' => 'a452c449', ), )); diff --git a/webroot/rsrc/css/application/base/standard-page-view.css b/webroot/rsrc/css/application/base/standard-page-view.css index 819e29d595..121e72072b 100644 --- a/webroot/rsrc/css/application/base/standard-page-view.css +++ b/webroot/rsrc/css/application/base/standard-page-view.css @@ -134,13 +134,10 @@ td.phabricator-login-details { } .keyboard-focus-focus-reticle { - z-index: 32; - border: 1px solid #999999; - + z-index: 1; + background: #fffff3; position: absolute; - -webkit-box-shadow: 0 0 3px #000; - -mox-box-shadow: 0 0 3px #000; - box-shadow: 0 0 3px #000; + border: 1px solid #eeeed3; } .keyboard-shortcuts-available { diff --git a/webroot/rsrc/css/application/differential/changeset-view.css b/webroot/rsrc/css/application/differential/changeset-view.css index ac78c5a6e4..0ac8bd6ebe 100644 --- a/webroot/rsrc/css/application/differential/changeset-view.css +++ b/webroot/rsrc/css/application/differential/changeset-view.css @@ -2,8 +2,13 @@ * @provides differential-changeset-view-css */ +.differential-changeset { + z-index: 2; + position: relative; +} + .differential-diff { - background: #ffffff; + background: transparent; font-family: "Menlo", "Consolas", "Monaco", monospace; font-size: 10px; width: 100%; diff --git a/webroot/rsrc/js/application/core/KeyboardShortcutManager.js b/webroot/rsrc/js/application/core/KeyboardShortcutManager.js index 2de1b46281..9dc427a7f6 100644 --- a/webroot/rsrc/js/application/core/KeyboardShortcutManager.js +++ b/webroot/rsrc/js/application/core/KeyboardShortcutManager.js @@ -54,21 +54,40 @@ JX.install('KeyboardShortcutManager', { * Scroll an element into view. */ scrollTo : function(node) { - window.scrollTo(0, JX.$V(node).y - 40); + window.scrollTo(0, JX.$V(node).y - 60); }, /** * Move the keyboard shortcut focus to an element. + * + * @param Node Node to focus, or pass null to clear the focus. + * @param Node To focus multiple nodes (like rows in a table), specify the + * top-left node as the first parameter and the bottom-right + * node as the focus extension. + * @return void */ - focusOn : function(node) { + focusOn : function(node, extended_node) { this._clearReticle(); + if (!node) { + return; + } + var r = JX.$N('div', {className : 'keyboard-focus-focus-reticle'}); - // Outset the reticle 8 pixels away from the element, so there's some + extended_node = extended_node || node; + + // Outset the reticle some pixels away from the element, so there's some // space between the focused element and the outline. - JX.Vector.getPos(node).add(-8, -8).setPos(r); - JX.Vector.getDim(node).add(16, 16).setDim(r); + var p = JX.Vector.getPos(node); + p.add(-6, -6).setPos(r); + // Compute the size we need to extend to the full extent of the focused + // nodes. + JX.Vector.getPos(extended_node) + .add(-p.x, -p.y) + .add(JX.Vector.getDim(extended_node)) + .add(12, 12) + .setDim(r); document.body.appendChild(r); this._focusReticle = r; diff --git a/webroot/rsrc/js/application/differential/behavior-keyboard-nav.js b/webroot/rsrc/js/application/differential/behavior-keyboard-nav.js index 49b639315a..ea373278a7 100644 --- a/webroot/rsrc/js/application/differential/behavior-keyboard-nav.js +++ b/webroot/rsrc/js/application/differential/behavior-keyboard-nav.js @@ -7,7 +7,8 @@ JX.behavior('differential-keyboard-navigation', function(config) { - var cursor = null; + var cursor = -1; + var cursor_block = null; var changesets; function init() { @@ -17,19 +18,104 @@ JX.behavior('differential-keyboard-navigation', function(config) { changesets = JX.DOM.scry(document.body, 'div', 'differential-changeset'); } + function getBlocks(cursor) { + // TODO: This might not be terribly fast; we can't currently memoize it + // because it can change as ajax requests come in (e.g., content loads). + + var rows = JX.DOM.scry(changesets[cursor], 'tr'); + var blocks = [[changesets[cursor], changesets[cursor]]]; + var start = null; + var type; + for (var ii = 0; ii < rows.length; ii++) { + type = getRowType(rows[ii]); + if (type == 'ignore') { + continue; + } + if (!type && start) { + blocks.push([start, rows[ii - 1]]); + start = null; + } else if (type && !start) { + start = rows[ii]; + } + } + if (start) { + blocks.push([start, rows[ii - 1]]); + } + + return blocks; + } + + function getRowType(row) { + // NOTE: Being somewhat over-general here to allow other types of objects + // to be easily focused in the future (inline comments, 'show more..'). + + if (row.className.indexOf('inline') !== -1) { + return 'ignore'; + } + + var cells = JX.DOM.scry(row, 'td'); + + for (var ii = 0; ii < cells.length; ii++) { + // NOTE: The semantic use of classnames here is for performance; don't + // emulate this elsewhere since it's super terrible. + if (cells[ii].className.indexOf('old') !== -1 || + cells[ii].className.indexOf('new') !== -1) { + return 'change'; + } + } + + return null; + } + function jump(manager, delta) { init(); - if (cursor === null) { - cursor = -1; + if (cursor < 0) { + if (delta < 0) { + // If the user goes "back" without a selection, just reject the action. + return; + } else { + cursor = 0; + } } - cursor = (cursor + changesets.length + delta) % changesets.length; + var selected; + var extent; + while (true) { + var blocks = getBlocks(cursor); + var focus; + if (delta < 0) { + focus = blocks.length; + } else { + focus = -1; + } - var selected = changesets[cursor]; + for (var ii = 0; ii < blocks.length; ii++) { + if (blocks[ii][0] == cursor_block) { + focus = ii; + break; + } + } + + focus += delta; + + if (blocks[focus]) { + selected = blocks[focus][0]; + extent = blocks[focus][1]; + cursor_block = selected; + break; + } else { + var adjusted = (cursor + delta); + if (adjusted < 0 || adjusted >= changesets.length) { + // Stop cursor movement when the user reaches either end. + return; + } + cursor = adjusted; + } + } manager.scrollTo(selected); - manager.focusOn(selected); + manager.focusOn(selected, extent); } new JX.KeyboardShortcut('j', 'Jump to next change.')