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

chore: forward ref to Button #4150

Merged
merged 1 commit into from
Oct 24, 2023
Merged

chore: forward ref to Button #4150

merged 1 commit into from
Oct 24, 2023

Conversation

DimitarNestorov
Copy link
Contributor

Motivation

Closes #3988 and #3992

Related issue

#3988

Test plan

CI passes

@DimitarNestorov DimitarNestorov marked this pull request as ready for review October 23, 2023 18:08
@github-actions
Copy link

The mobile version of example app from this branch is ready! You can see it here.

@callstack-bot
Copy link

Hey @DimitarNestorov, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

@lukewalczak lukewalczak merged commit 1b89fc6 into main Oct 24, 2023
3 checks passed
@DimitarNestorov DimitarNestorov deleted the chore/forward-ref-to-button branch October 24, 2023 07:31
@RichardLindhout
Copy link
Contributor

Why are we given the ref to the surface and not to the pressable?

@DimitarNestorov
Copy link
Contributor Author

DimitarNestorov commented Feb 23, 2024

@RichardLindhout Because that's what is the root of the button

@SleeplessByte
Copy link
Contributor

@DimitarNestorov can you tell us how to get the pressable so we can focus the button programmatically among other things?

@DimitarNestorov
Copy link
Contributor Author

@SleeplessByte opened a PR which adds touchableRef to Button #4322

@SleeplessByte
Copy link
Contributor

@SleeplessByte opened a PR which adds touchableRef to Button #4322

Great! FWIW this is also missing on things like dialog ☺️

@DimitarNestorov
Copy link
Contributor Author

this is also missing on things like dialog ☺️

Could you be more specific?

@SleeplessByte
Copy link
Contributor

SleeplessByte commented Feb 25, 2024

Definitely. Related to #3912.

The modal (element with aria-modal) should receive focus when it's shown, which means that either it, its title, its paragraph, or its first focusable element should be focussed, depending on how much content there is in the modal (aka, you don't want to focus the button if it's in a scroll container, at the bottom, and there is a scroll bar).

If the modal ref is exposed, and given a tabindex=-1 on web, we can actually make the modal accessible, as well as install a focus trap on web (for native, react-native-community/discussions-and-proposals#640 is still open)

Right now we can only achieve this by:

// Cannot use useCallback, because during the callback the dialog is not
// yet mounted, so there is nothing to focus.
useEffect(() => {
  if (Platform.OS !== "web" || !visible) {
    return;
  }

  // TODO: solve for other platforms
  // Wait for dialog to be mounted
  setTimeout(() => {
    const modal = document.querySelector<HTMLDivElement>(
      '[data-testid="dialog"] > [data-testid="dialog-wrapper"] > div',
    );

    if (!modal) {
      return;
    }

    modal.setAttribute("tabIndex", "-1");
    modal.focus();
  }, 1);
}, [visible]);

With this PR, it becomes easier as in a lot of cases we can focus the actual cancel button in Dialog.Actions, but installing the focustrap is hard without a reference to the node.

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