mtlynch
commented
3 months ago Yeah, this is looking promising to me. I like replacing home-rolled stuff for standardized functionality the browser is implementing for us already. It's less code that we have to own and lower probability that we've missed a corner case, especially in places where we don't have a lot of experience like a11y.
jotaen4tinypilot
Resolves https://github.com/tiny-pilot/tinypilot/issues/1699.
This PR replaces our home-brewed dialog implementation with the native
<dialog>element.Some notes on the branch:
<dialog>API are slightly different, although our own solution apparently also wasn’t wildly off track. At least the backdrop / shadow layer is noticeably simplified.<dialog>comes with a few CSS presets that we have to account for. E.g., we have to unset themax-height, to prevent the browser from scrolling inside the dialog. We also have to adjust thewidthimplementation.OverlayTrackerfor preventing keystroke forwarding while a dialog is open. However, we cannot reliably interceptkeydownevents, as those are only generated on specific elements, so we they don’t necessarily pass through the dialog element.Some learnings about
<dialog>in general:<dialog>has some useful default behaviour, e.g. when it comes to intercepting outside clicks, which it does automatically in “modal” mode. It also has better accessibility.<dialog>automatically closes itself it the user pressesESC. I’m not sure what to think of potentially reconsidering that decision, but at least for now I’d leave it as is (i.e., no close onESC).<dialog>has some automagic behaviour if the dialog contains a<form>(which we use e.g. in the security dialog). I’ve tested the relevant dialogs in Pro, and couldn’t spot any problems/interferences.(This PR was a draft initially.)