Skip to content
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

Simplify the interaction with overlays by adding an OverlayManager #4823

Merged
merged 3 commits into from
May 27, 2014
Merged

Simplify the interaction with overlays by adding an OverlayManager #4823

merged 3 commits into from
May 27, 2014

Conversation

Snuffleupagus
Copy link
Collaborator

The current way that the overlays (PasswordPrompt and DocumentProperties) are handled are less than ideal for a number of reasons. Among those are:

  • Currently the code that is used to open/close an overlay needs to be duplicated for every one. If we want to add more overlays (e.g. UI for viewing and setting default preferences #3985), this will only get worse over time.
  • The way that the current overlays are implemented also means that adding another overlay would require code changes to all the existing ones.
  • There is no way to prevent multiple overlays being opened at the same time, which might lead to "interesting" results.

To avoid the issues described above, and to also unify and simplify how overlays work, this PR introduces the OverlayManager.

This PR is thus the one mentioned in #4777 (comment)

/cc @timvandermeij Would you mind reviewing this?

@timvandermeij
Copy link
Contributor

This PR looks really good. Other than the above nits, I couldn't find anything so far that would need improvement. I'll go over this again soon just to be sure that it's all good to go.

@Snuffleupagus
Copy link
Collaborator Author

@timvandermeij Compared to the first version, I've made a number of changes. Sorry about that!

  • The first version wasn't really as modular as it could/should have been. Hence I've changed it so that all overlays are no longer required to be placed in the same container. (As an added bonus, this slightly reduced the number of lines added by the PR.)
  • A small change which allows an overlay to also force close itself. This was done to ensure a consistent behaviour, and also to prevent any future issues.
  • Add/remove the keydown event listener when opening/closing overlays, since always having an event listener attached to the window seems unnecessary.
  • Address the review comments from the previous version.

@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

From: Bot.io (Linux)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/7f77d650bf80e44/output.txt

timvandermeij added a commit that referenced this pull request May 27, 2014
Simplify the interaction with overlays by adding an OverlayManager
@timvandermeij timvandermeij merged commit 03dff83 into mozilla:master May 27, 2014
@timvandermeij
Copy link
Contributor

Nice work!

@Snuffleupagus Snuffleupagus deleted the overlay-manager branch May 27, 2014 20:59
@Snuffleupagus
Copy link
Collaborator Author

@timvandermeij Thanks for merging!

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

Successfully merging this pull request may close these issues.

None yet

3 participants