react-modal icon indicating copy to clipboard operation
react-modal copied to clipboard

Discussion for v4 of react-modal

Open diasbruno opened this issue 3 years ago • 13 comments

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

diasbruno avatar Jun 02 '21 01:06 diasbruno

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.

TusharShahi avatar Jun 12 '21 08:06 TusharShahi

@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} ... />

diasbruno avatar Jun 12 '21 15:06 diasbruno

@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?

TusharShahi avatar Jun 14 '21 06:06 TusharShahi

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 https://github.com/reactjs/react-modal/pull/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 https://github.com/reactjs/react-modal/issues/799.

bvellacott avatar Jun 14 '21 10:06 bvellacott

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 avatar Jun 14 '21 13:06 diasbruno

@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-.

diasbruno avatar Jun 14 '21 14:06 diasbruno

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

bvellacott avatar Jun 18 '21 07:06 bvellacott

add animations please, it appears roughly

alterfo avatar Jun 18 '21 11:06 alterfo

@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.

diasbruno avatar Jun 19 '21 00:06 diasbruno

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

bvellacott avatar Aug 05 '21 08:08 bvellacott

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

forget this, we have changed our implementation

bvellacott avatar Aug 17 '21 16:08 bvellacott

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 😄

ligabloo avatar Oct 29 '21 01:10 ligabloo

@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.

diasbruno avatar Oct 29 '21 01:10 diasbruno

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

memark avatar Oct 12 '22 19:10 memark

@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 avatar Oct 13 '22 23:10 diasbruno

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

diasbruno avatar Oct 13 '22 23:10 diasbruno

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

Cavallando avatar Nov 18 '22 21:11 Cavallando

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.

yungcalibri avatar Nov 18 '22 22:11 yungcalibri

@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 avatar Nov 18 '22 22:11 diasbruno

@yungcalibri I'll open a discussion for this.

diasbruno avatar Nov 18 '22 22:11 diasbruno

Discussion use the <dialog />.

diasbruno avatar Nov 18 '22 22:11 diasbruno

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

Thank you all for helping.

diasbruno avatar Nov 18 '22 22:11 diasbruno

@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...

ogooluwanick avatar Feb 07 '23 00:02 ogooluwanick