Update aria-pressed state for ToolbarButton when pressed

[MUFFANUJ]

References

This PR fixes #14464 where the "More commands" button in the toolbar overflow menu did not reflect its pressed state. The aria-pressed attribute is now correctly toggled when the button is clicked.

Code changes

• Updated onClick logic to toggle the pressed state.
• Ensured aria-pressed updates dynamically based on the button’s pressed state.

After necessary changes

Screen.Recording.2025-01-26.at.10.29.44.PM.mov

[jupyterlab-probot]

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

[MUFFANUJ]

@meeseeksdev tag bug

[krassowski]

Does it also work when using Space to toggle it?

[MUFFANUJ]

Does it also work when using Space to toggle it?

I have updated the function passed through props to handle both onClick and onKeyDown events, which will simply ensure it works everywhere it is used. While testing for Space and Enter functionality, I noticed a missing piece of code causing the action to behave inconsistently. Should I address this within the current issue or open a new one to formalize the fix?

Here Is the demo for the issue when I tried to toggle using space

Screen.Recording.2025-01-27.at.5.46.54.PM.mov

Issue lies here:

jupyterlab/packages/ui-components/src/components/toolbar.tsx

Lines 842 to 848 in a3eb9b5

	const handleKeyDown = (event: React.KeyboardEvent) => {
	const { key } = event;
	if (key === 'Enter' || key === ' ') {
	props.onClick?.();
	}
	};

Proposed Solution

const handleKeyDown = props.noFocusOnClick ?? false ? (event: React.KeyboardEvent) => {
  const { key } = event;
  if (key === 'Enter' || key === ' ') {
    props.onClick?.();
  }
} : undefined;

This implementation works fine because it conditionally assigns handleKeyDown only if props.noFocusOnClick is true, ensuring the function exists only when needed.

In contrast, the code implementation used so far always defines handleKeyDown, even when it shouldn't be triggered (e.g. when props.noFocusOnClick is false). This could lead to unintended behavior, such as executing props.onClick unnecessarily.

[krassowski]

I can reproduce the issue that you are referring to. I am not sure what solution would be the best so maybe let's open a new issue to discuss it first :)

[MUFFANUJ]

I have opened a new issue for the same #17229

[MUFFANUJ]

Hey @krassowski, After spending additional time investigating this issue, I have identified a relevant solution that addresses the underlying cause of the problem. I have also opened a pull request with the proposed fix (#17230). If the pull request is merged, we can close this one as well.

[MUFFANUJ]

hey @krassowski @Darshan808 , since this is closed #17230 we can merge this one as well ?

[krassowski]

Testing this on Binder I saw flickering between states when using keyboard, merging main branch in to make sure if this was because the other PR was not merged into this branch yet.

[krassowski]

No, I still see that pressing space does not toggle it correctly when using this PR.

[MUFFANUJ]

Yes i too tested on binder and see the flickering effect. Will see what needs to be fix and make the changes

[MUFFANUJ]

hey @krassowski, checked on binder it seems to be working now :)

[krassowski]

Thank you!