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

Handle keyboard events for nested dialogs properly #3522

Closed
wants to merge 1 commit into from
Closed

Conversation

njx
Copy link
Contributor

@njx njx commented Apr 21, 2013

Instead of attaching a keydown listener for each dialog, keep track of the stack of nested dialogs and have the listener always deal with the topmost dialog. Fixes #3505.

(Ideally we wouldn't have nested dialogs at all, but the current Extension Manager implementation requires it. In the future, we might get rid of one level of dialogs there.)

Note that when #3086 (or similar) is merged, we can clean this up a bit by just having the stack hold Dialog objects and storing autoDismiss in the object.

As an aside, we should add some unit tests for the dialog infrastructure. Maybe the right time to do that is when we review #3086.

@peterflynn
Copy link
Member

@njx should we not bother merging this until a later sprint, given the change in Extension Manager plans?

@njx
Copy link
Contributor Author

njx commented Apr 23, 2013

Yup, no need to review this now.

@iwehrman
Copy link
Contributor

iwehrman commented May 3, 2013

For comparison, here's my (very quick, possibly buggy, probably incomplete) solution: b79284a

@peterflynn
Copy link
Member

@njx do you have a preference between this and @iwehrman's implementation? Ian's is definitely a simpler diff. We'd need to port over your Esc handling fix, and probably do some minor cleanups (e.g. add JSDocs and leading "_" to keydownListeners), but that might be enough to do the trick.

@njx
Copy link
Contributor Author

njx commented May 10, 2013

Yeah, I like Ian's better, though I haven't looked to see if it's equivalent. @iwehrman, mind submitting yours to the brackets repo?

@njx
Copy link
Contributor Author

njx commented May 14, 2013

Closing in favor of #3785.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Install from URL" popup from Extension Manager won't accept keyboard input
3 participants