diff --git a/resources/celerity/map.php b/resources/celerity/map.php --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ 'core.pkg.js' => '8c5f913d', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '58712637', - 'differential.pkg.js' => 'e486afd0', + 'differential.pkg.js' => 'bd321b6e', 'diffusion.pkg.css' => 'b93d9b8c', 'diffusion.pkg.js' => '84c8f8fd', 'favicon.ico' => '30672e08', @@ -391,10 +391,9 @@ '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' => '145c34e2', - 'rsrc/js/application/diff/DiffChangesetList.js' => 'd93e34c2', + 'rsrc/js/application/diff/DiffChangesetList.js' => 'f1101e6e', 'rsrc/js/application/diff/DiffInline.js' => 'bdf6b568', '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', 'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1', 'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '89d11432', @@ -619,7 +618,6 @@ 'javelin-behavior-detect-timezone' => '4c193c96', 'javelin-behavior-device' => 'bb1dd507', 'javelin-behavior-diff-preview-link' => '051c7832', - 'javelin-behavior-differential-comment-jump' => '4fdb476d', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', 'javelin-behavior-differential-edit-inline-comments' => '89d11432', 'javelin-behavior-differential-feedback-preview' => 'b064af76', @@ -782,7 +780,7 @@ 'phabricator-darkmessage' => 'c48cccdd', 'phabricator-dashboard-css' => 'fe5b1869', 'phabricator-diff-changeset' => '145c34e2', - 'phabricator-diff-changeset-list' => 'd93e34c2', + 'phabricator-diff-changeset-list' => 'f1101e6e', 'phabricator-diff-inline' => 'bdf6b568', 'phabricator-drag-and-drop-file-upload' => '58dea2fa', 'phabricator-draggable-list' => 'bea6e7f4', @@ -1291,11 +1289,6 @@ 'javelin-stratcom', 'javelin-dom', ), - '4fdb476d' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - ), '503e17fd' => array( 'javelin-install', 'javelin-typeahead-source', @@ -2079,9 +2072,6 @@ 'javelin-util', 'phabricator-shaped-request', ), - 'd93e34c2' => array( - 'javelin-install', - ), 'de2e896f' => array( 'javelin-behavior', 'javelin-dom', @@ -2184,6 +2174,9 @@ 'javelin-workflow', 'javelin-json', ), + 'f1101e6e' => array( + 'javelin-install', + ), 'f50152ad' => array( 'phui-timeline-view-css', ), @@ -2431,7 +2424,6 @@ 'javelin-behavior-differential-edit-inline-comments', 'javelin-behavior-differential-populate', 'javelin-behavior-differential-diff-radios', - 'javelin-behavior-differential-comment-jump', 'javelin-behavior-aphront-drag-and-drop-textarea', 'javelin-behavior-phabricator-object-selector', 'javelin-behavior-repository-crossreference', diff --git a/resources/celerity/packages.php b/resources/celerity/packages.php --- a/resources/celerity/packages.php +++ b/resources/celerity/packages.php @@ -196,7 +196,6 @@ 'javelin-behavior-differential-edit-inline-comments', 'javelin-behavior-differential-populate', 'javelin-behavior-differential-diff-radios', - 'javelin-behavior-differential-comment-jump', 'javelin-behavior-aphront-drag-and-drop-textarea', 'javelin-behavior-phabricator-object-selector', 'javelin-behavior-repository-crossreference', diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -270,8 +270,6 @@ ), )); - $this->initBehavior('differential-comment-jump', array()); - if ($this->inlineURI) { Javelin::initBehavior('differential-edit-inline-comments', array( 'uri' => $this->inlineURI, diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -190,61 +190,31 @@ } } - $nextprev = null; - if (!$this->preview) { - $nextprev = new PHUIButtonBarView(); - $nextprev->setBorderless(true); - $nextprev->addClass('inline-button-divider'); - + $anchor_name = $this->getAnchorName(); - $up = id(new PHUIButtonView()) - ->setTag('a') - ->setTooltip(pht('Previous')) - ->setIcon('fa-chevron-up') - ->addSigil('differential-inline-prev') - ->setMustCapture(true); + $action_buttons = array(); - $down = id(new PHUIButtonView()) - ->setTag('a') - ->setTooltip(pht('Next')) - ->setIcon('fa-chevron-down') - ->addSigil('differential-inline-next') - ->setMustCapture(true); + $can_reply = + (!$this->editable) && + (!$this->preview) && + ($this->allowReply) && - if ($this->canHide()) { - $hide = id(new PHUIButtonView()) - ->setTag('a') - ->setTooltip(pht('Hide Comment')) - ->setIcon('fa-times') - ->addSigil('hide-inline') - ->setMustCapture(true); + // NOTE: No product reason why you can't reply to synthetic comments, + // but the reply mechanism currently sends the inline comment ID to the + // server, not file/line information, and synthetic comments don't have + // an inline comment ID. + (!$is_synthetic); - $nextprev->addButton($hide); - } - $nextprev->addButton($up); - $nextprev->addButton($down); - - $action_buttons = array(); - if ($this->allowReply) { - if (!$is_synthetic) { - // NOTE: No product reason why you can't reply to these, but the reply - // mechanism currently sends the inline comment ID to the server, not - // file/line information, and synthetic comments don't have an inline - // comment ID. - - $action_buttons[] = id(new PHUIButtonView()) - ->setTag('a') - ->setIcon('fa-reply') - ->setTooltip(pht('Reply')) - ->addSigil('differential-inline-reply') - ->setMustCapture(true); - } - } + if ($can_reply) { + $action_buttons[] = id(new PHUIButtonView()) + ->setTag('a') + ->setIcon('fa-reply') + ->setTooltip(pht('Reply')) + ->addSigil('differential-inline-reply') + ->setMustCapture(true); } - $anchor_name = $this->getAnchorName(); - if ($this->editable && !$this->preview) { $action_buttons[] = id(new PHUIButtonView()) ->setTag('a') @@ -280,6 +250,15 @@ ->setMustCapture(true); } + if (!$this->preview && $this->canHide()) { + $action_buttons[] = id(new PHUIButtonView()) + ->setTag('a') + ->setTooltip(pht('Hide Comment')) + ->setIcon('fa-times') + ->addSigil('hide-inline') + ->setMustCapture(true); + } + $done_button = null; if (!$is_synthetic) { @@ -430,7 +409,6 @@ $done_button, $links, $actions, - $nextprev, )); $markup = javelin_tag( @@ -441,10 +419,16 @@ 'meta' => $metadata, ), array( - phutil_tag_div('differential-inline-comment-head grouped', array( - $group_left, - $group_right, - )), + javelin_tag( + 'div', + array( + 'class' => 'differential-inline-comment-head grouped', + 'sigil' => 'differential-inline-header', + ), + array( + $group_left, + $group_right, + )), phutil_tag_div( 'differential-inline-comment-content', phutil_tag_div('phabricator-remarkup', $content)), diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -50,6 +50,12 @@ var onresize = JX.bind(this, this._ifawake, this._onresize); JX.Stratcom.listen('resize', null, onresize); + + var onselect = JX.bind(this, this._ifawake, this._onselect); + JX.Stratcom.listen( + 'mousedown', + ['differential-inline-comment', 'differential-inline-header'], + onselect); }, properties: { @@ -665,6 +671,47 @@ this._redrawFocus(); }, + _onselect: function(e) { + // If the user clicked some element inside the header, like an action + // icon, ignore the event. They have to click the header element itself. + if (e.getTarget() !== e.getNode('differential-inline-header')) { + return; + } + + var inline = this._getInlineForEvent(e); + if (!inline) { + return; + } + + // The user definitely clicked an inline, so we're going to handle the + // event. + e.kill(); + + var selection = this._getSelectionState(); + var item; + + // If the comment the user clicked is currently selected, deselect it. + // This makes it easy to undo things if you clicked by mistake. + if (selection.cursor !== null) { + item = selection.items[selection.cursor]; + if (item.target === inline) { + this._setSelectionState(null); + return; + } + } + + // Otherwise, select the item that the user clicked. This makes it + // easier to resume keyboard operations after using the mouse to do + // something else. + var items = selection.items; + for (var ii = 0; ii < items.length; ii++) { + item = items[ii]; + if (item.target === inline) { + this._setSelectionState(item); + } + } + }, + _onaction: function(action, e) { e.kill(); diff --git a/webroot/rsrc/js/application/differential/behavior-comment-jump.js b/webroot/rsrc/js/application/differential/behavior-comment-jump.js deleted file mode 100644 --- a/webroot/rsrc/js/application/differential/behavior-comment-jump.js +++ /dev/null @@ -1,32 +0,0 @@ -/** - * @provides javelin-behavior-differential-comment-jump - * @requires javelin-behavior - * javelin-stratcom - * javelin-dom - */ - -JX.behavior('differential-comment-jump', function() { - function handle_jump(offset) { - return function(e) { - var parent = JX.$('differential-review-stage'); - var clicked = e.getNode('differential-inline-comment'); - var inlines = JX.DOM.scry(parent, 'div', 'differential-inline-comment'); - var jumpto = null; - - for (var ii = 0; ii < inlines.length; ii++) { - if (inlines[ii] == clicked) { - jumpto = inlines[(ii + offset + inlines.length) % inlines.length]; - break; - } - } - JX.Stratcom.invoke('differential-toggle-file-request', null, { - element: jumpto - }); - JX.DOM.scrollTo(jumpto); - e.kill(); - }; - } - - JX.Stratcom.listen('click', 'differential-inline-prev', handle_jump(-1)); - JX.Stratcom.listen('click', 'differential-inline-next', handle_jump(+1)); -});