Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

fixes issues #1270 #1271 #1272, ESC key handling with popups #1276

Merged
merged 2 commits into from
Jul 18, 2012
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/command/Menus.js
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REMINDER: need to update new context menu unit test I added in pull request #1273 to uncomment line that checks for contextMenuClose event.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Randy, I've pushed my changes so you can update test in the pull request?

},
false);
}
ContextMenu.prototype = new Menu();
ContextMenu.prototype.constructor = ContextMenu;
Expand Down
47 changes: 19 additions & 28 deletions src/widgets/PopUpManager.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,49 +36,42 @@ 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;

if (removeHandler && visible) {
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: Specifiy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another typo: PopUPManager

* 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);
}
}
Expand All @@ -91,16 +84,15 @@ 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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removeHandler doesn't seem to be used in this function.


if (index >= 0) {
_removePopUp($popUp, index);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_removePopUp is a private function that's only called in 2 places. Seems unnecessary to have the 3rd parameter optional. Why not specify it here, then remove the logic that sets the default value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up removing _removePopup() function completely since complexity of having this helper function isn't worth the savings of reusing the "index" var. I don't expect there to be many visible popups at one time, so computing the index more than once isn't costly. I also made the other simplifications you suggested.

}
}

function _keydownCaptureListener(keyEvent) {
if (keyEvent.keyCode !== 27) {
if (keyEvent.keyCode !== 27) { // escape key
return;
}

Expand Down Expand Up @@ -132,5 +124,4 @@ define(function (require, exports, module) {

exports.addPopUp = addPopUp;
exports.removePopUp = removePopUp;
exports.configurePopUp = configurePopUp;
});