Allow data-toggle="dropdown" and form click events to bubble #32942
Conversation
caseyjhol
force-pushed
the
bubble-click-events
branch
from
8f87857
to
9048cf5
Compare
|
Merge conflicts have been resolved. |
|
You can not open the dropdowns now: |
|
|
When will this be released? Waiting for bootstrap-select that requires this fix. |
|
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. |
|
@caseyjhol Can you please add a test case to check that the dropdown should be shown when clicking on a child element of Also, I believe that #33044 can be fixed in the same PR |
js/src/dropdown.js
Outdated
| dropdownMenu.contains(event.target)) { | ||
| continue | ||
| if (event) { | ||
| if (context._element === SelectorEngine.parents(event.target, SELECTOR_DATA_TOGGLE)[0]) { |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
|
@rohit2sharma95 @XhmikosR Any more thoughts/comments? |
|
@caseyjhol can you please make a proper rebase?
@RyanBerliner could you have a quick look please? |
|
Tests for form inside menu can be left less strict, those will be updated in #33389. |
There was a problem hiding this comment.
@caseyjhol Can you please address few last comments? Only you can edit this PR 🙂
js/tests/unit/dropdown.spec.js
Outdated
| btnDropdown.addEventListener('shown.bs.dropdown', () => setTimeout(() => { | ||
| expect(btnDropdown.classList.contains('show')).toEqual(true) | ||
| expect(btnDropdown.getAttribute('aria-expanded')).toEqual('true') | ||
| done() | ||
| })) |
There was a problem hiding this comment.
setTimeout is not required here IMO
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
Do you want me to remove all code related to checking for the form
No, that should be handled in the next PR.
caseyjhol
force-pushed
the
bubble-click-events
branch
from
b5f3d91
to
aa1902c
Compare
caseyjhol
force-pushed
the
bubble-click-events
branch
from
2c4455b
to
ccfe261
Compare
|
Sorry - not sure what happened when I was rebasing...getting this sorted. |
caseyjhol
force-pushed
the
bubble-click-events
branch
from
fc3ba6c
to
f08dae5
Compare
There was a problem hiding this comment.
@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
| if ([context._element].some(element => event.composedPath().includes(element))) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
@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!
There was a problem hiding this comment.
Okay, for smooth release please restore setTimeouts and rebase your branch 🙂
|
@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 |
Fixes #30267. Fixes #33044.
Demo of #33044 being fixed: https://jsbin.com/vureqihoze/edit?html,output.
Preview: https://deploy-preview-32942--twbs-bootstrap.netlify.app/