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

offcanvas scrolls back to trigger element on close #38070

Open
3 tasks done
lekoala opened this issue Feb 16, 2023 · 20 comments · May be fixed by #38076, #38079 or #40553
Open
3 tasks done

offcanvas scrolls back to trigger element on close #38070

lekoala opened this issue Feb 16, 2023 · 20 comments · May be fixed by #38076, #38079 or #40553

Comments

@lekoala
Copy link

lekoala commented Feb 16, 2023

Prerequisites

Describe the issue

When closing the offcanvas menu, the trigger element will get the focus due to

EventHandler.one(target, EVENT_HIDDEN, () => {
// focus on trigger when it is closed
if (isVisible(this)) {
this.focus()
}
})

this will make the whole page scroll back. If you scrolled (data-bs-scroll=true) or used an anchor, you will "jump back" to top of page.

maybe it may make sense in some situations, but at least there should be an option to disable that behaviour.

Reduced test cases

https://codepen.io/lekoalabe/pen/VwGYgdw

What operating system(s) are you seeing the problem on?

Windows

What browser(s) are you seeing the problem on?

Chrome

What version of Bootstrap are you using?

5.3.0-alpha1

@lekoala
Copy link
Author

lekoala commented Feb 16, 2023

as a way around, you can just use the component directly without the data api, but that's still strange

@ChellyAhmed
Copy link
Contributor

I think this should be the expected behavior if you use an anchor that takes you to a different page.
However, if data-bs-scroll is set to true , why would we want to focus the trigger element in the first place since we are scrolling?
Maybe we can add a condition for data-bs-scroll to be false on the this.focus() part

@lekoala
Copy link
Author

lekoala commented Feb 17, 2023

@ChellyAhmed for one pagers, this wouldn't be the case, so offcanvas cannot assume that it should always focus back.
also, it's inconsistent: why would the data api be doing that when using the toggle/hide feature wouldn't?

I agree with your proposal, I think a safe default would be: if you allow scrolling, then disable this.focus

@ChellyAhmed
Copy link
Contributor

@julien-deramond Do you think it will be a good idea to create a pull request implementing the change mentioned?

@julien-deramond
Copy link
Member

Just by reading quickly the discussion, difficult to say for sure if it is the right way to handle this issue. Maybe @GeoSot has some opinion about it already.

But IMO if you have an idea to improve the behavior, sure you can create a PR. I don't know if it is the appropriate change or not but having an example to play with in the PR would be helpful. I don't guarantee to be able to review it quickly but I'll try to do my best :) Thanks all for the improvement ideas.

@lekoala
Copy link
Author

lekoala commented Feb 17, 2023

@julien-deramond you can have a look at the codepen. implementing the proposal would prevent jumping back to the top of the page when closing the menu if you clicked on section3 for example

@GeoSot
Copy link
Member

GeoSot commented Feb 17, 2023

this will make the whole page scroll back. If you scrolled (data-bs-scroll=true) or used an anchor, you will "jump back" to top of page.

This may be a fine idea, but have in mind at that point, config is out of our scope, as we are outside Offcanvas's code

As Julien said, a Pr with the proper example, would help

@lekoala lekoala linked a pull request Feb 17, 2023 that will close this issue
6 tasks
@lekoala
Copy link
Author

lekoala commented Feb 17, 2023

made a pr, and created another codepen with fixed sourcecode

https://codepen.io/lekoalabe/pen/RwYPQdW

vs not fixed

https://codepen.io/lekoalabe/pen/VwGYgdw

@ffoodd
Copy link
Member

ffoodd commented Feb 17, 2023

I think @patrickhlauke may help there: I think focusing the trigger back is mandatory for keyboard users.

@patrickhlauke
Copy link
Member

patrickhlauke commented Feb 17, 2023

I think focusing the trigger back is mandatory for keyboard users

yes. unless you want to offer a way for authors to suppress this behavior (for regular modals as well) if they for whatever reason want to do their own post-close focus handling, and we stress this in the documentation

@lekoala
Copy link
Author

lekoala commented Feb 17, 2023

ok so not restoring focus is not an option, but then the question is more about what needs to be focused. if the anchor has changed, it makes no sense to focus back to the original button.
maybe we can simply set the focus to the new anchor then?

@ChellyAhmed ChellyAhmed linked a pull request Feb 17, 2023 that will close this issue
10 tasks
@ChellyAhmed
Copy link
Contributor

I am wondering, why don't we just restore focus simply without scrolling? We can do that simply by adding preventScroll: true to the focus method. I opened a pull request above to demonstrate that.

@lekoala
Copy link
Author

lekoala commented Feb 17, 2023

@ChellyAhmed your solution is better than mine
maybe even tweak it a little and set it to

  this.focus({
    preventScroll: this._config.scroll
  })

showed here https://codepen.io/lekoalabe/pen/OJoVwyp

@ffoodd
Copy link
Member

ffoodd commented Feb 17, 2023

Keyboard users wouldn't know focus was restored without scroll. There's a reason for this standard behaviour.

You're talking about anchor change, but opening offcanvas shouldn't change anchor. Those are two separate actions.

@patrickhlauke
Copy link
Member

as said, I'd offer the option maybe (both for "don't actually focus anything, the author will do their own focusing" and for "yeah, set focus back, but don't visibly scroll" - though that would be quite confusing, and a keyboard user would then need to Tab to the next focusable element for the viewport to snap back to where they actually are), but very strongly stress the implications of these in the docs

@ChellyAhmed
Copy link
Contributor

@lekoala Thanks for your comment. But I am not entirely sure we should add the : this._config.scroll. Because if the user did not scroll, we wouldn't care anymore about whether we should scroll back or not, because the user did not scroll in the first place.
Also, following up on @ffoodd, when a keyboard user opens the offcanvas menu, it would be through the trigger element. Hence, IMO it would make sense that this trigger element is still focused regardless of the scrolling position.

@ruiarii
Copy link

ruiarii commented May 13, 2023

assuming we disable the automatic behaviour, IMO, to allow the user to choose to scroll back to the initial position, I think it depends on the placement of the trigger element, if we decide to place it initially on a sticky navbar, it will follow the navigation through the page, and when the user wants to go back to the initial starting point, he would only have to use the trigger element once again since it follows him.

On another hand, we can just make the trigger element follow you while navigating, for example, we can make it stick to the top once you use it to scroll down.

opinion?

@Maria-qtr2
Copy link

i am having the same issue
i need help

@Extazykeep
Copy link

Extazykeep commented Jun 10, 2024

As a temporary solution u can ommit keyboard users, and use link without href instead of button for burger, this works as ur expect it to work, our use ur burger button inside fixed-top menu.

 <a class="navbar-toggler" data-bs-toggle="offcanvas" data-bs-target="#offcanvasNavbar"
                aria-controls="offcanvasNavbar" aria-label="burger">
                <span class="navbar-toggler-icon"></span>
            </a>

^^ this works fine

i am having the same issue i need help

Prerequisites

Describe the issue

When closing the offcanvas menu, the trigger element will get the focus due to

EventHandler.one(target, EVENT_HIDDEN, () => {
// focus on trigger when it is closed
if (isVisible(this)) {
this.focus()
}
})

this will make the whole page scroll back. If you scrolled (data-bs-scroll=true) or used an anchor, you will "jump back" to top of page.

maybe it may make sense in some situations, but at least there should be an option to disable that behaviour.

Reduced test cases

https://codepen.io/lekoalabe/pen/VwGYgdw

What operating system(s) are you seeing the problem on?

Windows

What browser(s) are you seeing the problem on?

Chrome

What version of Bootstrap are you using?

5.3.0-alpha1

@Xcreen
Copy link

Xcreen commented Jun 12, 2024

I think we should add an extra option like scrollBackAfterClose (default = true).
Then we have the ability to change the behavior like we need.

@Xcreen Xcreen linked a pull request Jun 12, 2024 that will close this issue
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment