Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

[Modal] focus-wrapping feature prevents user from using stacked modal #5947

Closed
2 tasks done
davidicus opened this issue Apr 27, 2020 · 15 comments
Closed
2 tasks done
Labels
component: modal package: react carbon-components-react severity: 1 https://ibm.biz/carbon-severity type: bug 🐛

Comments

@davidicus
Copy link
Contributor

What package(s) are you using?

  • carbon-components
  • carbon-components-react

Detailed description

focusTrap previously allowed users to opt out of this feature and effectively manage their own focus. When upgrading to carbon-components v10.10.3, a minor version from previous, this prop was both deprecated and the functionality was removed. This breaks the contract of semantic versioning and, as a result of consuming this change, breaks applications relying on this functionality.

This change also produces a bug where secondary modals (eg. Confirmation or error dialogs) are unable to receive focus once trapped.

Is this issue related to a specific component?

Modal

What did you expect to happen? What happened instead? What would you like to
see changed?

I expect existing API to only be removed on major version changes and deprecation warning to happen before features are removed completely.

What version of the Carbon Design System are you using?

10.10.3

What offering/product do you work on? Any pressing ship or release dates we
should be aware of?

This effects a few products in IoT org.

@joshblack joshblack added impact: medium severity: 1 https://ibm.biz/carbon-severity component: modal package: react carbon-components-react labels Apr 27, 2020
@tw15egan
Copy link
Member

Relevant PR #5260

@asudoh
Copy link
Contributor

asudoh commented Apr 27, 2020

Theoretically no feature is lost, given focus wrapping is enabled regardless of focusTrap property value since forever - Here’s the history; There was a limitation in the original focus wrapping feature (requiring modal not to be the last keyboard focusable element). To address that limitation, a dev brought in a 3rd-party library, but it came with a significant side effects (which broke lots of apps) and we introduced focusTrap={false} prop to revert back to the original code. Some months later we fixed the limitation in the original code itself (#5260), and thus the need for focusTrap prop is gone.

@asudoh
Copy link
Contributor

asudoh commented Apr 27, 2020

Seems that the author meant to post some more details. Good to have a reduced case based on our template wrt:

a bug where secondary modals (eg. Confirmation or error dialogs) are unable to receive focus once trapped.

@stuckless
Copy link

Not sure how you can say 'no feature is lost' when you previous had a focusTrap=false that disabled it, and you now say that it's on always and you can never disable it. Seems like a feature lost and change in the api and and change in the behaviour of the component, and when you look at the code (previously), it was checking focusTrap===false and choosing a different render path.

@stuckless
Copy link

I guess either way, I think you need a way for you to NOT always manage the focus trap, since, you are not allowing for mutiple dialogs to be stacked, and only the bottom dialog is catching the focus trap. Tabbing the top dialog just tabs through the bottom one :(

@asudoh
Copy link
Contributor

asudoh commented Apr 28, 2020

There were two confusing parts, one is the prop name (focusTrap), another is what is does. To clarify both, let me explain how modal used to work before #5260 with different values of focusTrap:

  • focusTrap={false}: Performs focus-wrapping (keeping keyboard focus within modal), but it has a limitation; It requires application to put focus sentinel (a pseudo keyboard-focusable element to ensure our JavaScript can detect a change in focus before the viewport loses focus) as the last element in <body>
  • focusTrap={true}: Uses a 3rd-party library as an attempt to address above limitation

#5260 addresses the limitation without requiring a 3rd-party library, and thus we no longer need focusTrap prop.

Not sure how you can say 'no feature is lost' when you previous had a focusTrap=false that disabled it, and you now say that it's on always and you can never disable it.

Given how the deprecated focusTrap worked, the short answer is that focusTrap={false} does not disable our focus-wrapping feature, even in before-#5260 code.

@asudoh
Copy link
Contributor

asudoh commented Apr 28, 2020

Note on stacked modal: #582

@davidicus
Copy link
Contributor Author

@asudoh Thank you for your replies and further explanation of the details. I apologize on the game of telephone for the bug details as I am trying to relay information second hand. Unfortunately, application functionality has broken from this update and it seems like this is the only change to modals. If this is not the source of the focus being trapped what, in your opinion, could possibly cause issues like the one @stuckless has mentioned above or like the issue #582?

@joshblack @asudoh What would be the guidance for doing things conditionally in modals. For instance, you open a modal to send some data to a server. You hit submit, the application runs some validations that may or may not require the user to perform additional actions (eg. "Doesn't match formatting rules, proceed?). You want the user to be able to confirm his decision or go back to the modal form and change some things.

I imagine you can achieve this by writing some validation logic to only validate on first attempt and swap out the text in the button but this seems like it would be easy to introduce bugs.

What I have seen in applications in our org is a secondary confirmation dialog. Once popped you can submit or cancel. If submitted then both modals are closed and data is sent. if cancelled only the top modal closes and you are back in the same context of the form with the ability to change and hit submit.

Thanks again for your help.

@asudoh
Copy link
Contributor

asudoh commented Apr 29, 2020

Thank you for more explanation @davidicus. As #582 tells, we never made stacked modal work; There may have been "we could made it work by doing ..." moment in application dev, but it's never been our intent.

Wrt the use case of action in modal requiring confirmation, my knee-jerk reaction is the action should live in the main page, but I think it's UX call: CC @carbon-design-system/design

@aagonzales
Copy link
Member

aagonzales commented Apr 30, 2020

What would be the guidance for doing things conditionally in modals. For instance, you open a modal to send some data to a server. You hit submit, the application runs some validations that may or may not require the user to perform additional actions (eg. "Doesn't match formatting rules, proceed?). You want the user to be able to confirm his decision or go back to the modal form and change some things.

If your task required a modal for confirmation then that first modal should be its own page and not a modal itself. One modal should never trigger another modal. It is a bad ux pattern.

Sometimes these scenarios occur because of legacy design and I'm not sure what the solution should be for that. But if this is a new design or a redesign then the scenario should be re-designed without nesting modals.

Modal task should be clear and simple enough that you don't need an extra confirmation to complete the task.

We recommend that any validation in a modal be done inline for this reason. See docs
https://www.carbondesignsystem.com/components/modal/usage#validation
https://www.carbondesignsystem.com/components/modal/usage#loading
https://www.carbondesignsystem.com/patterns/dialog-pattern#when-not-to-use

@lukefirth
Copy link

lukefirth commented May 1, 2020

Understood completely, and I think you hit on our challenge precisely there @aagonzales with

Sometimes these scenarios occur because of legacy design and I'm not sure what the solution should be for that.

This should probably be discussed in a different, broader forum so feel free to tell me to move it! 😄

I know we've all heard it all before - there's a stack of massive apps that are on the journey to alignment, but for better or worse (we all know which) the strategy defined by the leadership of those applications to focus on simple 'skin deep' alignment first, no major IA reworks. This means the teams need bandaids, and to get on an evolutionary path.

Which... leaves us in a situation where unless we convince high-level leadership that it's not a plausible option to just do skin deep re-works, we either say:

  • 'We are all or nothing, if you don't accept all the required UX/IA changes, you get no help from us, we only support the best practice". Which will mean each instance of the problem in each application is left to someone to hack, in a million different ways each time (and if the code won't let them, they'll make their own version of it).

Or

  • Say 'this is a recognised situation many find themselves in, and these are the steps you should take to extend what we do offer, which will help get you out of it and towards the best practice'. This means that teams can start solving the problem (like modal on modal) consistently and start using the real components etc. They won't be at the best practice yet but at least they are all solving the problem consistently and are aware its not best practice.

I guess this is a challenge between if the answer to what 'can' be done with components and what 'should' be done with them should be the same thing. Is it better that people who have to work in the 'wrong way' (bad legacy UX) at least use the right stuff (Carbon components) to do so?


Now, I'm totally not expecting Carbon to in any way need to be the ones who bridge the gap from legacy to modern best practice. If anything I see that as one of the values PALs can deliver - acting as a connecting bridge for any of their legacy teams on how to reach the gold standard. As teams get closer and closer to the golden standard that Carbon (plus their industry specifics), the PALs should naturally shrink.

Long story short - What you see in issues like this, is a PAL team desperately searching for ways of bridging the gap with as little custom work in our PAL code as possible by looking for handholds on the Carbon components that we can latch onto in order to extend, instead of needing building our own nearly identical copies that do have the ability to be used in more scenarios than clean slate / greenfield design alone.

@asudoh asudoh changed the title [Modal] focusTrap prop was removed and deprecated in same minor version. [Modal] focus-wrapping feature prevents user from using stacked modal May 1, 2020
@stuckless
Copy link

@aagonzales While I think Luke covered this quite well, the other take away from this is that Carbon is only going to be successful if teams using Carbon are successful. We've all seen the rise and fall of multiple component/design architectures during our time. The ones that succeed are the ones that developers can use and depend on.

Now sadly as a "customer" of Carbon React components, my most common answer I get when something changes or doesn't work, is, "It's not our intention..." or "You need to change how you do things". And that can work sometimes, but, as Luke mentioned many of us don't have the luxury of just changing on a whim.

I've mentioned in the past that every time Carbon does something that forces us to work around it, then, we use a little less of Carbon React each time. That isn't good for us, and it's not good for Carbon. At some point, I'll have no dependencies on the Carbon components, and just on the Carbon design, and Carbon components will have fewer complaints from people like myself, because they have fewer customers. No one wants this.

If moving from carbon 9 to carbon 10 requires a huge effort, then you are going to lose developers along the way. If moving from carbon 10 to carbon 11 is huge effort, you are going to lose developers. As more friction happens in the adoption of carbon or the use of carbon, we lose developers. The trick is balance ideology with adoption. I don't have a good solution for that, but, I know the answer isn't telling teams to rewrite their applications, or, to tell teams that they need to rewrite and bypass huge parts of the Carbon ecosystem.

I'm not even asking Carbon React team to support stacked modals (which is most commonly a message modal over the dialog). I'm just asking that they not prevent me from implementing it. At this point I'm left with trying to hack modals to allow this, or just write my own modal layer and not use Carbon's. I'm leaning to the latter because in my experience any hacks we do are generally erased during a another upgrade cycle, and, we re left solving the hack again.

Message modals over a dialog are quite common in "real" applications. I could be a dialog, and the application framework detects a loss in network, and it shows a modal at the system level. I could be in a dialog and it the close button with unsaved changes, and decide that maybe I ask the user if they really want to lose those changes? I could be in a dialog allowing the user to enter some information and that could trigger a lookup model from when they can select something. Maybe none of these are ideal UX scenarios, but they are are real.

@aagonzales
Copy link
Member

I guess this is a challenge between if the answer to what 'can' be done with components and what 'should' be done with them should be the same thing.

I think this is the ultimate question. And a little bit goes to the heart of what the purpose of Carbon and a standard design system is.

Do we adjust/lower our design ux standards in-order to accommodate legacy application? Does allowing for legacy application ux practices facilitate greater adoption but increase inconsistencies and doublespeak? ie we say "never nest modals" but then allow nesting modals in code? Which then is the source of truth, the code or the guidelines? If we incorporate/approve legacy patterns at the top level what then is the motivation for a team to ever update and improve the ux?

This is a much bigger discussion, like Luke said, and not just about allowing a nested modal (which I still whole heartedly believe is a bad ux pattern). I am not un-sympathetic to your scenario, which is why I pointed it out in the first place. I'm just not sure what the official stance should be when it comes to these things. As of right now the Carbon common components operate as the best practice ideal ux implementation and any deviation from the common implementation we are encouraging those changes be done at the PAL/product level. I feel like your team having to make one or two custom components to aid in legacy adoption is fine. Especially if the goal is to eventually be Carbon compliant once you are allowed to do major UX changes. If you're having to make custom components for every component in carbon common then that is also a much larger discussion.

@stuckless
Copy link

Thanks for the comments. You can close the issue.

@tw15egan tw15egan closed this as completed May 5, 2020
@joshblack
Copy link
Contributor

Just to chime in with the comments above, definitely agreed that we want to make the barrier to using components from Carbon as minimal as possible. To this goal, we try to introduce components and aspects of the IBM Design Language in the smallest slices that we can while still providing consolidated entry points for convenience.

For the Design Language, this manifests itself with individual packages for color, type, layout, etc. These all get rolled up into elements and also get initialized in carbon-components. This helps teams who can only use aspects of the DL, but not all of it, to still stay connected to the ecosystem.

Similarly, for components, we try to slice this with styles and behavior. We definitely recognize that behavior may not align 1:1 with what we can implement or support, but are happy to provide the building blocks which we use to provide the behavior for these components.

An example of this could be DataTable where we provide functionality for sorting, filtering, selection, etc. out-of-the-box. However, teams can choose not to use DataTable directly and can use the associated presentational components for constructing their own DataTable component that works for their use-case.

In both cases, we try and present the smallest units we can offer that we use ourselves to build up components. Sometimes this ideal falls apart though and we try and rectify it. The hope is that folks can interop at any particular meaningful stage and I hope we can find a similar solution for Modal in this case that doesn't block any particular use-case from being satisfied 👍

Based on the above, it seems like providing appropriate presentational components that don't manage focus explicitly would be sufficient?

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
component: modal package: react carbon-components-react severity: 1 https://ibm.biz/carbon-severity type: bug 🐛
Projects
None yet
Development

No branches or pull requests

8 participants