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

Use <dialog> element for the document properties modal #14698

Closed
calixteman opened this issue Mar 21, 2022 · 7 comments · Fixed by #14710
Closed

Use <dialog> element for the document properties modal #14698

calixteman opened this issue Mar 21, 2022 · 7 comments · Fixed by #14710
Labels

Comments

@calixteman
Copy link
Contributor

It has been recently released in Firefox:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog
and it's available in Safari too:
https://webkit.org/blog/12445/new-webkit-features-in-safari-15-4/

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Mar 21, 2022

Looking at https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog#browser_compatibility, we probably need to wait a little while longer since we try and keep the GENERIC PDF.js library/viewer compatible with at least the current Firefox ESR version; please note https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#faq-support

Also, we really should be updating all of the overlays then (e.g. the PasswordPrompt as well) and not just the PDFDocumentProperties one. This work will likely require some refactoring, and allow some simplification, of the OverlayManager.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Mar 22, 2022

Based on some quick tests, replacing our custom-overlays with <dialog> elements seems to mostly work fine (although it will require some refactoring).
The only thing that didn't "just work" is the horizontal/vertical centering of the <dialog>s, since they end up at the top-left corner of the window, and I don't really understand why!?

@calixteman
Copy link
Contributor Author

@Snuffleupagus, would you mind to push what you did in order to try to figure out what's wrong with the positioning ?

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Mar 22, 2022

I was, perhaps naively, assuming that by default the <dialog> elements would be centered automatically, since that's how all of the examples I found online seemed to work (although most of those were quite small/simple).
I also tried to replicate the centering CSS used with the custom-overlays, but that didn't seem to help either when testing in both Firefox and Chrome.

would you mind to push what you did in order to try to figure out what's wrong with the positioning ?

Sure, please see https://github.com/mozilla/pdf.js/compare/master...Snuffleupagus:overlays-dialog?w=1 for a very basic patch (ignore the ugly hacks in the JS code).

@calixteman
Copy link
Contributor Author

It works in commenting:

margin: 0;

In chrome devtools:
image
I noticed that the dialog has margin set to auto when we've a 0 for all the elements.
So I just tried in unchecking and it worked.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Mar 22, 2022

I noticed that the dialog has margin set to auto when we've a 0 for all the elements.
So I just tried in unchecking and it worked.

Thank you; it seems that simply setting margin: auto; for the <dialog> CSS works fine :-)


There's still, as far as I'm concerned, the question if we should perhaps wait a little while longer with making this change (given that the feature is fairly new)?
Besides that, the "only" thing left to do for the patch would be to fix-up the OverlayManager functionality to properly handle things now that <dialog> elements are used.

@calixteman
Copy link
Contributor Author

I noticed that the dialog has margin set to auto when we've a 0 for all the elements.
So I just tried in unchecking and it worked.

Thank you; it seems that simply setting margin: auto; for the <dialog> CSS works fine :-)

There's still, as far as I'm concerned, the question if we should perhaps wait a little while longer with making this change (given that the feature is fairly new)?

In the meantime, using it helps to find some potential issues, hence it isn't so bad.
I filed two bugs about that:

Besides that, the "only" thing left to do for the patch would be to fix-up the OverlayManager functionality to properly handle things now that <dialog> elements are used.

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

Successfully merging a pull request may close this issue.

2 participants