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

Remove modal-wrapper whenever modal dialog is closed #5579

Merged
redmunds merged 6 commits into from
Oct 23, 2013
Merged

Remove modal-wrapper whenever modal dialog is closed #5579

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4680443
Fix dialogs not hiding properly
WebsiteDeveloper Oct 18, 2013
a0f4b15
Fix dialog button focus behavior
WebsiteDeveloper Oct 19, 2013
a0649c6
Fix travis build
WebsiteDeveloper Oct 19, 2013
0d76828
Simplify change
WebsiteDeveloper Oct 19, 2013
c252576
Fix JSLint errors
WebsiteDeveloper Oct 21, 2013
d79cc5e
Fix review issues
WebsiteDeveloper Oct 23, 2013
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
23 changes: 16 additions & 7 deletions src/widgets/Dialogs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ define(function (require, exports, module) {
function _dismissDialog($dlg, buttonId) {
$dlg.data("buttonId", buttonId);
$dlg.modal("hide");
$(".modal-wrapper:last").remove();
}

/**
Expand Down Expand Up @@ -120,9 +119,10 @@ define(function (require, exports, module) {
* @return {boolean}
*/
var _keydownHook = function (e, autoDismiss) {
var $primaryBtn = this.find(".primary"),
buttonId = null,
which = String.fromCharCode(e.which);
var $primaryBtn = this.find(".primary"),
buttonId = null,
which = String.fromCharCode(e.which),
$focusedElement = this.find(".dialog-button:focus, a:focus");

// There might be a textfield in the dialog's UI; don't want to mistake normal typing for dialog dismissal
var inTextArea = (e.target.tagName === "TEXTAREA"),
Expand All @@ -133,11 +133,17 @@ define(function (require, exports, module) {
} else if (e.which === KeyEvent.DOM_VK_ESCAPE) {
buttonId = DIALOG_BTN_CANCEL;
} else if (e.which === KeyEvent.DOM_VK_RETURN && !inTextArea) { // enter key in single-line text input still dismisses
// Click primary button
$primaryBtn.click();
// Click focused button or primary if no focus
if($focusedElement.length !== 0) {
Copy link
Contributor

@redmunds redmunds Oct 21, 2013

Choose a reason for hiding this comment

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

JSLint: Expected exactly one space between 'if' and '('.

$focusedElement.click();
Copy link
Contributor

@redmunds redmunds Oct 21, 2013

Choose a reason for hiding this comment

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

@RaymondLim brought up a good point (offline), so I'll comment here for him. Focused element should only receive click with space-bar, not Enter key.

Copy link
Contributor Author

@WebsiteDeveloper WebsiteDeveloper Oct 21, 2013

Choose a reason for hiding this comment

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

I thought about that too, but it is the default behavior in chrome and firefox so i decided to add it.
I also find it more convenient.

Copy link
Contributor Author

@WebsiteDeveloper WebsiteDeveloper Oct 21, 2013

Choose a reason for hiding this comment

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

@redmunds

Copy link
Contributor

@redmunds redmunds Oct 21, 2013

Choose a reason for hiding this comment

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

Yes, that seems to be the case. I'll wait until @RaymondLim responds.

Copy link
Contributor

@RaymondLim RaymondLim Oct 21, 2013

Choose a reason for hiding this comment

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

@WebsiteDeveloper
I agree with you that it is more convenient, but it is not standard both on Windows and Mac. You can try it in the Open File dialog on both platforms. On Windows, default button switches as you tab between OK and Cancel button. So it is not ambiguous when you hit Enter key when focus is on Cancel button. However, if you tab to other buttons (Organize, New Folder, More Options) and hit Enter key, you will see that default button takes action. On Mac, it is pretty clear that hitting Enter key on any button other than OK (default button) always takes the action of the default button.

Regarding your claim of default behavior in chrome and firefox, I'm not sure you're referring to any modal dialog that has that behavior. Let me know if you see that behavior in any modal dialog and not in a web page.

Copy link
Contributor Author

@WebsiteDeveloper WebsiteDeveloper Oct 21, 2013

Choose a reason for hiding this comment

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

will check tomorrow as it's already late here in germany

Copy link
Contributor Author

@WebsiteDeveloper WebsiteDeveloper Oct 23, 2013

Choose a reason for hiding this comment

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

I will rechange it.

} else {
$primaryBtn.click();
}
} else if (e.which === KeyEvent.DOM_VK_SPACE) {
// Space bar on focused button or link
this.find(".dialog-button:focus, a:focus").click();
if($focusedElement.length !== 0) {
Copy link
Contributor

@redmunds redmunds Oct 21, 2013

Choose a reason for hiding this comment

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

JSLint: Expected exactly one space between 'if' and '('.

$focusedElement.click();
}
Copy link
Contributor

@redmunds redmunds Oct 21, 2013

Choose a reason for hiding this comment

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

You don't need to check length here. If there is no focused element, then nothing happens, so you should change:

            if ($focusedElement.length !== 0) {
                $focusedElement.click();
            }

to:

            $focusedElement.click();

} else if (brackets.platform === "mac") {
// CMD+D Don't Save
if (e.metaKey && (which === "D")) {
Expand Down Expand Up @@ -259,6 +265,9 @@ define(function (require, exports, module) {

// Remove our global keydown handler.
KeyBindingManager.removeGlobalKeydownHook(keydownHook);

//Remove wrapper
$(".modal-wrapper:last").remove();
}).one("shown", function () {
// Set focus to the default button
var primaryBtn = $dlg.find(".primary");
Expand Down