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

Fix dropdown escape propagation #33479

Merged
merged 5 commits into from Apr 1, 2021

Conversation

alpadev
Copy link
Contributor

@alpadev alpadev commented Mar 25, 2021

Closes #33465

@alpadev alpadev requested a review from a team as a code owner March 25, 2021 17:36
@GeoSot
Copy link
Member

GeoSot commented Mar 26, 2021

LGTM

@rohit2sharma95 rohit2sharma95 added this to Inbox in v5.0.0 via automation Mar 26, 2021
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.

if (!isActive || event.key === SPACE_KEY) {
Dropdown.clearMenus()
return
}

This should not be changed as below? 🤔

if (isActive && event.key === SPACE_KEY) {
  Dropdown.clearMenus()
  return
}

js/src/dropdown.js Outdated Show resolved Hide resolved
@alpadev
Copy link
Contributor Author

alpadev commented Mar 30, 2021

if (!isActive || event.key === SPACE_KEY) {
Dropdown.clearMenus()
return
}

This should not be changed as below? 🤔

if (isActive && event.key === SPACE_KEY) {
  Dropdown.clearMenus()
  return
}

@rohit2sharma95 Not sure if this is even needed.

This checks for keys that are dropdown commands.

if (/input|textarea/i.test(event.target.tagName) ?
event.key === SPACE_KEY || (event.key !== ESCAPE_KEY &&
((event.key !== ARROW_DOWN_KEY && event.key !== ARROW_UP_KEY) ||
event.target.closest(SELECTOR_MENU))) :
!REGEXP_KEYDOWN.test(event.key)) {
return
}

But the regex is missing the space key so I'm not sure if we even reach this block.

const REGEXP_KEYDOWN = new RegExp(`${ARROW_UP_KEY}|${ARROW_DOWN_KEY}|${ESCAPE_KEY}`)

@rohit2sharma95
Copy link
Collaborator

rohit2sharma95 commented Mar 30, 2021

This change was made in #29885 (and then in #30597, that's unrelated though). Anything related to input groups?

/CC @MartijnCuppens

@rohit2sharma95
Copy link
Collaborator

And can not the selectMenuItem method be a private method instead of a static one (as it is not being used directly 🤔)? It can be used through the context of the dropdown then IMO. Because the dataApiKeydownHandler method is bound with the dropdown elements only (menu and the toggle button).

@alpadev
Copy link
Contributor Author

alpadev commented Mar 30, 2021

And can not the selectMenuItem method be a private method instead of a static one (as it is not being used directly 🤔)? It can be used through the context of the dropdown then IMO. Because the dataApiKeydownHandler method is bound with the dropdown elements only (menu and the toggle button).

I didn't mean to refactor too much in the scope of this issue but rather to just get rid of that Eslint warning. I agree that using a static method isn't ideal but the item selection functionality wasn't bound to the instance before and I made it the way dataApiKeydownHandler was implemented.

If you want me to change it I should be able to do this tho.

@GeoSot GeoSot force-pushed the fix-dropdown-escape-propagation branch from c019d3d to 3f658e5 Compare March 30, 2021 22:48
@GeoSot
Copy link
Member

GeoSot commented Mar 30, 2021

Till now is just a refactoring that I can follow, a minor fix and a valid test that succeeds . So till here I find it OK.

I agree to refactor selectMenuItem to private method, and would also love to see dataApiKeydownHandler gordian knot untied, but on another PR, to avoid mistakes

So if we can all agree on these, I would prefer to keep it as it is and if @alpadev has the courage to keep up with a following PR

v5.0.0 automation moved this from Inbox to Approved Mar 31, 2021
@XhmikosR XhmikosR merged commit f36f834 into twbs:main Apr 1, 2021
v5.0.0 automation moved this from Approved to Done Apr 1, 2021
alpadev added a commit to alpadev/bootstrap that referenced this pull request Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

v5: Dropdown trigger button "swallows" Esc keypresses when focused
4 participants