Remove modal-wrapper whenever modal dialog is closed #5579
Conversation
|
Focusing with tab is working, but the arrow keys still don't work. |
|
@SAplayer well i plan to add the support for the arrow keys in a subsequent pull request. |
| // Click primary button | ||
| $primaryBtn.click(); | ||
| // Click focused button or primary if no focus | ||
| if($focusedElement.length !== 0) { |
There was a problem hiding this comment.
JSLint: Expected exactly one space between 'if' and '('.
|
Done with review. Nice refactoring! Just a couple JSLint errors to fix. |
|
@redmunds fixed. |
|
Note: I gave this a more descriptive subject. It's best to put bug numbers in description. |
|
Will do so in the future. |
| $primaryBtn.click(); | ||
| // Click focused button or primary if no focus | ||
| if ($focusedElement.length !== 0) { | ||
| $focusedElement.click(); |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, that seems to be the case. I'll wait until @RaymondLim responds.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
will check tomorrow as it's already late here in germany
There was a problem hiding this comment.
I will rechange it.
ghost
assigned
|
@WebsiteDeveloper Any progress on this PR? We really want to get this fixed in Sprint 33 which ends Friday. Thanks. |
|
@redmunds pushed changes. |
|
Looks good. Merging. |
Remove modal-wrapper whenever modal dialog is closed
Old Subject: Issue #5558 and #5548 Dialog fixes
More general fix for issue #5558 (see pull #5559 )
@redmunds
cc @peterflynn
Edit: Incorporated fix for #5548