Convert the existing overlays to use `<dialog>` elements (issue 14698) #14710

Snuffleupagus · 2022-03-23T21:15:29Z

Please refer to the individual commit messages for additional details.

Snuffleupagus · 2022-03-23T22:13:03Z

/botio-linux preview

Snuffleupagus · 2022-03-24T10:56:53Z

/botio integrationtest

pdfjsbot · 2022-03-24T10:56:54Z

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/54bc80e2b6db665/output.txt

pdfjsbot · 2022-03-24T10:56:54Z

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/83ab7bce4bf095b/output.txt

pdfjsbot · 2022-03-24T11:00:58Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/54bc80e2b6db665/output.txt

Total script time: 4.06 mins

Integration Tests: FAILED

pdfjsbot · 2022-03-24T11:04:59Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/83ab7bce4bf095b/output.txt

Total script time: 8.07 mins

Integration Tests: Passed

calixteman · 2022-03-24T11:37:56Z

I tested in windows 11 in using high contrast themes (I selected "Dusk") and there is potentially a regression.

In Firefox nightly, in using http://54.241.84.105:8877/4c5d97f84b91cdd/web/viewer.html:

In Firefox nightly, with the builtin viewer:

In edge, in using http://54.241.84.105:8877/4c5d97f84b91cdd/web/viewer.html:

@Snuffleupagus, do you think it's an issue on our side or something wrong with dialog itself or something else ?
@MReschenberg, what is the correct way to display this dialog ?

Snuffleupagus · 2022-03-24T11:50:09Z

do you think it's an issue on our side or something wrong with dialog itself or something else ?

Given that I didn't really change the CSS all that much, and that the regular prefers-color-scheme: dark mode appears to work, I'd be inclined to assume that it's actually a platform issue with the <dialog> element here.

Edit: It almost seem as if the color and background-color properties are being ignored, for the <dialog> element, when the high contrast theme is enabled.

MReschenberg · 2022-03-24T15:05:11Z

We should be using the version that respects user-set colors (so the non-black and white one, if you're running Dusk theme). If this happens on all dialogs, it is probably a platform bug -- do we have something on file for that in bugzilla?
edit: also thanks for testing with HCM 😀

Snuffleupagus · 2022-03-25T10:41:54Z

I tested in windows 11 in using high contrast themes (I selected "Dusk") and there is potentially a regression.

While I'm currently on an older Windows version, I can reproduce similar problems with dark high contrast themes in basic <dialog> examples as well; see e.g. https://css-tricks.com/some-hands-on-with-the-html-dialog-element/
The examples are broken in Firefox Nightly 100, but work in Google Chrome Beta 100, which to me clearly suggests a browser bug.

timvandermeij · 2022-03-27T14:14:50Z

Given that this is an upstream issue, should we just continue with this or do we need to await a bugfix upstream?

marco-c · 2022-03-27T14:18:56Z

I think we should await for a fix upstream. The first step would be to file a bug on Bugzilla. @Snuffleupagus could you do that?

calixteman · 2022-03-27T14:23:20Z

I filed a bug upstream yesterday:
https://bugzilla.mozilla.org/show_bug.cgi?id=1761611

This replaces our *custom* overlays with standard `<dialog>` DOM elements, see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dialog, thus simplifying the related CSS, HTML, and JavaScript code. With these changes, some of the functionality of the `OverlayManager` class is now handled natively (e.g. `Esc` to close the dialog). However, since we still need to be able to prevent dialogs from overlaying one another, it still makes sense to keep this functionality (as far as I'm concerned).

This way we're able to store the `<dialog>` elements directly, which removes the need to use manually specified name-strings thus simplifying both the `OverlayManager` itself and its calling code.

…es` dialogs This will hopefully improve the a11y a little bit in these dialogs, however there's most definately more things that can be done here (by someone more knowledgeable about a11y).

Please note that this patch is purposely quite basic, e.g. it doesn't add the polyfill-CSS in order to simplify the build process, and things such as `::backdrop` thus isn't working. However, this patch does ensure that older browsers can at least still *access* all of the previous overlays and that things like e.g. opening of password-protected documents respectively printing still works.

Snuffleupagus · 2022-03-29T14:14:09Z

https://bugzilla.mozilla.org/show_bug.cgi?id=1761611 has already been fixed in Firefox Nightly, and the HTML standard was updated accordingly as well, hence that should no longer block this PR as far as I can tell :-)

/botio-linux preview

pdfjsbot · 2022-03-29T14:14:10Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/267a19444d3a822/output.txt

pdfjsbot · 2022-03-29T14:16:43Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/267a19444d3a822/output.txt

Total script time: 2.54 mins

Published

Viewer: http://54.241.84.105:8877/267a19444d3a822/web/viewer.html
Viewer (legacy): http://54.241.84.105:8877/267a19444d3a822/legacy/web/viewer.html

calixteman · 2022-03-29T16:19:49Z

In using Night sky theme, I noticed that the buttons have a yellow border line (like the text) but in the dialog it's purple.
And in opening the viewer in edge and in mouse-hovering the button, I get:

The printing UI in firefox (and in settings UI), switches bg to yellow and fg to black on mouse-hover, I wonder if we should do the same for the dialog in pdf.js, just to be consistent.

Snuffleupagus · 2022-03-29T16:30:53Z

And in opening the viewer in edge and in mouse-hovering the button, I get:

That sounds a lot like issue #14649, which honestly feels unrelated to this PR.

[...] I wonder if we should do the same for the dialog in pdf.js, just to be consistent.

In this PR, I've purposely tried to only convert the existing overlays while not changing e.g. the look of things.
Any further improvements, or re-designs, would seem more appropriate for follow-up work and we'd probably need an actual design-spec to follow here.

calixteman

Looks good to me.
Thank you for doing that.

Snuffleupagus added the viewer label Mar 23, 2022

Snuffleupagus linked an issue Mar 23, 2022 that may be closed by this pull request

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

Closed

Snuffleupagus force-pushed the overlays-dialog branch 2 times, most recently from 374c655 to 320f2f0 Compare March 23, 2022 22:03

Snuffleupagus force-pushed the overlays-dialog branch 3 times, most recently from 3aad662 to 2362149 Compare March 27, 2022 13:42

Snuffleupagus added 4 commits March 28, 2022 11:36

Re-factor the OverlayManager class to use a WeakMap internally

923bd52

This way we're able to store the `<dialog>` elements directly, which removes the need to use manually specified name-strings thus simplifying both the `OverlayManager` itself and its calling code.

Try to improve a11y for the PasswordPrompt and `PDFDocumentProperti…

b3d58e1

…es` dialogs This will hopefully improve the a11y a little bit in these dialogs, however there's most definately more things that can be done here (by someone more knowledgeable about a11y).

Snuffleupagus force-pushed the overlays-dialog branch from 2362149 to c9f262c Compare March 28, 2022 09:36

mozilla deleted a comment from pdfjsbot Mar 29, 2022

calixteman approved these changes Mar 29, 2022

View reviewed changes

Snuffleupagus merged commit d6592b5 into mozilla:master Mar 30, 2022

Snuffleupagus deleted the overlays-dialog branch March 30, 2022 09:47

Snuffleupagus mentioned this pull request Apr 2, 2022

Set focus into the "Document Properties" dialog #9560

Closed

Snuffleupagus mentioned this pull request Jun 4, 2022

Bundle the <dialog> polyfill-CSS in the GENERIC legacy/-viewer (PR 14710 follow-up) #14984

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert the existing overlays to use `<dialog>` elements (issue 14698) #14710

Convert the existing overlays to use `<dialog>` elements (issue 14698) #14710

Snuffleupagus commented Mar 23, 2022

Snuffleupagus commented Mar 23, 2022

Snuffleupagus commented Mar 24, 2022

pdfjsbot commented Mar 24, 2022

pdfjsbot commented Mar 24, 2022

pdfjsbot commented Mar 24, 2022

pdfjsbot commented Mar 24, 2022

calixteman commented Mar 24, 2022

Snuffleupagus commented Mar 24, 2022 •

edited

Loading

MReschenberg commented Mar 24, 2022 •

edited

Loading

Snuffleupagus commented Mar 25, 2022 •

edited

Loading

timvandermeij commented Mar 27, 2022

marco-c commented Mar 27, 2022

calixteman commented Mar 27, 2022

Snuffleupagus commented Mar 29, 2022

pdfjsbot commented Mar 29, 2022

pdfjsbot commented Mar 29, 2022

calixteman commented Mar 29, 2022

Snuffleupagus commented Mar 29, 2022 •

edited

Loading

calixteman left a comment

Convert the existing overlays to use <dialog> elements (issue 14698) #14710

Convert the existing overlays to use <dialog> elements (issue 14698) #14710

Conversation

Snuffleupagus commented Mar 23, 2022

Snuffleupagus commented Mar 23, 2022

Snuffleupagus commented Mar 24, 2022

pdfjsbot commented Mar 24, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Mar 24, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Mar 24, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Mar 24, 2022

From: Bot.io (Windows)

Success

calixteman commented Mar 24, 2022

Snuffleupagus commented Mar 24, 2022 • edited Loading

MReschenberg commented Mar 24, 2022 • edited Loading

Snuffleupagus commented Mar 25, 2022 • edited Loading

timvandermeij commented Mar 27, 2022

marco-c commented Mar 27, 2022

calixteman commented Mar 27, 2022

Snuffleupagus commented Mar 29, 2022

pdfjsbot commented Mar 29, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Mar 29, 2022

From: Bot.io (Linux m4)

Success

Published

calixteman commented Mar 29, 2022

Snuffleupagus commented Mar 29, 2022 • edited Loading

calixteman left a comment

Choose a reason for hiding this comment

Convert the existing overlays to use `<dialog>` elements (issue 14698) #14710

Convert the existing overlays to use `<dialog>` elements (issue 14698) #14710

Snuffleupagus commented Mar 24, 2022 •

edited

Loading

MReschenberg commented Mar 24, 2022 •

edited

Loading

Snuffleupagus commented Mar 25, 2022 •

edited

Loading

Snuffleupagus commented Mar 29, 2022 •

edited

Loading