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

Prevent Scrolling back when focus on close #38079

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ChellyAhmed
Copy link
Contributor

@ChellyAhmed ChellyAhmed commented Feb 17, 2023

Description

This Pull Request is a suggestion to prevent scrolling back to the trigger element on closing off-canvas, all while keeping the behavior of focusing the element unchanged for accessibility. This modification is simply done by adding the preventScroll: true object to the this.focus() method.

Motivation & Context

This PR comes as a better solution for the issue: #38070 . Another PR #38076 is suggested but it solves the issue by completely disabling the focus of the trigger element when the user scrolls. Yet, it is important to focus the trigger element for accessibility.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

I created a codepen link to preview the changes:
https://codepen.io/ChellyAhmed/pen/VwGLdbR

Related issues

Closes #38070

@GeoSot
Copy link
Member

GeoSot commented Feb 28, 2023

I am not sure how can we test this, but seems a fair choice

@ChellyAhmed
Copy link
Contributor Author

Thank you @GeoSot for your comment and for your review. I did not create an automated test for this (should I?) But I manually tried it, and it gave exactly the expected behavior, even when I only used the keyboard to navigate.

@patrickhlauke
Copy link
Member

as said before, I'd make this an option/parameter that an author can explicitly opt into for their off-canvas, not the default behaviour

@GeoSot
Copy link
Member

GeoSot commented Mar 10, 2023

As much as I agree, config scope is not available at that point (it could be as, a hack, but is not a wise choice) 😞

The only alternative I can think of, is to check a global option. like bootstrap.Offcanvas.Default.focusOnClose.

@ChellyAhmed if we proceed on this, a valid test could be to check that the doc scroll is not changed after closing the offcanvas

@patrickhlauke
Copy link
Member

any movement on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs review
Development

Successfully merging this pull request may close these issues.

offcanvas scrolls back to trigger element on close
5 participants