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

fix focusable custom-element tabbing issue #1072

Merged

Conversation

simonxabris
Copy link
Contributor

@simonxabris simonxabris commented Oct 5, 2023

fixes #1068

Added a function that recursively checks for the activeElement and if the activeElement is a custom-element, it pierces the shadow-dom and checks for activeElement in it to determine which element is truly in focus.

I was not sure how to properly test this as imo it's very much an edge case, so for now I just updated the current in-open-shadow-dom example to have the structure I described in the issue and manually checked if my changes fix that. I have not written any extra tests for now, but if you think that new tests are warranted, I'm happy to add them

aside: I also added an all-contributos script to package.json, as according to to cotribution docs it already should've been there.

PR Checklist

Please leave this checklist in your PR.

  • Issue being fixed is referenced.
  • Source changes maintain stated browser compatibility.
  • Web APIs introduced have deep browser coverage, including Safari (often very late to adopt new APIs).
  • Includes updated docs demo bundle if source/docs code was changed (run npm run demo-bundle in your branch and include the /docs/demo-bundle.js file that gets generated in your PR).
  • Unit test coverage added/updated.
  • E2E (i.e. demos) test coverage added/updated.
    • ⚠️ Non-covered demos (look for // TEST MANUALLY comments here) that can't be fully tested in Cypress have been manually verified.
  • Typings added/updated.
  • Changes do not break SSR:
    • Careful to test typeof document/window !== 'undefined' before using it in code that gets executed on load.
  • README updated (API changes, instructions, etc.).
  • Changes to dependencies explained.
  • Changeset added (run npm run changeset locally to add one, and follow the prompts).
    • EXCEPTION: A Changeset is not required if the change does not affect any of the source files that produce the package bundle. For example, demo changes, tooling changes, test updates, or a new dev-only dependency to run tests more efficiently should not have a Changeset since it will not affect package consumers.

@changeset-bot
Copy link

changeset-bot bot commented Oct 5, 2023

🦋 Changeset detected

Latest commit: 8511822

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
focus-trap Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@stefcameron stefcameron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR! I appreciate you tried to follow all the existing repo patterns, and it's good to add the all-contributors package script.

For testing, I don't think we can do more than what you've done because of Cypress' lack of support for Shadow DOM.

I ran the demo locally, and the usual active focus trap styling isn't working after your changes (the trap's container should get a pinkish background like all the other demos when the trap is active). Also, it would be nice if the tabbable <custom-content> element you added would get the usual teal border around it when it has focus.

.changeset/rich-fans-unite.md Outdated Show resolved Hide resolved
cypress/downloads/downloads.html Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
simonxabris and others added 2 commits October 10, 2023 09:27
Co-authored-by: Stefan Cameron <stefan@stefcameron.com>
@simonxabris
Copy link
Contributor Author

simonxabris commented Oct 10, 2023

Fixed everything you pointed out in the review, this is how the focus states look now:

Focus demo
Screen.Recording.2023-10-10.at.10.02.09.mov

I added a focus ring around the whole element, as it's now focusable because of the tabindex="0" is added to the custom element

@stefcameron
Copy link
Member

Fixed everything you pointed out in the review, this is how the focus states look now:

Focus demo
I added a focus ring around the whole element, as it's now focusable because of the tabindex="0" is added to the custom element

Nice, thank you! The demo looks great now in your video. I'll check out your changes ASAP!

Copy link
Member

@stefcameron stefcameron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you for this PR. Everything looks good to me. 😄

@stefcameron stefcameron merged commit 680f6e8 into focus-trap:master Oct 12, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug with selecting jumping to first tabbable from last if web component is involved.
2 participants