[popover2] fix(Popover2): don't react twice to simulated keyboard clicks

[justinbhopper]

Fixes #5775

Checklist

• Includes tests
• Update documentation

Changes proposed in this pull request:

Adds check in Popover2 to expect and ignore a simulated click that follows any keyboard click (enter/space) event.

Reviewers should focus on:

The root of the bug is caused by the following in single keystroke (up and down) when pressing Enter on a button that is wrapped within a popover:

1. onKeyDown "Enter" occurs on AbstractButton, which bubbles to the Popover2. Popover2 handles this and opens as a result.
2. onKeyUp "Enter" occurs on AbstractButton, which it is programmed to dispatch a click event.
3. The click event reaches Popover2, which causes it to immediately toggle back to close.

This PR adds code to track when enter/space happens prior to a click event, and then to ignore any subsequent simulated click event (detected via isTrusted flag). If any real click or other keyboard events occur, the tracking property is reset.

[justinbhopper]

@adidahiya An alternative fix is to remove the onKeyDown handler that was added to Popover2. Popover (v1) does not support this, and it is ultimately the root cause of the issue.

[justinbhopper]

Another alternative: Keep the onKeyDown handler, but detect if the target of the event has a bp4-button classname. If it does, we do not open from it, because we expect that a simulated click event will come later.

[adidahiya]

@justinbhopper thanks for the PR. I like the idea of checking e.isTrusted, this seems like a good use of that property. I would suggest going even further and also checking that the keyDown event came from a known Blueprint component that triggers "fake" click events (in this case, e.target.matches(Classes.BUTTON). This approach will change Popover2 behavior in the least number of cases, which minimizes the possibility of unintended consequences from this PR.

I think we should add unit tests for this change. I poked at the code a bit and tried to add a test but I wasn't able to come up with a failing test (see my work in progress commit here). Can you try adding a unit test or two?

I'm not 100% sure but it seems like the behavior is different based on whether autoFocus is enabled (the Popover2 example does not have the bug, while the Select2 example does), so we should test both cases when autoFocus is and isn't enabled.

[justinbhopper]

@adidahiya You were right, autoFocus=true was the reason that the Popover2 example did not have the same issue.

This is because autoFocus steals focus as soon as keydown occurs. This means the subsequent keyup does not happen on the Button anymore, but instead on some element within the overlay/popover.

I've added tests based on your WIP example. I had to use a real focus() call instead of a simulated one, because it seems simulated ones don't actually update the document.activeElement in tests.

I've also narrowed the simulated-click check to only be for bp4-button as you requested.

[adidahiya]

I'm still getting double reactions in the Select2 example in the latest docs preview build (pressing both Enter and Space keys):

[justinbhopper]

@adidahiya Turns out that was caused by #6149. The popover for Select2 was being dismissed because the focus was immediately going into the Filterable input field, and the old logic was interpreting incorrectly as a click on a menu item.

While investigating this, I found out that Select2 completely overrides the onKeyDown that is normally handled by Popover2. This means its not possible for the Popover2 to know whether the last keydown event was a keyboard click.

Therefore, I coded it to just make the assumption that it should ignore simulated click events from Button if it's current state is already open. If the user presses Enter/Space while the popover is open, the keydown event will already close it, so the simulated click doesn't serve any purpose in any realistic scenario.

[adidahiya]

code changes look fine, and I can now open the select popover with enter/space keys. but there is a regression where selecting an item in the list with the Enter key no longer closes the popover in the latest docs preview build:

here is the expected behavior on develop edit: I was actually testing on the last docs release, core v4.18.0 (requires clicking to open):

[justinbhopper]

@adidahiya I just pulled latest from develop and it seems like this regression bug exists there too. I am guessing #6149 had this unintended side effect and its not due to the changes in this PR.

The target of the event passed in onItemSelect is apparently the target button, and not the MenuItem element like the code seems to think.

[adidahiya]

@justinbhopper ah, sorry about that, you're right. There is a regression on develop, I can see it on this commit preview build: 4ee638c#comments. I'll fix that after merging this PR.

[adidahiya]

Note for posterity: there was apparently some code in QueryList which attempted to work around this problem already. But it seems like the event.preventDefault() was insufficient or buggy:

blueprint/packages/select/src/components/query-list/queryList.tsx

Lines 549 to 553 in db161b4

	// We handle ENTER in keyup here to play nice with the Button component's keyboard
	// clicking. Button is commonly used as the only child of Select. If we were to
	// instead process ENTER on keydown, then Button would click itself on keyup and
	// the Select popover would re-open.
	event.preventDefault();