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

Discussion for v4 of react-modal #881

Closed
diasbruno opened this issue Jun 2, 2021 · 23 comments
Closed

Discussion for v4 of react-modal #881

diasbruno opened this issue Jun 2, 2021 · 23 comments

Comments

@diasbruno
Copy link
Collaborator

diasbruno commented Jun 2, 2021

First, thanks for all the support through the years...reporting and answering issues, or submitting PRs. You guys are great.

I try my best to find time to help improve this package, but I can't do it alone...

I have a dream to finally release version 4 for react-modal, but I didn't have a good goal to invest time to push it forward.

The goals for v4:

  1. Remove from the API everything that can disable accessibility

It would be better to have a more stable releases without breaking accessibility.

  1. Fix the bogus default behavior related to hide the application.

The default is to apply the aria-hidden attribute to the document.body. But this hides everything

This will require the user to explicitly define the element(s), using its id or class or element, using Modal.setAppElement().

To normalize the API, Modal.setAppElement() and appElement={} must use arrays to keep the API simple.

  1. Deprecate ariaHideApp

This is an unnecessary flag, this behavior can be archived by passing [] to Modal.setAppElement() and appElement={}. But it shouldn't be recommended.

  1. Deprecate shouldFocusAfterRender={boolean}

The modal must always gain focus and trap (correctly).

  1. Replace shouldReturnFocusAfterClose={boolean} with returnFocusTo={Element}

This can be an element that will gain focus after the modal is closed.

The default behavior is to return the focus to the element on document.activeElement before open the modal, but...in some browsers, this behavior is broken so we can't always trust it. So, the user can pass to this attribute an element that must receive the focus later.

  1. Review all additional systems that react-modal uses (see helpers folder)

This package manages a lot of state (like open instances, styles classes added to external elements, focus system...)


With this goals in mind, the API will be less flexible to disable the features related to accessibility, which should not be a problem, but maybe some users will not see this a gain...So, I decided to start a discussion before moving forward with this...

I'll let this open until the end of Jun, and if the feedback is positive, I'll set up the project so everybody can participate!

Thanks,
Dias

@TusharShahi
Copy link

Point 5 gives the developer good control over choosing accessibility options and should help with browsers like Safari. But the default behaviour is definitely easier and in a way auto enables accessibility.

With the new approach, developers will have to take the responsibility of managing focus in all places. Might I suggest to keep the default behaviour as is, and only override it if the user has explicitly mentioned the prop. Since, in most browsers returning focus to activeElement will work, it will lead to less code for the users and they can choose to pass the element where they find the default functionality breaking.

@diasbruno
Copy link
Collaborator Author

diasbruno commented Jun 12, 2021

@TusharShahi

Might I suggest to keep the default behaviour as is,...

Problem is that it will be required anyways, because of the browser issue (if I recall correctly this is an safari/iOS issue - not sure if it was already fixed or not).

I'd prefer this design, even if document.activeElement could be the default, because in order to do the right thing in all browsers the user will have to deal with it anyways...

<Modal returnFocusTo={document.activeElement} ... />

@TusharShahi
Copy link

@TusharShahi

Might I suggest to keep the default behaviour as is,...

Problem is that it will be required anyways, because of the browser issue (if I recall correctly this is an safari/iOS issue - not sure if it was already fixed or not).

I'd prefer this design, even if document.activeElement could be the default, because in order to do the right thing in all browsers the user will have to deal with it anyways...

<Modal returnFocusTo={document.activeElement} ... />

Ah, you are absolutely correct. This will be the ideal prop.
But, is the reason for this only the safari/ios issue? If yes, can we not fix it somehow?

@bvellacott
Copy link

Hi guys,

Sorry I'm late to the discussion, but can you let me know how these objectives relate this pr I created in your oppinion #876 ?

It's purpose is to provide a way to disable the focus- and tabtrap on a modal. The need stems from accessibility concerns in the applications our teams are developing and is also touched in this conversation #799.

@diasbruno
Copy link
Collaborator Author

diasbruno commented Jun 14, 2021

But, is the reason for this only the safari/ios issue? If yes, can we not fix it somehow?

On safari/iOS when you click on a button, the browser don't register the button as document.activeElement. So, we'll have this blind spot if we default to activeElement.

Found it:
react-modal #389

@diasbruno
Copy link
Collaborator Author

@bvellacott In your case, you have a very particular UX problem and it's in conflict with the guidelines for accessibility on modals. It seems that react-modal is not best component that fits your needs. Sorry.

It's hard to get the accessibility right, on normal browsers and the accessible ones, so we are adding unnecessary complexity by allowing to disable those features.

We can accept #876, but you will be stuck on versions 4-.

@bvellacott
Copy link

@diasbruno understood - sounds like we'll have to go with that then.

@alterfo
Copy link

alterfo commented Jun 18, 2021

add animations please, it appears roughly

@diasbruno
Copy link
Collaborator Author

@alterfo Users are free to create the animation with css. Was it difficult to add animations, or, should this package provides some options built-in?

The reason this package doesn't provide any animations is to not add unnecessary files.

@bvellacott
Copy link

@diasbruno is it possible to get the feature into a 4- version?

@bvellacott
Copy link

@diasbruno is it possible to get the feature into a 4- version?

forget this, we have changed our implementation

@ligabloo
Copy link

One problem that might occur if there's no escape hatch for skipping the focus trap is that in some instances a developer might already have a custom focus-trap implementation inside the project, which they want to use for managing modals focus trap as well.

Case in point, that's what I'm dealing with right now - We're using dom-focus-lock to control the focus on some custom elements, and when it is active, it prevents React Modal from capturing the focus.

Of course, that's a very peculiar edge case, and I would completely understand if this package decided to enforce strict defaults to have better accessibility by default on the projects that use it - just wanted to bring this up in case you haven't considered it 😄

@diasbruno
Copy link
Collaborator Author

@ligabloo I'll have a look on this package. Maybe it will be better to add it as a dependency than try to write a specific implementation.

@memark
Copy link
Contributor

memark commented Oct 12, 2022

Has anything been decided on v4? Has the "project" been set up?

@diasbruno
Copy link
Collaborator Author

@memark This is the plan, but unfortunately, I don't have time to work on this and there are only a few brave souls that contribute...

If we find more volunteers, it should not take much time to complete...

@diasbruno
Copy link
Collaborator Author

I can help guiding the project, but it would be nice to have someone to coordinate...

@Cavallando
Copy link

@diasbruno i'd love the opportunity to help out with v4, are you still looking for volunteers?

@yungcalibri
Copy link

This is an offhand suggestion, but I would love it if v4 of react-modal would use the new native <dialog> element by default.

According to caniuse, this feature is at ~92% support across the web. Maybe that's not high enough for a library that may as well, by now, be part of React itself. But it'd be great.

@diasbruno
Copy link
Collaborator Author

@Cavallando That'd be great! I think what need to be done now is to create a roadmap to make this reality.

@yungcalibri I'd say I'm not sure if we can get this in this version. The reason is to keep it backwards compatible.
But if you can bring more information, we can discuss it. Please open a new issue so we can discuss the adoption of this tag.

@diasbruno
Copy link
Collaborator Author

@yungcalibri I'll open a discussion for this.

@diasbruno
Copy link
Collaborator Author

Discussion use the <dialog />.

@diasbruno
Copy link
Collaborator Author

Moving this thread to discussion for v4 of react-modal.

Thank you all for helping.

@ogooluwanick
Copy link

@diasbruno amazing work so far, but because Users can freely create thier own animations with css or even framer. i think that keeps it user friendly

and this keeps the whole thing lightweight...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants