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

collapse: prevent url change if A nested tag is clicked #32438

Merged
merged 6 commits into from
Dec 21, 2020

Conversation

joke2k
Copy link
Contributor

@joke2k joke2k commented Dec 11, 2020

Removing jQuery the target of the click event is not the A tag if the user clicks on a nested tag,
ignoring the preventDefault and changing the hash in the url.

What this PR does:

  • Add event.delegateTarget into the check for preventDefault
  • Test this behaviour

Details:

this code:

<a role="button" data-bs-toggle="collapse" class="collapsed" href="#collapse">
  <span id="nested">Clickme</span>
</a>
<div id="collapse" class="collapse"></div>

bubbles ups the default behaviour and changes the URL appending #collapse to it because the event.target is not an A tag, this let fail the check for event.preventDefault.

See on bs5:

EventHandler.on(document, EVENT_CLICK_DATA_API, SELECTOR_DATA_TOGGLE, function (event) {
// preventDefault only for <a> elements (which change the URL) not inside the collapsible element
if (event.target.tagName === 'A') {
event.preventDefault()
}

On bs4 jQuery did the work:

$(document).on(EVENT_CLICK_DATA_API, SELECTOR_DATA_TOGGLE, function (event) {
// preventDefault only for <a> elements (which change the URL) not inside the collapsible element
if (event.currentTarget.tagName === 'A') {
event.preventDefault()
}

it happened with this commit 69e4d4f#diff-3cc7f0d8580d17c90274789ffc45552c56fed69ac7404d65e88ccccb81ffff31R378

@joke2k joke2k requested a review from a team as a code owner December 11, 2020 20:39
@XhmikosR
Copy link
Member

We need tests for this, can you add a test please?

@joke2k
Copy link
Contributor Author

joke2k commented Dec 11, 2020

emm... tests were already in the PR 😅

@XhmikosR
Copy link
Member

Oops, sorry I missed them!

@@ -374,6 +374,29 @@ describe('Collapse', () => {
})

describe('data-api', () => {
it('should prevent url change if click on nested elements', done => {
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this describe is the right place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is a bug of the data-api as you can see from the part of collapse.js file:


/**
 * ------------------------------------------------------------------------
 * Data Api implementation
 * ------------------------------------------------------------------------
 */

EventHandler.on(document, EVENT_CLICK_DATA_API, SELECTOR_DATA_TOGGLE, function (event) {
  // preventDefault only for <a> elements (which change the URL) not inside the collapsible element
  if (event.target.tagName === 'A' || (event.delegateTarget && event.delegateTarget.tagName === 'A')) {
    event.preventDefault()
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is It right?

@XhmikosR XhmikosR changed the title Collapse - prevent the url change if A nested tags is clicked collapse: prevent url change if A nested tag is clicked Dec 21, 2020
@XhmikosR XhmikosR merged commit 59cd716 into twbs:main Dec 21, 2020
sijusamson pushed a commit to sijusamson/bootstrap that referenced this pull request Jan 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants