Skip to content

fix(ui5-split-button): fire click event when focused with shift + tab #11197

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hinzzx
Copy link
Contributor

@hinzzx hinzzx commented Mar 26, 2025

Main Problem

There was an issue, where if the ui5-split-button was to be focused by using Shift + Tab combination, the click event wasn't firing, after the first time a default action was executed (pressed Enter or Space).

The reason behind this, was a flag called _shiftOrEscapePressed.
Since the component has a handler for focus-in event which was just early returning if the component matches the :focus-within CSS selector, which was causing the flag to NOT reset. Therefore the component wasn't firing click event, due to the condition:

_fireClick(e?: Event) {
   e?.stopPropagation();
   if (!this._shiftOrEscapePressed) {
      this.fireDecoratorEvent("click");
   }
   this._shiftOrEscapePressed = false;
}

Solutions

  • We solved few issues, by introducing a new flag called _shiftOrEscapePressedDuringSpace, which handles the cases only when Shift or Escape keys are pressed simultaneously with the Space Bar.
  • Deleted an unused forgotten flag;
  • Handled an issue where the arrow button was being 'frozen' if simultaneously pressing ArrowUp/Down keys and Tab/Shift + Tab.
  • Shift/Escape keys now correctly prevent events from firing;
  • Fixed a bug where arrow-button fired click AND arrow-click events.

Note: Test will be written in a separate change when migrating the SplitButton.spec.js file to a SplitButton.cy.tsx

Fixes: #11115

@hinzzx hinzzx requested a review from Copilot March 28, 2025 07:56
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue where the ui5-split-button failed to fire the click event when focused with Shift + Tab and refines the key event handling logic.

  • Removed the onFocusIn handler from the button template.
  • Renamed and replaced the flag handling (using _shiftOrEscapePressedDuringSpace instead of _shiftOrEscapePressed) to address conflicts with combined key presses.
  • Refactored key event handlers to reset UI states and correctly manage default actions.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
packages/main/src/SplitButtonTemplate.tsx Removed the onFocusIn attribute from the button template to prevent premature state resetting.
packages/main/src/SplitButton.ts Renamed the flag and refactored key event handling logic for correct click event firing behavior.

}

if (this._isShiftOrEscape(e)) {
this._handleShiftOrEscapePressed();
if (isEnter(e)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest combining this statement and the one below into a variable like "shouldToggleOffActiveButton" and then if (should ...): this._buttonActive = false to avoid the duplicate code

Copy link
Contributor

@tsanislavgatev tsanislavgatev left a comment

Choose a reason for hiding this comment

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

I don't really like the idea of always having flags to follow the states. Can we use checks for modifier keys? Can we try this? If not we can fallback to this solution, but let's explore using checks in the events for key modifiers. An example can be seein in Keys.js in the method isSpaceShift.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SF][SplitButton]: [SplitButton requires two presses of Enter or Space to be activated]
3 participants