Skip to content

Commit

Permalink
[86bxrwyxa][popper] fixed fast keyboard navigation issues (#1271)
Browse files Browse the repository at this point in the history
## Motivation and Context

Fast keyboard navigation over closely placed tooltips may cause some
tooltips to stay visible even after triger blur. This PR fixes it.

## How has this been tested?

I found no way to cover it with tests. I've been testing it on 4 placed
next to each other tooltips with hover interaction.

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [x] Bug fix (non-breaking change which fixes an issue).
- [ ] New feature (non-breaking change which adds functionality).
- [ ] Breaking change (fix or feature that would cause existing
functionality to not work as expected).
- [ ] Nice improve.

## Checklist:

<!--- Go over all the following points, and put an `x` in all the boxes
that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->

- [x] My code follows the code style of this project.
- [x] I have updated the documentation accordingly or it's not required.
- [x] Unit tests are not broken.
- [x] I have added changelog note to corresponding `CHANGELOG.md` file
with planned publish date.
- [ ] I have added new unit tests on added of fixed functionality.
  • Loading branch information
msereniti authored Apr 9, 2024
1 parent 0054141 commit 0af501e
Show file tree
Hide file tree
Showing 6 changed files with 53 additions and 0 deletions.
20 changes: 20 additions & 0 deletions semcore/popper/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,32 @@

CHANGELOG.md standards are inspired by [keepachangelog.com](https://keepachangelog.com/en/1.0.0/).

## [5.26.1] - 2024-09-04

### Fixed

- Holding mouse over a place where popper appear was preventing popper from disappear back.
- Fast keyboard navigation over multiple closely placed poppers was causing multiple poppers to stay open.

## [5.26.0] - 2024-03-27

### Fixed

- Select and similar components with disabled portal were not working properly being wrapped into `label` tag.

## [5.25.2] - 2024-04-03

### Fixed

- Holding mouse over a place where popper appear was preventing popper from disappear back.
- Fast keyboard navigation over multiple closely placed poppers was causing multiple poppers to stay open.

## [5.25.1] - 2024-04-03

### Fixed

- Select with `disabpledPortal` inside a label was opening second time after selecting the option.

## [5.25.0] - 2024-03-27

### Changed
Expand Down
1 change: 1 addition & 0 deletions semcore/popper/__tests__/index.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ describe('Popper', () => {
</Popper>,
);

fireEvent.mouseMove(getByTestId('reference'));
fireEvent.mouseEnter(getByTestId('reference'));
// because wait call onVisibleChange
act(() => {
Expand Down
1 change: 1 addition & 0 deletions semcore/popper/__tests__/stands/disablePortal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const Demo = () => {
</DropdownMenu.Popper>
</DropdownMenu>
<input />
<input />
</div>
);
};
Expand Down
27 changes: 27 additions & 0 deletions semcore/popper/src/Popper.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,13 @@ const useUpdatePopperEveryFrame = (popperRef) => {
return handleAnimationFrame;
};

let lastMouseMove = 0;
if (canUseDOM()) {
document.addEventListener('mousemove', () => {
lastMouseMove = Date.now();
});
}

const MODIFIERS_OPTIONS = [
'offset',
'preventOverflow',
Expand Down Expand Up @@ -274,6 +281,13 @@ class Popper extends Component {
return;
}
}
/**
* When popper appears right under mouse that doesn't move, it gets undesired onMouseEnter event.
* That may cause hover interaction poppers to display two closely placed poppers.
* That check ensures that onMouseEnter means mouse entered the popper, not popper entered the mouse.
*/
if (action === 'onMouseEnter' && Date.now() - lastMouseMove > 100) return;

const now = Date.now();
const focusAction = ['onFocus', 'onKeyboardFocus', 'onFocusCapture'].includes(action);
if (
Expand All @@ -284,6 +298,19 @@ class Popper extends Component {
) {
return;
}
if (!visible) {
/**
* When sibling popovers triggers with hover/focus interactions are navigated fast,
* sometimes focus moves into closing popovers and prevents it from closing. It may cause
* multiple popovers to be opened at the same time.
*/
setTimeout(() => {
const node = this.popperRef.current;
if (node?.getAttribute('tabindex') === '0') {
node.setAttribute('tabindex', '-1');
}
}, 0);
}
const currentTarget = e?.currentTarget;
this.handlerChangeVisibleWithTimer(visible, e, () => {
clearTimeout(this.timerMultiTrigger);
Expand Down
1 change: 1 addition & 0 deletions semcore/tooltip/__tests__/index.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,7 @@ describe('TooltipBase', () => {
</Tooltip>,
);

fireEvent.mouseMove(getByTestId('trigger'));
fireEvent.mouseEnter(getByTestId('trigger'));
act(() => {
vi.runAllTimers();
Expand Down
3 changes: 3 additions & 0 deletions semcore/tooltip/__tests__/tooltip.browser-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ test.describe('Tooltip', () => {
await page.mouse.move(
triggerRect.x + triggerRect.width / 2,
triggerRect.y + triggerRect.height / 2,
{
steps: 5,
},
);

await new Promise((resolve) => setTimeout(resolve, 1000));
Expand Down

0 comments on commit 0af501e

Please sign in to comment.