Skip to content

Conversation

@louismaximepiton
Copy link
Member

Fixes #35968

Should be merged after #35948

Re-introducing @mdo 's changes in #35893
Tried my best to understand the expected behaviour on Esc pressed. Feel free to ask or request changes

@mdo
Copy link
Member

mdo commented Mar 8, 2022

Should be merged after #35948

This doesn't look to be required based on my testing.

@mdo mdo merged commit 6c40476 into twbs:main Mar 8, 2022
@louismaximepiton louismaximepiton deleted the main-lmp-dropdown-fixes branch March 9, 2022 08:01
@GeoSot GeoSot self-assigned this Mar 9, 2022
@GeoSot GeoSot mentioned this pull request Mar 10, 2022
@GeoSot
Copy link
Member

GeoSot commented Mar 11, 2022

@louismaximepiton I know the PR is merged now, and it is late for questions, however I need your help.

Can you explain me the reason we need event.stopPropagation() on escape event?

and if it is ok with you, is it possible to add a test (new pr) to support the 'shown' check ?

@louismaximepiton
Copy link
Member Author

louismaximepiton commented Mar 11, 2022

Yup, no problem, as explained in #35968, there was an issue when wraping a dropdown in an other element that was based on escape event, such as offcanvas : When trying to escape the dropdown, escape the offcanvas too in all cases.

Then, I thought that with your last refactoring in twbs@bb7664d, you wanted to identify two cases :

  • the opened dropdown, that should not spread the escape event
  • the closed dropdown, that should spread the escape event to its parents

For the tests, I'll try to develop them next week if you don't mind !

EDIT : Sorry to be a bit intrusive, but you should maybe consider re-opening #35948 !

@louismaximepiton louismaximepiton mentioned this pull request Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Few regressions on dropdowns

3 participants