Skip to content

Fixes Ticket #6966 #134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Fixes Ticket #6966 #134

wants to merge 4 commits into from

Conversation

fracmak
Copy link
Contributor

@fracmak fracmak commented Feb 25, 2011

When multiple dialogs are open, pressing the escape key closes all open dialogs instead of the top most one. This checkin fixes that and adds a unit test for it.

@scottgonzalez
Copy link
Member

Thanks for the updated patch. However this seems to only work if you destroy the dialog on close. Removing the close callback causes the test to fail.

@fracmak
Copy link
Contributor Author

fracmak commented Mar 1, 2011

the test fails because the last test isn't an isOpen test, but a data("dialog") exists test, because having the close: function(){d2.remove()} actually blows away the data for the dialog, so I can't run a isOpen check without getting an initialization error. The code fix is still valid, I can fix the test if you'd like to make it work without the close handler, or add another test to show that the fix works under both scenarios, just let me know what you'd prefer

@scottgonzalez
Copy link
Member

I manually tested the patch before and it didn't work if I didn't destroy the dialog on close. I'll try again later today and let you know if I continue having a problem.

@fracmak
Copy link
Contributor Author

fracmak commented Mar 1, 2011

I added an extra set of tests without the close handler to help out diagnosing the issue you were seeing. Let me know if I set up the second set of tests incorrectly.

@scottgonzalez
Copy link
Member

I tested again and I'm still seeing the problem when I test manually. I'm opening the default demo page then running the following in the console:
$( "<div>1</div>" ).dialog({ modal: true });
$( "<div>2</div>" ).dialog({ modal: true });
I then click on the topmost dialog (#2) and then press escape. Both dialogs close.
The unit tests are passing for me.

@fracmak
Copy link
Contributor Author

fracmak commented Mar 2, 2011

I've fixed the unit test to show it failing under more real world circumstances. Now that I see the problem, I've kind of hit a wall about how to fix it. The problem is that the event is bubbling, but the event object that bubbles is only the event code and the event type, not the heavy weight jquery event object which I've set preventDefault() on. preventDefault() only works when all the event handlers are on the same element. Returning false from the keydown handler solves the problem. So two things, is there something I'm missing when it comes to event bubbling? If not, I'd like some more incite into the scenario preventing me from preventing bubbling entirely. What is the use case where you have a modal dialog that has reacted to the escape key, and someone behind the dialog needs to react to the escape key (who doesn't just register itself with a dialog close event)

@scottgonzalez
Copy link
Member

We have a policy not to stop bubbling because it makes writing generic code very difficult. The underlying problem that you've discovered is actually something that causes bugs in several plugins. We'll need to find a way to fix this in jQuery core.

I'm going to close this pull request in hopes that we can find a better solution. I'll leave a comment on the ticket about your findings and a link to this pull request.

Thanks.

@fracmak
Copy link
Contributor Author

fracmak commented Mar 7, 2011

So funny story, I was preparing a patch for jquery core to fix the issue, when I was surprised to see that they'd already fixed the issue in jquery 1.5.1. My unit tests pass using that version of jquery. So I'm reopening this ticket cause it's now a valid fix.

Jay Merrifield and others added 4 commits March 7, 2011 16:27
…are open, closes all dialogs, not the top one. Included unit test of issue
…dler, instead check if isDefaultPrevented() so we still bubble up the event, but give an indication to other dialogs that this event has been handled
…ill works as intended when noclose handler is added
…is breaks the unit test, so 'Fix' should be in quotes
@fracmak
Copy link
Contributor Author

fracmak commented Mar 7, 2011

I've rebased my branch to the latest master to show the unit tests working

@scottgonzalez
Copy link
Member

That's good news :-) I'll test this out tomorrow morning.

@scottgonzalez
Copy link
Member

Thanks, landed in f999668.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants