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

Overlay Drawer component #476

Merged
merged 5 commits into from
Sep 10, 2024
Merged

Overlay Drawer component #476

merged 5 commits into from
Sep 10, 2024

Conversation

echappen
Copy link
Contributor

@echappen echappen commented Sep 5, 2024

Changes proposed in this pull request:

  • Adds an OverlayDrawer component

How to test

  1. Spin up a local server (a CF Token is not needed)
  2. Navigate to /prototype/tables
  3. Click the "open dialog" button in the top left corner

Notes

  • Focus trapping is being handled natively by the element, so the trapping behavior is a bit different than our current USWDS modal in that it extends focus to browser buttons.

Screenshots

Open drawer:

Screenshot 2024-09-05 at 4 33 36 PM

Mobile:

Screenshot 2024-09-05 at 4 34 44 PM

Related issues

Part of #468

Submitter checklist

  • Added logging is not capturing sensitive data and is set to an appropriate level (DEBUG vs INFO etc)
  • Updated relevant documentation (README, ADRs, explainers, diagrams)

Security considerations

None, UI element only

@echappen
Copy link
Contributor Author

echappen commented Sep 5, 2024

@beepdotgov I'm still refining the animations (especially on close), but lmk how this is looking to you so far.

@beepdotgov
Copy link
Contributor

@echappen This is looking really stellar! I love how quickly you were able to get this up and running, and the skeleton’s really looking (and working) great so far.

A couple notes, if they’re helpful while you’re working:

  • The opening animation’s timing feels a little quick compared to the sketch? I might suggest playing around with a slight delay to start the animation, or maybe lengthening the timing to 0.3s.
  • The backdrop should be a little darker, if possible? (Looks like it’s a 10% black right now; it’d be great if that could be bumped up to 70%.)
  • The close button looks like it’s appearing immediately, rather than sliding in.

Excited to see this evolve, it already looks great!

@echappen
Copy link
Contributor Author

echappen commented Sep 6, 2024

@beepdotgov Thanks for taking a look! I just pushed some improvements to the animations.

Dialogs achieve the overlay with a ::backdrop pseudo-element. Animations apparently don't play nicely with ::backdrop in Firefox, which is why you weren't seeing the darker overlay. So while I was able to get a darker overlay in Firefox, I'm still not able to make the overlay transition at all for that browser (the dialog itself transitions, just not the background). Check it out on Chrome for a comparison. Your codepen does work on Firefox, and I suspect it's because it's using class selectors instead of ::backdrop. I'm not quite sure how to solve this. If Firefox is still not looking good enough for you, let's pair on it!

In other news, the button transition should be fixed, the background should be darker for all browsers, and the timing should now be more comparable to the Codepen. 🤞

@beepdotgov
Copy link
Contributor

@echappen Just two commits later, and this looks even better. Confirmed that the :backdrop isn’t animating/transitioning in Firefox, but I don’t think that should be a blocker here; everything else is working and looking so good, and the timing feels a lot smoother.

One question: when the PR’s ready, will we be able to dismiss the overlay by clicking on the :dialog?

Thanks again for the preview!

@echappen
Copy link
Contributor Author

echappen commented Sep 6, 2024

will we be able to dismiss the overlay by clicking on the :dialog?

@beepdotgov Not sure I comprehend—do you mean by clicking outside of the dialog? If so, then you read my mind because I was about to bring that up!...

Default dialog behavior is: when opening a dialog with the native showModal() function, “Interaction outside the dialog is blocked and the content outside it is rendered inert.” [from the MDN docs] This means that clicking outside the dialog won’t close it. This differs from our current USWDS Modal, where clicking outside will close it.

Do you think both modals should behave the same way? Happy to jump on a call to talk through.

@beepdotgov
Copy link
Contributor

do you mean by clicking outside of the dialog?

Oops, yes — that’s exactly what I meant, @echappen! (So sorry about that; that should’ve read “clicking on the :backdrop, not :dialog.)

Do you think both modals should behave the same way?

So reflexively, I would expect them to behave the same way. But! I don’t have any direct experience with dialog-based modals, and I’d hate to mess with any of the default accessibility ergonomics. How about this: we leave it as-is for now, and we can revisit in the future when/if we get some user feedback on the behavior?

@echappen
Copy link
Contributor Author

echappen commented Sep 6, 2024

@beepdotgov Thanks! And same, I'm also fairly new to dialog elements and am curious to see how they fare.

@echappen echappen changed the title WIP: Overlay Drawer component Overlay Drawer component Sep 6, 2024
@echappen echappen requested a review from cannandev September 6, 2024 18:00
@echappen echappen marked this pull request as ready for review September 6, 2024 18:00
@echappen echappen requested a review from a team as a code owner September 6, 2024 18:00
@echappen echappen requested a review from hursey013 September 6, 2024 18:00
@echappen
Copy link
Contributor Author

echappen commented Sep 9, 2024

@beepdotgov Please keep the feedback coming if you have any, but at this point, this is ready for a dev review @cannandev or @hursey013

@beepdotgov
Copy link
Contributor

@echappen I think this is still looking great! Two things I noticed:

  1. Right now, the close animation is only firing in Chrome; Firefox and Safari close the drawer immediately.
  2. In Firefox, it looks like tab focus is only partially trapped. I can’t tab to the parent page, but the entire browser chrome’s still accessible via tab. (Chrome and Safari both cycle between the overlay’s contents and the browser location bar.)

(Not sure if 2 is a browser implementation bug, or something we can address. Just thought I’d note it.)

@echappen echappen mentioned this pull request Sep 9, 2024
7 tasks
@echappen
Copy link
Contributor Author

close animation is only firing in Chrome; Firefox and Safari close the drawer immediately

@beepdotgov Thanks for catching! I thought I had solved the overlay animation issue in Firefox and it was just a matter of handling the background, but alas... After chatting with folks in engineering sync, we decided to mark this as a bug in a separate/non-blocking GH issue, since this behavior isn't preventing users from performing the tasks.

In Firefox, it looks like tab focus is only partially trapped

I haven't found any official documentation about this (only a bunch of SO threads), but it looks like certain browsers allow tabbing into the browser controls with dialog and others don't. (All still prevent tabbing into the rest of the document body.) For me, I found that only Safari traps focus completely within the dialog, while Firefox and Chrome allow tabbing into browser controls. There still seems to be debate about which is preferable to users. I'm torn—I could see good use cases for both. Currently I'm in favor of reducing our custom overhead by going with the browser defaults, and testing if users really want full trapping in every browser.

@beepdotgov
Copy link
Contributor

@echappen Sounds good on both fronts! Opening a non-blocking issue for the closing animation makes sense, and I’m all about leaning on browser defaults as much as possible. (Maybe we open a non-blocking issue for that too? Dunno, just thought it might be worth logging/tracking somewhere.)

Thank you so much for digging into this, and in so much detail!

@echappen
Copy link
Contributor Author

Also noting other things we decided in Engineering sync:

  • When opening the dialog, we'll keep focus on the close button and allow users to tab through the rest of the content.
  • Opening the dialog should also announce a title specific to the functionality being presented in the dialog.
  • When closing the dialog, we'll keep the default behavior of returning focus to the last point of regard.
  • When closing the dialog, we'll have SR's announce any success messages (while keeping default focus).
  • While dialog is open, we agreed that a user should be able to click outside of it to close it. Since this is not default behavior for dialogs, we're going to spike on this and see what we come up with.

@echappen
Copy link
Contributor Author

I think at this point, it'll be easier to work on the above items in this PR if we want to close this one

Copy link
Contributor

@cannandev cannandev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on what we talked about in engineering sync, LGTM! Other issues will be addressed in this issue

@echappen echappen merged commit dd57736 into main Sep 10, 2024
4 checks passed
@echappen echappen deleted the eoc/468-dialog branch September 10, 2024 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants