Skip to content

fix(tooltip): dismiss tooltip on esc#1846

Open
akashsonune wants to merge 1 commit intomainfrom
fix/dismiss-tooltip-on-esc
Open

fix(tooltip): dismiss tooltip on esc#1846
akashsonune wants to merge 1 commit intomainfrom
fix/dismiss-tooltip-on-esc

Conversation

@akashsonune
Copy link
Copy Markdown
Member

@akashsonune akashsonune commented Apr 9, 2026

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request attempts to add functionality to hide the siTooltip directive when the Escape key is pressed. However, the current implementation using a (keydown.escape) host listener is not fully effective as it only works when the directive's element has focus. A more robust solution is suggested, involving listening for global Escape key events within the TooltipRef service using Angular CDK's OverlayRef to ensure the tooltip can be dismissed regardless of focus.

@akashsonune akashsonune force-pushed the fix/dismiss-tooltip-on-esc branch 2 times, most recently from 4c667e6 to 30f6c28 Compare April 10, 2026 09:13
@akashsonune akashsonune marked this pull request as ready for review April 10, 2026 09:26
@akashsonune akashsonune requested review from a team as code owners April 10, 2026 09:26
Copy link
Copy Markdown
Member

@spike-rabbit spike-rabbit left a comment

Choose a reason for hiding this comment

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

please add a unit test

@akashsonune akashsonune force-pushed the fix/dismiss-tooltip-on-esc branch 4 times, most recently from 0b630d6 to 8c5ee80 Compare April 10, 2026 11:11
@akashsonune akashsonune force-pushed the fix/dismiss-tooltip-on-esc branch from 8c5ee80 to c00134b Compare April 10, 2026 14:26
@akashsonune
Copy link
Copy Markdown
Member Author

please add a unit test

Done.

It seems that the navbar expects the tooltip to be open when the menu is closed in one of the tests. The test is failing. because the escape would call the hide() and close the tooltip. So now we check if the tooltip was even visible before calling the hide

@akashsonune akashsonune added this to the 49.x milestone Apr 10, 2026
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.

2 participants