search

twilio-labs / paste

Paste is a design system for designing and building consistent experiences at Twilio.
https://paste.twilio.design
MIT License
440 stars 114 forks source link

Alert Dialog should use Dialog #3860

Open cogwizzle opened 5 months ago

cogwizzle commented 5 months ago

Description

When I use the alert dialog component, I expect that it will utilize the built in dialog element, instead it is using a bunch of divs in order to create the a dialog.

Link to Reproduction

https://paste.twilio.design/components/alert-dialog

Steps to reproduce

  1. Go to 'https://paste.twilio.design/components/alert-dialog#default-alert-dialog'
  2. Click on 'Open Alert Dialog'.
  3. Inspect Element.

Paste Core Version

latest

Browser

Google Chrome 123.0.6312.124

Operating System

Additional Information

Using the semantic elements moving forward will improve the accessibility of the applications that use this library. Potentially we should use a div if dialog does not exist, but if it does we should use dialog.

TheSisb commented 5 months ago

Hi @cogwizzle,

Thank you for the suggestion!

First an explanation: our Modal component is currently powered by https://reach.tech/dialog/, which claims to be "the accessible foundation of your React-based design system". We used this library because our Modal component was created roughly 4 years ago. In that time, browser vendors have added the native Dialog element. This new element received baseline support in browsers within the past couple years. So we aren't using the native Dialog element because it was released after we needed it.

Our current implementation has served us well and we haven't received any complaints about its usage, performance, or accessibility. Now, that isn't to say that we don't think it's worth exploring migrating to the native element. However, we must be careful with any changes in our system to avoid potential breaking changes. A swap of the underlying technology could lead to unintended consequences to our downstream teams. For that reason we have to consider a broad range of factors when considering work like this, such as the benefits of the change, how reliable the solution is (do each browsers implement quirks?), feature parity, guardrails to prevent doing too much with it, the impact-to-effort ratio, where this work falls on our priority list and roadmap, and so on.

Are you testing this component with NVDA or Jaws? Is there anything you noticed was missing or improperly implemented? Providing that kind of information can help us make better decisions when we rebalance our backlog.

Thanks!

cogwizzle commented 5 months ago

Are you testing this component with NVDA or Jaws? Is there anything you noticed was missing or improperly implemented? Providing that kind of information can help us make better decisions when we rebalance our backlog.

I am not testing with either tool. I happened to be utilizing Paste in a product (I'm a Twillion) and I noticed some odd accessibility issues first with Combobox. Then I kind of just started browsing the library and started noticing other potential problems. I was basing my recommendations on my previous experiences with div based modals / popups.

For that reason we have to consider a broad range of factors when considering work like this, such as the benefits of the change, how reliable the solution is (do each browsers implement quirks?), feature parity, guardrails to prevent doing too much with it, the impact-to-effort ratio, where this work falls on our priority list and roadmap, and so on.

I agree with this assessment 100%. We should consider the implications of browsers within a reasonable range. I understand the div technique was the way to go up until recently within the last few years. I do believe that the transition will provide a more seamless maintenance long term for the implementors of Paste as well as provide the most accessible experience.

Finally my reasoning for bringing it up was that I wanted to push stuff onto the radar of the Paste team rather than remaining silent about these things. I understand we can't always do everything that may need to happen as time is finite. Ultimately though I don't have visibility into the current workload of the Paste team so I couldn't know if it could be able to be prioritized. I do ultimately think small tweaks like this have a wide reach since this is fundamentally a foundational library used in many places. I hope that makes sense from a consumer's perspective.

serifluous commented 5 months ago

(Shadi can totally continue replying from an eng perspective but just wanted to chime in too)

Finally my reasoning for bringing it up was that I wanted to push stuff onto the radar of the Paste team rather than remaining silent about these things. I understand we can't always do everything that may need to happen as time is finite.

We toooootally appreciate this. Hope you don't see our responses as pushback for the sake of pushback! We just want to set expectations with everyone about what we can address and improve in a realistic amount of time, and why it might end up falling where it does in our priority list (so they can better plan their roadmaps too). Appreciate the suggestions and shared motivation of trying to make all our customer experiences better 🙏