dialog & ::backdrop - Githubissues

jensimmons commented 2 years ago

Description Implement / update implementations of the dialog element & ::backdrop pseudo-element.

Specifications

Tests dialog tests tests needed for tabindex +

bkardell commented 2 years ago

Really high interop on a newly landing in several browsers feature like this, very quickly, seems kind of good to me.

josepharhar commented 2 years ago

There was a recent change to the spec and tests here:

And here is a crbug for changing that behavior: https://bugs.chromium.org/p/chromium/issues/detail?id=383230 The new tests are already included in the same directory as the WPT link in the issue description.

foolip commented 2 years ago

Rationale The <dialog> element has been highly requested for years. If it's not appearing on recent surveys, it may just be because web developers have given up hope.

While <dialog> wasn't among the top categories in https://github.com/web-platform-tests/interop-2022/issues/7, there were a few mentions of it in State of CSS 2021. Twice in the "browser compatibilities" question, once for "other missing features", but I think most interestingly dialogs (native or custom) were brought up in the accessibility section:

keyboard trap für Dialoge

Appropriate focus handling (programmatically moving focus, retaining focus inside custom modal dialogs, etc)

Focus management (such as when a dialogue is open).

WCAG compliant components via JS (i.e. dialogs, etc).

There were also mentions of <dialog> in the MDN browser compat survey, most without much detail, but https://github.com/willmcpo/body-scroll-lock came up in an interview. It's sometimes used to prevent scrolling behind modals.

So even though the HTML element <dialog> isn't top of mind like Container Queries or Subgrid, problems that have to do with the use case do seem fairly prominent, and making <dialog> solid cross-browser should help with that, even if it takes a few years for the effect to be visible.

foolip commented 2 years ago

I just found an old email from @nt1m, who pointed out that we have some tests for <dialog> in Chromium: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/html/dialog/

Looking through the history, I see that some were already converted and upstreamed to WPT by @shanmuga-m: https://github.com/web-platform-tests/wpt/pull/7338 https://github.com/web-platform-tests/wpt/pull/7859 https://github.com/web-platform-tests/wpt/pull/8174

However, a bunch of the tests that remain probably still make sense to upstream. I don't know who might be able to work on that (if this proposal is accepted) but I will ask around.

josepharhar commented 2 years ago

@foolip here is a related crbug: https://bugs.chromium.org/p/chromium/issues/detail?id=1240798

jensimmons commented 2 years ago

We should make sure we use modern tests that match the HTML spec version that Mozilla & Apple are endorsing, and not the older version from the past that isn't accessible. IOW, I'm not sure that older tests are testing the right version of the spec.

jensimmons commented 2 years ago

I wanted to get another data point for a sense of whether or not this is important to web developers. After 6 years of doing this research non-stop, my instincts say yes, dialog is highly requested. But hey, looking for new information to double check long-held beliefs is always a good idea. So I wrote a tweet.

63% of responders said, yes, dialog is important to them. They will use it. It should be a priority.

More than that, however, the number of people who answered the poll tells me a lot. Over 2,000 people answered in 24 hours — which is far more than my quick informal twitter polls usually get.

There was excitement about and clarity in expressing an opinion about dialog. Fifty-two people replied as well, with why & how they'll use it. Read the replies to get a sense of how people feel.

People want this element. They will use it. They've been waiting a really long time. And they are going to be really frustrated if we don't have interop.

foolip commented 2 years ago

More than that, however, the number of people who answered the poll tells me a lot. Over 2,000 people answered in 24 hours — which is far more than my quick informal twitter polls usually get.

Thanks for pointing this out, I agree that's a strong signal in itself.

We should make sure we use modern tests that match the HTML spec version that Mozilla & Apple are endorsing, and not the older version from the past that isn't accessible. IOW, I'm not sure that older tests are testing the right version of the spec.

Do you know which spec changes were made around this? The WHATWG working mode involves adding tests, so PRs like https://github.com/web-platform-tests/wpt/pull/31523 might have made the necessary changes.

nt1m commented 2 years ago

Do you know which spec changes were made around this? The WHATWG working mode involves adding tests, so PRs like web-platform-tests/wpt#31523 might have made the necessary changes.

We're waiting to hear back from Google on https://github.com/whatwg/html/pull/4184#issuecomment-975773127

foolip commented 2 years ago

@nt1m thanks, I've asked internally about that PR and if there are any other <dialog> issues related to focus or accessibility that need attention. Do you know of others, or is some resolution of https://github.com/whatwg/html/pull/4184 the only outstanding issue here?

josepharhar commented 2 years ago

We should make sure we use modern tests that match the HTML spec version that Mozilla & Apple are endorsing, and not the older version from the past that isn't accessible. IOW, I'm not sure that older tests are testing the right version of the spec.

The only WPT which is explicitly concerned with the behavior in the proposed spec change is show-modal-focusing-steps.html.

I made a patch for the proposed change in chromium based on the firefox one in order to find out which WPTs would also fail, and I found that these tests also fail:

dialog-focus-shadow.html
dialog-cancel-with-select.html
dialog-showModal.html
inert-node-is-unfocusable.html
shadowdom-in-dialog.html

These tests aren't necessarily concerned with the proposed focus changes, and perhaps could even be changed to pass regardless of which focus behavior is implemented. Some have many subtests where only one fails with the proposed changes.

nt1m commented 2 years ago

I think we can probably just use autofocus on tests unrelated which rely on focusing the first item.

josepharhar commented 2 years ago

I have opened PRs to migrate the rest of the chrome-only dialog tests to WPT:

josepharhar commented 2 years ago

All of the dialog tests from my last comment have been merged! If there any issues with any of them I'm happy to address them, but we can at least collect the test names now. @foolip

foolip commented 2 years ago

Thanks @josepharhar! I've labeled all the tests (I think!) and you can see them here: https://wpt.fyi/results/html/semantics/interactive-elements/the-dialog-element?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&q=label%3Ainterop-2022-dialog

nt1m commented 2 years ago

dialog-focusing-steps-inert.html and dialog-inert.tentative.html should be unlabeled, they test the inert attribute which isn't in the HTML spec yet.

foolip commented 2 years ago

@nt1m done in https://github.com/web-platform-tests/wpt-metadata/pull/2303

nt1m commented 2 years ago

Can these 2 tests be labeled? https://github.com/web-platform-tests/wpt/commit/b3958a2b77988a3e4c6948dfce1b62f7b0ab04cf

They test ::backdrop + animation

foolip commented 2 years ago

@nt1m I've labeled those in https://github.com/web-platform-tests/wpt-metadata/pull/2327

foolip commented 2 years ago

A proposed list of tests has been labeled with interop-2022-dialog. It would be great if someone from each browser engine team can look over this list and comment if there are any issues.

josepharhar commented 2 years ago

I just looked through the tests, and it seems that modal-dialog-scroll-height.html fails in all browsers even though it passes locally for me with content_shell and chrome... maybe there is something different about the machines that wpt.fyi runs on? inert-label-focus.html also has a weird error, it says MISSING in chrome and firefox. I added both of these tests when I upstreamed the chrome dialog tests.

emilio commented 2 years ago

I'm technically on PTO, but from a quick look the tests that Firefox fail are about backdrop-animation (which is sort - of unrelated to dialog, it's about ~every non-element backed pseudo in Gecko, and affects :fullscreen too).

One that Safari + Firefox fail (the one about "dialog inside a replaced renderer") is pretty dubious... I don't think children of replaced elements generally render, even if they are in the top layer. What in the spec says they should?

A few others seem to depend on the inert attribute, which is not particularly related either (they pass on Firefox with the inert attribute enabled fwiw).

emilio commented 2 years ago

(Same about modal-dialog-in-table-column.html... seems dubious at least that should work. For context they come from https://bugs.webkit.org/show_bug.cgi?id=103477)

nt1m commented 2 years ago

I fixed inert-label-focus.html in https://github.com/web-platform-tests/wpt/pull/32308.

focus-after-close.html WebKit failure: not directly due to the dialog implementation, but rather because of intentional WebKit special scrolling behavior. It could be rewritten to make waiting for scrolling asynchronously, but not sure how to do that right (maybe @rniwa could take a look if they have time).
modal-dialog-in-replaced-renderer.html & modal-dialog-in-table-column.html: I agree with emilio here, this isn't really specified in the top layer spec. The tests were added in an older WebKit implementation (which has since been reverted).
modal-dialog-scroll-height.html: needs investigation/removal since it fails in all 3 browsers.

The rest seem like genuine bugs in WebKit which I am actively working on.

emilio commented 2 years ago

I'm technically on PTO, but from a quick look the tests that Firefox fail are about backdrop-animation (which is sort - of unrelated to dialog, it's about ~every non-element backed pseudo in Gecko, and affects :fullscreen too).

Well given this is <dialog> and ::backdrop these are legit I suppose :-)

josepharhar commented 2 years ago

I'll contact the author of modal-dialog-scroll-height.html and see if we can test the same behavior without the exact height expectation thats causing the failure

foolip commented 2 years ago

I've sent https://github.com/web-platform-tests/wpt-metadata/pull/2352 to drop modal-dialog-scroll-height.html from Interop 2022. If another test can be added to test the same thing in a better way soon, maybe we can include that.

josepharhar commented 2 years ago

I believe I found a way to fix modal-dialog-scroll-height.html by replacing the hard coded height with window.innerHeight: https://chromium-review.googlesource.com/c/chromium/src/+/3379191 https://github.com/web-platform-tests/wpt/pull/32319

josepharhar commented 2 years ago

@foolip Can you put modal-dialog-scroll-height.html back on the list? The new version is passing in all browsers: https://wpt.fyi/results/html/semantics/interactive-elements/the-dialog-element/modal-dialog-scroll-height.html

nt1m commented 2 years ago

Done: https://github.com/web-platform-tests/wpt-metadata/pull/2399/

foolip commented 2 years ago

One test came up in my analysis in https://github.com/web-platform-tests/interop-2022/issues/48. It looks like a solved problem, but quoting for visibility:

/html/semantics/interactive-elements/the-dialog-element/backdrop-receives-element-events.html is TIMEOUT in Safari with the single subtest being NOTRUN. It's linked to https://webkit.org/b/233072 which was recently fixed, confirmed with Safari + WebKitGTK.

kbrilla commented 2 years ago

What about :modal pseudoclass? https://chromestatus.com/feature/5192833009975296 https://www.w3.org/TR/selectors-4/#modal-state

foolip commented 2 years ago

@criskrzysiu that's newer than Interop 2022, and changes to the tests we include for scoring need to be reviewed, so that the target doesn't change much throughout the year.

But we have both added and removed tests, for various reasons. If you want to suggest that, you can file a new issue in this repo. Whether everyone is on board with it probably depends mostly on whether they already intend to ship :modal this year.

jgraham commented 2 years ago

Without knowing anything about Gecko's plans here, I don't like the idea of making major changes to the scope of a focus area during the year. I think test additions / removals should be scope-neutral and only used to address issues of correctness or completeness within the agreed boundaries of the focus area.

nt1m commented 2 years ago

WebKit and Gecko have both implemented :modal and I think only Chromium is not shipping it, but has a patch up.

I don't have strong opinions on including it in interop 2022 though.

kbrilla commented 2 years ago

Turns out there already is test that checks for :modal introduced by this PR https://github.com/web-platform-tests/wpt/pull/34107/files

kbrilla commented 2 years ago

update - :modal is shipping in Chrome106 Chromestatus

web-platform-tests / interop

dialog & ::backdrop #12