From 89c8fdb83fda5de770703b63cd555866136477aa Mon Sep 17 00:00:00 2001 From: Ty Voliter Date: Wed, 18 Jul 2012 11:50:16 -0700 Subject: [PATCH 1/2] fixes issues #1270 #1271 #1272 - fixed ESC key not working to dismiss dialogs by fixing bug where popup was removed from list when it was persistant - simplified addPopUp() API by removing different function call relating to popup persistance and instead having an "autoAddRemove" parameter - prefixed data keys with "PopUpManager" since PopUpManager is decorating elements it doesn't own - changed closing call for menus and context menus to call this.close() instead of closeAll() --- src/command/Menus.js | 10 ++++++-- src/widgets/PopUpManager.js | 47 +++++++++++++++---------------------- 2 files changed, 27 insertions(+), 30 deletions(-) diff --git a/src/command/Menus.js b/src/command/Menus.js index 74293cb6c97..0ddcdf94af1 100644 --- a/src/command/Menus.js +++ b/src/command/Menus.js @@ -651,13 +651,14 @@ define(function (require, exports, module) { _insertInList($menubar, $newMenu, position, $relativeElement); // Install ESC key handling - PopUpManager.configurePopUp($popUp, closeAll); + PopUpManager.addPopUp($popUp, menu.close, false); // todo error handling return menu; } + /** * @constructor * @extends {Menu} @@ -689,7 +690,12 @@ define(function (require, exports, module) { // insert into DOM $("#context-menu-bar > ul").append($newMenu); - PopUpManager.configurePopUp($popUp, closeAll); + var self = this; + PopUpManager.addPopUp($popUp, + function () { + self.close(); + }, + false); } ContextMenu.prototype = new Menu(); ContextMenu.prototype.constructor = ContextMenu; diff --git a/src/widgets/PopUpManager.js b/src/widgets/PopUpManager.js index 9aa5dcf1a15..0765a7ca189 100644 --- a/src/widgets/PopUpManager.js +++ b/src/widgets/PopUpManager.js @@ -36,8 +36,8 @@ define(function (require, exports, module) { var _popUps = []; function _removePopUp($popUp, index, visible) { - var initiallyInDOM = $popUp.data("initiallyInDOM"), - removeHandler = $popUp.data("removeHandler"); + var autoAddRemove = $popUp.data("PopUpManager-autoAddRemove"), + removeHandler = $popUp.data("PopUpManager-removeHandler"); visible = visible || $popUp.find(":visible").length > 0; @@ -45,40 +45,33 @@ define(function (require, exports, module) { removeHandler(); } - if (!initiallyInDOM) { + if (autoAddRemove) { $popUp.remove(); + _popUps = _popUps.slice(index); } - - _popUps = _popUps.slice(index); } - /** - * Add Esc key handling for pop-up DOM elements that persist in the DOM. - * These pop-up elements must handle appearance (i.e. opening) themselves. - * - * @param {!jQuery} $popUp jQuery object for the DOM element pop-up - * @param {function} removeHandler Pop-up specific remove (e.g. display:none or DOM removal) - */ - function configurePopUp($popUp, removeHandler) { - _popUps.push($popUp[0]); - - $popUp.data("initiallyInDOM", true); - $popUp.data("removeHandler", removeHandler); - } + /** - * Add Esc key handling for transient DOM elements. Immediately adds - * the element to the DOM if it's not already attached. + * Add Esc key handling for a popup DOM element. * * @param {!jQuery} $popUp jQuery object for the DOM element pop-up * @param {function} removeHandler Pop-up specific remove (e.g. display:none or DOM removal) + * @param {?Boolean} autoAddRemove - Specify true to indicate the PopUPManager should + * add/remove the popup from the DOM when the popup is open/closed. Specifiy false + * when the popup is either always persistant in the DOM or the add/remove is handled + * external to the PopupManager + * */ - function addPopUp($popUp, removeHandler) { - var initiallyInDOM = $popUp.parent().length === 1; + function addPopUp($popUp, removeHandler, autoAddRemove) { + autoAddRemove = autoAddRemove || false; - configurePopUp($popUp, removeHandler, initiallyInDOM); + _popUps.push($popUp[0]); + $popUp.data("PopUpManager-autoAddRemove", autoAddRemove); + $popUp.data("PopUpManager-removeHandler", removeHandler); - if (!initiallyInDOM) { + if (autoAddRemove) { $(window.document.body).append($popUp); } } @@ -91,8 +84,7 @@ define(function (require, exports, module) { */ function removePopUp($popUp) { var index = _popUps.indexOf($popUp[0]), - initiallyInDOM = $popUp.data("initiallyInDOM"), - removeHandler = $popUp.data("removeHandler"); + removeHandler = $popUp.data("PopUpManager-removeHandler"); if (index >= 0) { _removePopUp($popUp, index); @@ -100,7 +92,7 @@ define(function (require, exports, module) { } function _keydownCaptureListener(keyEvent) { - if (keyEvent.keyCode !== 27) { + if (keyEvent.keyCode !== 27) { // escape key return; } @@ -132,5 +124,4 @@ define(function (require, exports, module) { exports.addPopUp = addPopUp; exports.removePopUp = removePopUp; - exports.configurePopUp = configurePopUp; }); \ No newline at end of file From 727bf98e80d0e832abc80478fedf924680f01f1d Mon Sep 17 00:00:00 2001 From: Ty Voliter Date: Wed, 18 Jul 2012 14:17:09 -0700 Subject: [PATCH 2/2] - code review fixes - removed _removePopup() function since complexity of having this helper function isn't worth the savings of reusing the "index" var. I don't except their to be many visible popups at one time, so computing the index more than once isn't costly --- src/widgets/PopUpManager.js | 40 ++++++++++++++----------------------- 1 file changed, 15 insertions(+), 25 deletions(-) diff --git a/src/widgets/PopUpManager.js b/src/widgets/PopUpManager.js index 0765a7ca189..f9b4b5455b8 100644 --- a/src/widgets/PopUpManager.js +++ b/src/widgets/PopUpManager.js @@ -34,32 +34,14 @@ define(function (require, exports, module) { var EditorManager = require("editor/EditorManager"); var _popUps = []; - - function _removePopUp($popUp, index, visible) { - var autoAddRemove = $popUp.data("PopUpManager-autoAddRemove"), - removeHandler = $popUp.data("PopUpManager-removeHandler"); - - visible = visible || $popUp.find(":visible").length > 0; - - if (removeHandler && visible) { - removeHandler(); - } - if (autoAddRemove) { - $popUp.remove(); - _popUps = _popUps.slice(index); - } - } - - - /** * Add Esc key handling for a popup DOM element. * * @param {!jQuery} $popUp jQuery object for the DOM element pop-up * @param {function} removeHandler Pop-up specific remove (e.g. display:none or DOM removal) - * @param {?Boolean} autoAddRemove - Specify true to indicate the PopUPManager should - * add/remove the popup from the DOM when the popup is open/closed. Specifiy false + * @param {?Boolean} autoAddRemove - Specify true to indicate the PopUpManager should + * add/remove the popup from the DOM when the popup is open/closed. Specify false * when the popup is either always persistant in the DOM or the add/remove is handled * external to the PopupManager * @@ -83,11 +65,19 @@ define(function (require, exports, module) { * @param {!jQuery} $popUp */ function removePopUp($popUp) { - var index = _popUps.indexOf($popUp[0]), - removeHandler = $popUp.data("PopUpManager-removeHandler"); - + var index = _popUps.indexOf($popUp[0]); if (index >= 0) { - _removePopUp($popUp, index); + var autoAddRemove = $popUp.data("PopUpManager-autoAddRemove"), + removeHandler = $popUp.data("PopUpManager-removeHandler"); + + if (removeHandler && $popUp.find(":visible").length > 0) { + removeHandler(); + } + + if (autoAddRemove) { + $popUp.remove(); + _popUps = _popUps.slice(index); + } } } @@ -111,7 +101,7 @@ define(function (require, exports, module) { // Stop the DOM event from propagating keyEvent.stopImmediatePropagation(); - _removePopUp($popUp, i, true); + removePopUp($popUp); EditorManager.focusEditor(); }