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.

Sign up for GitHub

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

Jump to bottom

Allow data-toggle="dropdown" and form click events to bubble #32942

Closed
wants to merge 23 commits into from
Closed

Allow data-toggle="dropdown" and form click events to bubble #32942

wants to merge 23 commits into from

Conversation

caseyjhol
Copy link
Contributor

@caseyjhol caseyjhol commented Jan 30, 2021 •
edited

@caseyjhol caseyjhol requested a review from a team as a code owner January 30, 2021 19:46
@caseyjhol
Copy link
Contributor Author

Merge conflicts have been resolved.

@rohit2sharma95
Copy link
Collaborator

You can not open the dropdowns now:
https://deploy-preview-32942--twbs-bootstrap.netlify.app/docs/5.0/components/dropdowns/

@caseyjhol
Copy link
Contributor Author

caseyjhol commented Jan 31, 2021 •
edited

  • This also needs a test for event delegation. Working on it.

@thijndehaas
Copy link

When will this be released? Waiting for bootstrap-select that requires this fix.

@XhmikosR XhmikosR added this to Inbox in v5.0.0-beta3 via automation Feb 15, 2021
rohit2sharma95
rohit2sharma95 reviewed Feb 21, 2021
View reviewed changes
Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 left a comment

Choose a reason for hiding this comment

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

Its purpose is exactly the same as #32943 and it seems cleaner.

/CC @XhmikosR

js/tests/unit/dropdown.spec.js Outdated Show resolved Hide resolved
@XhmikosR
Copy link
Member

I'm OK with either way as long as the added tests here cover the tests added in #32943.

@rohit2sharma95 feel free to close the other PR if you think this is better.

@rohit2sharma95
Copy link
Collaborator

rohit2sharma95 commented Feb 21, 2021 •
edited

@caseyjhol Can you please add a test case to check that the dropdown should be shown when clicking on a child element of data-bs-toggle="dropdown" 🙂

Also, I believe that #33044 can be fixed in the same PR

@XhmikosR XhmikosR mentioned this pull request Feb 21, 2021
Closed
caseyjhol
caseyjhol commented Feb 21, 2021
View reviewed changes
js/src/dropdown.js Outdated
dropdownMenu.contains(event.target)) {
continue
if (event) {
if (context._element === SelectorEngine.parents(event.target, SELECTOR_DATA_TOGGLE)[0]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rohit2sharma95 I also need to compare context._element with event.target to ensure clicks on the button itself toggle the menu, but that increases the complexity to 21, causing eslint to fail. Any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

It's a warning, although it'd be nice to not have it. We should try to move some logic outside of clearMenus() at some point

Copy link
Contributor Author

@caseyjhol caseyjhol Feb 22, 2021 •
edited

Choose a reason for hiding this comment

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

I switched to event.composedPath().includes(context._element) instead. composedPath seems to be standard in modern browsers (https://developer.mozilla.org/en-US/docs/Web/API/Event/composedPath), but let me know if you'd prefer I switch back.

Copy link
Member

Choose a reason for hiding this comment

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

Not against this if it's supported but perhaps we should make that change in a separate PR in case other compononents can use it? https://caniuse.com/?search=composedPath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The alternative would be something like:

if (context._element === event.target || context._element === SelectorEngine.parents(event.target, SELECTOR_DATA_TOGGLE)[0] || dropdownForm === event.target || dropdownForm === SelectorEngine.parents(event.target, SELECTOR_FORM_CHILD)[0]) {
    ...
}

Could we maybe merge this as is and then open another issue addressing using composedPath in other places where it would make sense?

@caseyjhol
Copy link
Contributor Author

@rohit2sharma95 @XhmikosR Any more thoughts/comments?

@rohit2sharma95 rohit2sharma95 moved this from Inbox to Review in v5.0.0-beta3 Mar 16, 2021
@XhmikosR
Copy link
Member

@caseyjhol can you please make a proper rebase?

image

@RyanBerliner could you have a quick look please?

XhmikosR
XhmikosR reviewed Mar 16, 2021
View reviewed changes
js/src/dropdown.js Outdated Show resolved Hide resolved
@rohit2sharma95 rohit2sharma95 mentioned this pull request Mar 17, 2021
Merged
@rohit2sharma95
Copy link
Collaborator

Tests for form inside menu can be left less strict, those will be updated in #33389.

rohit2sharma95
rohit2sharma95 requested changes Mar 18, 2021
View reviewed changes
Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 left a comment

Choose a reason for hiding this comment

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

@caseyjhol Can you please address few last comments? Only you can edit this PR 🙂

js/src/dropdown.js Show resolved Hide resolved
js/tests/unit/dropdown.spec.js Outdated Show resolved Hide resolved
js/tests/unit/dropdown.spec.js Outdated
Comment on lines 1879 to 1883
btnDropdown.addEventListener('shown.bs.dropdown', () => setTimeout(() => {
expect(btnDropdown.classList.contains('show')).toEqual(true)
expect(btnDropdown.getAttribute('aria-expanded')).toEqual('true')
done()
}))
Copy link
Collaborator

Choose a reason for hiding this comment

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

setTimeout is not required here IMO

Copy link
Contributor Author

@caseyjhol caseyjhol Mar 18, 2021 •
edited

Choose a reason for hiding this comment

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

The test wasn't working properly for me (locally) without the setTimeout (it was passing when it shouldn't).

Do you want me to remove all code related to checking for the form (with the idea being users should set clickableMenu instead of Bootstrap detecting the presence of a form)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you want me to remove all code related to checking for the form

No, that should be handled in the next PR.

@caseyjhol caseyjhol requested a review from a team as a code owner March 19, 2021 00:01
@caseyjhol
Copy link
Contributor Author

Sorry - not sure what happened when I was rebasing...getting this sorted.

rohit2sharma95
rohit2sharma95 approved these changes Mar 22, 2021
View reviewed changes
Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 left a comment

Choose a reason for hiding this comment

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

@caseyjhol Please remove all newly added setTimeouts from tests. They are not needed in real 😄

LGTM 👍 (There are new updates in the next PR though)
Expecting one more approval from @twbs/js-review

v5.0.0-beta3 automation moved this from Review to Approved Mar 22, 2021
caseyjhol
caseyjhol commented Mar 22, 2021
View reviewed changes
js/src/dropdown.js
Comment on lines +411 to +413
if ([context._element].some(element => event.composedPath().includes(element))) {
continue
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rohit2sharma95 I'm concerned that removing the setTimeouts might affect the reliability of the tests. If I remove this section of code (preventing the menu from opening), both the "data-api -> should show and hide a dropdown" and "should open the dropdown when clicking the child element inside data-bs-toggle="dropdown"" tests pass. With the setTimeouts, they fail.

In any case, I'm eager to get this merged in, so I've gone ahead and removed the setTimeouts, but I urge you to check into it. If you still feel they're not necessary, if you get a chance, I'd love to know the reasoning. Thanks for all your help getting this feature implemented!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, for smooth release please restore setTimeouts and rebase your branch 🙂

@XhmikosR
Copy link
Member

@caseyjhol you are rebasing against the wrong remote plus I don't have rights to edit your branch because you are using an org fork I presume. Anyway, I pushed the branch upstream temporarily https://github.com/twbs/bootstrap/compare/bubble-click-events to see if BrowserStack passes too.

As for the setTimeouts, not sure what to do since both seem to work since tests pass? We can keep the setTimeouts for now and see if they are really needed later, because I'd like to land this patch in beta3 which we plan to release later today :)

@XhmikosR XhmikosR mentioned this pull request Mar 23, 2021
Merged
@XhmikosR XhmikosR removed this from Approved in v5.0.0-beta3 Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XhmikosR XhmikosR XhmikosR left review comments

@RyanBerliner RyanBerliner RyanBerliner left review comments

@rohit2sharma95 rohit2sharma95 rohit2sharma95 approved these changes

@GeoSot GeoSot GeoSot approved these changes

Assignees
No one assigned
Labels
backport-to-v4 js v4 v5
Projects
None yet
Milestone
No milestone
6 participants
@caseyjhol @rohit2sharma95 @thijndehaas @XhmikosR @RyanBerliner @GeoSot