From dcf3ca8e04508a1f8e290428cecebaecab8502f5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 5 Jun 2019 18:42:42 -0700 Subject: [PATCH] When a user clicks a navigation link in a dialog, close the dialog Summary: Ref T13302. Currently, if you enable Quicksand (by clicking "Persistent Chat"), open a dialog with links in it (like "Create Subtask" with multiple available subtypes), and then follow a navigation link, the page content reloads behind the dialog but the dialog stays in the foreground. Fix this by closing dialogs when users click navigation links inside them. Test Plan: With Quicksand enabled and disabled, clicked a subtask type in the "Create Subtask" dialog. - Before, Quicksand Disabled: Dialog stays on screen, then navigation occurs. - After, Quicksand Disabled: Dialog vanishes, then navigation occurs. - Before, Quicksand Enabled: Dialog stays on screen, navigation occurs behind it. - After, Quicksand Enabled: Dialog vanishes, then navigation occurs. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13302 Differential Revision: https://secure.phabricator.com/D20573 --- resources/celerity/map.php | 28 +++++------ .../rsrc/externals/javelin/lib/Workflow.js | 48 +++++++++++++------ 2 files changed, 48 insertions(+), 28 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 49d8f0df0b..02d4a67281 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => 'af983028', - 'core.pkg.js' => 'ee320ca2', + 'core.pkg.js' => 'f39ebda8', 'differential.pkg.css' => '8d8360fb', 'differential.pkg.js' => '67e02996', 'diffusion.pkg.css' => '42c75c37', @@ -253,7 +253,7 @@ return array( 'rsrc/externals/javelin/lib/URI.js' => '2e255291', 'rsrc/externals/javelin/lib/Vector.js' => 'e9c80beb', 'rsrc/externals/javelin/lib/WebSocket.js' => 'fdc13e4e', - 'rsrc/externals/javelin/lib/Workflow.js' => '958e9045', + 'rsrc/externals/javelin/lib/Workflow.js' => 'e9c6d3c7', 'rsrc/externals/javelin/lib/__tests__/Cookie.js' => 'ca686f71', 'rsrc/externals/javelin/lib/__tests__/DOM.js' => '4566e249', 'rsrc/externals/javelin/lib/__tests__/JSON.js' => '710377ae', @@ -752,7 +752,7 @@ return array( 'javelin-workboard-header' => '111bfd2d', 'javelin-workboard-header-template' => 'ebe83a6b', 'javelin-workboard-order-template' => '03e8891f', - 'javelin-workflow' => '958e9045', + 'javelin-workflow' => 'e9c6d3c7', 'maniphest-report-css' => '3d53188b', 'maniphest-task-edit-css' => '272daa84', 'maniphest-task-summary-css' => '61d1667e', @@ -1712,17 +1712,6 @@ return array( 'javelin-stratcom', 'javelin-vector', ), - '958e9045' => array( - 'javelin-stratcom', - 'javelin-request', - 'javelin-dom', - 'javelin-vector', - 'javelin-install', - 'javelin-util', - 'javelin-mask', - 'javelin-uri', - 'javelin-routable', - ), '9623adc1' => array( 'javelin-behavior', 'javelin-stratcom', @@ -2107,6 +2096,17 @@ return array( 'phabricator-title', 'phabricator-favicon', ), + 'e9c6d3c7' => array( + 'javelin-stratcom', + 'javelin-request', + 'javelin-dom', + 'javelin-vector', + 'javelin-install', + 'javelin-util', + 'javelin-mask', + 'javelin-uri', + 'javelin-routable', + ), 'e9c80beb' => array( 'javelin-install', 'javelin-event', diff --git a/webroot/rsrc/externals/javelin/lib/Workflow.js b/webroot/rsrc/externals/javelin/lib/Workflow.js index d767a58780..5c8d74a54d 100644 --- a/webroot/rsrc/externals/javelin/lib/Workflow.js +++ b/webroot/rsrc/externals/javelin/lib/Workflow.js @@ -75,6 +75,7 @@ JX.install('Workflow', { var workflow = new JX.Workflow(link.href); return workflow; }, + _push : function(workflow) { JX.Mask.show(); JX.Workflow._stack.push(workflow); @@ -85,8 +86,36 @@ JX.install('Workflow', { dialog._destroy(); JX.Mask.hide(); }, - disable : function() { - JX.Workflow._disabled = true; + _onlink: function(event) { + // See T13302. When a user clicks a link in a dialog and that link + // triggers a navigation event, we want to close the dialog as though + // they had pressed a button. + + // When Quicksand is enabled, this is particularly relevant because + // the dialog will stay in the foreground while the page content changes + // in the background if we do not dismiss the dialog. + + // If this is a Command-Click, the link will open in a new window. + var is_command = !!event.getRawEvent().metaKey; + if (is_command) { + return; + } + + var link = event.getNode('tag:a'); + + // If the link is an anchor, or does not go anywhere, ignore the event. + var href = '' + link.href; + if (!href.length || href[0] === '#') { + return; + } + + // This link will open in a new window. + if (link.target === '_blank') { + return; + } + + // Close the dialog. + JX.Workflow._pop(); }, _onbutton : function(event) { @@ -94,10 +123,6 @@ JX.install('Workflow', { return; } - if (JX.Workflow._disabled) { - return; - } - // Get the button (which is sometimes actually another tag, like an ) // which triggered the event. In particular, this makes sure we get the // right node if there is a