Tearsheets: focus with selectorPrimaryFocus and nested tearsheets is still broken

wkeese commented 3 months ago

Package

Carbon for IBM Products

Description

Despite #3988 and #5382, focus with nested tearsheets / selectorPrimaryFocus is broken.

TearsheetShell.tsx has this very strange code which seems obviously wrong:

useEffect(() => {
  if (open && position !== depth) {
    setTimeout(() => {
      firstElement?.focus();
    }, 0);
  }
}, [position, depth, firstElement, open]);

The first problem is that the above code is not accounting for selectorPrimaryFocus.

The second problem is that the position !== depth condition means that, in the case of nested tearsheets, it's operating on the wrong tearsheet, i.e. the original tearsheet rather than the nested one that's currently active.

The third problem is: Does that code have any purpose? There's already a bunch of other code in TearsheetShell.tsx dealing with focus, in addition to the code in ComposedModal.tsx.

ISTM that ComposedModal.tsx already handles focus when a tearsheet is opened, even for stacked tearsheets. So the only thing that TearsheetShell.tsx needs to do is to focus the original tearsheet when the nested tearsheet is closed.

Component(s) impacted

Tearsheet

Browser

No response

@carbon/ibm-products (previously @carbon/ibm-cloud-cognitive) version

2.44.0

Suggested Severity

Severity 2 = Aspects of design is broken, and impedes users in a significant way, but there is a way to complete their tasks. Affects major functionality, has a workaround.

Product/offering

IKC

CodeSandbox or Stackblitz example

https://codesandbox.io/p/devbox/tearsheet-example-wrq59h

Steps to reproduce the issue (if applicable)

Open https://codesandbox.io/p/devbox/tearsheet-example-wrq59h.
Click "Open tearsheet".
Click "Open nested tearsheet".

Step 3 should focus the <a> in the nested tearsheet, but instead it focuses the close icon in the first tearsheet.

Release date (if applicable)

No response

Code of Conduct

[X] I agree to follow this project's Code of Conduct
[X] I checked the current issues for duplicate issues

matthewgallo commented 3 months ago

Thanks for the detailed issue here! I have a PR open that contains similar changes to https://github.com/carbon-design-system/ibm-products/pull/5264 that fixed the behavior of selectorPrimaryFocus, but for stacked tearsheets.

I do agree with you that TearsheetShell could use some refactoring in the overall approach to handling focus states. In the meantime, I was able to reproduce the behavior you described and confirmed that it was resolved with the changes from https://github.com/carbon-design-system/ibm-products/pull/5631.

wkeese commented 3 months ago

Hmm, that's hard for me to believe because in my tests, that code was running on the wrong tearsheet (i.e. the bottom tearsheet not the top one).

Are you sure that you tested that the right tearsheet was getting focused?

Also, did you try just removing that code block completely?

carbon-design-system / ibm-products