| 15:24 | <annevk> | Domenic: FYI: https://github.com/whatwg/storage/pull/130 also failed participation check (I usually fix these by just telling participate to update the PR) |
| 15:25 | <annevk> | Domenic: we should maybe have a better endpoint for that. E.g., I can browse to https://participate.whatwg.org/agreement-status?user=annevk&repo=storage and submit the URL of that PR there and it all works out, but it's a lil weird to have to fill in a (random) user and repo. |
| 15:26 | <Domenic> | The logs look fine... the webhook payload was delivered and the server sent back a 204 |
| 15:27 | <Domenic> | I guess we could add more logging, and maybe send that back instead of a 204 |
| 15:28 | <Domenic> | Like maybe some random thing happened which caused one of the early bailouts in this function to be hit before we got to createCommitStatus(): https://github.com/whatwg/participate.whatwg.org/blob/main/lib/pr-webhook.js#L6-L40 |
| 15:36 | <ntim> | Domenic: wdyt of a spec change to prevent focusing steps on disconnected <dialog>? This is already what we ship in STP fwiw: https://wpt.fyi/results/html/semantics/interactive-elements/the-dialog-element/dialog-focusing-steps-disconnected.html?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&q=the-dialog-element |
| 15:37 | <ntim> | https://html.spec.whatwg.org/multipage/interactive-elements.html#dialog-focusing-steps |
| 15:37 | <Domenic> | ntim: I think the spec already does that. Or rather, it runs the focusing steps, but because it finds nothing focusable, the focusing steps are a no-op. |
| 15:37 | <ntim> | "If subject is inert, return." -> "If subject is disconnected or inert, return. |
| 15:38 | <Domenic> | The spec even has a note: "If control is not focusable, this will do nothing. For modal dialogs, this means that any earlier modifications to the focused area of the document will apply." |
| 15:38 | <Domenic> | (But, disconnected modal dialogs already threw in step 3 of showModal()) |
| 15:39 | <ntim> | wondering why the test fails in Chrome & FF |
| 15:39 | <Domenic> | Yeah fair question |
| 15:40 | <Domenic> | Looks like Chrome resets focus to body |
| 15:41 | <ntim> | yeah, it could have worked this way as well in STP, it was an intentional choice from me to just skip focusing steps |
| 15:42 | <ntim> | Domenic: I think "Run the focusing steps for control." means focusing the body when control is disconnected for current implementations |
| 15:42 | <Domenic> | Oh, that's weird |
| 15:42 | <ntim> | (FF included) |
| 15:44 | <Domenic> | ntim: not per my testing, in FF and Chrome: https://jsbin.com/fokusafumi/edit?html,console,output |
| 15:45 | <ntim> | that's weird |
| 15:46 | <ntim> | if I remove https://webkit-search.igalia.com/webkit/source/Source/WebCore/html/HTMLDialogElement.cpp#126-127 from webkit, it also does the same as ff and chrome |
| 15:52 | <ntim> | Domenic: oh, the old version of the spec had this: https://webkit-search.igalia.com/webkit/source/Source/WebCore/html/HTMLDialogElement.cpp#154-155 |
| 15:52 | <ntim> | which basically sets focus to body |
| 15:53 | <Domenic> | Ah, fascinating |
| 15:54 | <Domenic> | Our earliest commit snapshot (i.e. the first version of the spec after the move to GitHub, in 2015) doesn't seem to have such a step: https://html.spec.whatwg.org/commit-snapshots/c9e804f04d03a0658bfa689cb0f368a4d2e37936/#dialog-focusing-steps |
| 15:58 | <ntim> | https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/html_dialog_element.cc;l=73-77;drc=de68be3f18ba99cc01d75903e167ca09bade253c?q=html_dialog_element&ss=chromium |
| 15:58 | <ntim> | chromium does this too |
| 15:58 | <ntim> | let me remember the context |
| 16:00 | <ntim> | similar stuff in FF: https://searchfox.org/mozilla-central/source/dom/html/HTMLDialogElement.cpp#186-197 |
| 16:03 | <ntim> | Domenic: from what I recall, that made sense to me because otherwise focus would stay on an inert element in the showModal case |
| 16:03 | <ntim> | but yeah right it's not part of the spec |
| 16:03 | <Domenic> | Hmm interesting to think about the showModal case |
| 16:04 | <Domenic> | I'm happy to just fix Chromium to pass the test you pointed out since it seems more correct anyway... but yeah I wonder what the consequences would be of deleting that line. |
| 16:05 | <ntim> | I think the risk is to keep focus on an element behind the dialog |
| 16:05 | <ntim> | in the event that the control is not focusable |
| 16:06 | <ntim> | and in the showModal case that might be bogus |
| 16:10 | <Domenic> | So the spec solves that in another way, which is the focus fixup rule |
| 16:10 | <Domenic> | When the modal dialog makes everything inert, we hit https://html.spec.whatwg.org/#focus-fixup-rule |
| 16:10 | <Domenic> | See https://html.spec.whatwg.org/#dom-dialog-showmodal step 6 |
| 16:11 | <Domenic> | But focus fixup rule is very "action at a distance" so I can easily believe implementations need more special code for that |
| 16:11 | <Domenic> | I think I remember adding that paragraph explaining the focus fixup rule connection so hopefully we added tests at the same time... |
| 19:45 | <ntim> | Domenic: so I guess the right logic should be else if (is_modal) { setFocusedElement(nullptr) } |
| 19:46 | <ntim> | I guess those specific LoC were specifically to implement the fixup rule |
| 19:59 | <Domenic> | Yes, sounds right to me |
| 20:08 | <ntim> | Domenic: cool, are you planning to fix this in Chromium? |
| 20:08 | <Domenic> | ntim: yeah, we have a bunch of changes queued up for after https://github.com/whatwg/html/pull/7361 lands |
| 20:09 | <ntim> | just wondering if i should write WPTs |
| 20:09 | <ntim> | Domenic: oh I see you have a WPT PR: https://github.com/web-platform-tests/wpt/pull/31724 |
| 20:10 | <Domenic> | ntim: didn't this conversation start with an existing WPT you already wrote? i.e. https://wpt.fyi/results/html/semantics/interactive-elements/the-dialog-element/dialog-focusing-steps-disconnected.html?label=master&label=experimental&product=chrome&product=firefox&product=safari&aligned&q=the-dialog-element :) |
| 20:12 | <ntim> | Domenic: planning to make this change in WebKit too: https://webkit-search.igalia.com/webkit/source/Source/WebCore/html/HTMLDialogElement.cpp#126-127,154-155 |
| 20:12 | <Domenic> | Awesome, great to hear |
| 20:13 | <ntim> | Domenic: does it still make sense to have "If subject is inert, return."? |
| 20:13 | <ntim> | wouldn't that be covered by the fixup rule? |
| 20:13 | <Domenic> | Interesting. Yes, I think you're right. |
| 20:14 | <ntim> | Domenic: wondering if this changes anything for non modal dialogs |
| 20:15 | <ntim> | (thinking about which tests I should write) |
| 20:18 | <Domenic> | Maybe we should just update https://github.com/web-platform-tests/wpt/pull/31724 to focus a random (connected) element on the page between tests. And if there is no focus candidate (which occurs in one or two of the tests) then we test that that element is focused, instead of testing that it's body. I.e. test that it didn't get reset to body. |
| 20:21 | <ntim> | yeah that makes sense, it could alternate between body and an element with explicitly focus on |
| 20:22 | <Domenic> | Done in https://github.com/web-platform-tests/wpt/pull/31724/commits/6cbfb3b76b68ef9eec9e44544ac424c8b6eb7d48 |
| 20:23 | <Domenic> | Like I told sefeng on that PR, feel free to grab those tests and use them locally / export them after, instead of waiting for merge on the WPT repo. Writing tests separate from implementation is a bit suboptimal here so I am happy to just provide a headstart for the people doing the implementation. |
| 20:35 | <ntim> | Domenic: https://github.com/whatwg/html/pull/7366 |
| 20:35 | <Domenic> | Awesome, thank you |
| 20:37 | <ntim> | np, i haven't filled in implementation bugs, tests, since i assume this will probably be fixed in batch as part of the focus changes |
| 20:38 | <ntim> | chromium already omits this step, nice |
| 20:41 | <ntim> | Domenic: looks like Chromium & Firefox already don't have this step, so no need for impl bugs |
| 20:43 | <Domenic> | Perfect |
| 20:51 | <ntim> | Domenic: thanks! can you merge the PR? |
| 20:52 | <Domenic> | Done |
| 20:53 | <ntim> | thanks! |