fluentui icon indicating copy to clipboard operation
fluentui copied to clipboard

V9 Tooltip should stop event propagation on escape

Open micahgodbolt opened this issue 3 years ago • 1 comments

Similar to #14465, tooltips on focus targets in surfaces (modal, menu etc) should stop the propagation of the escape key so that the parent is not also dismissed.

v9 example of current behavior

Dismissing a tooltip on a button in a menu, also closed the menu https://codesandbox.io/s/hopeful-fast-9qrbbj

v8 example of expected/past behavior

Dismissing the tooltip on the button in a modal does not close the modal https://codesandbox.io/s/romantic-cerf-cw85t9?file=/src/App.tsx

micahgodbolt avatar Sep 14 '22 23:09 micahgodbolt

My assumption here is: Tooltip component attaches the keydown listener to the document when MenuPopover attaches the event listener to the div element with role presentation. So, it looks like the event from MenuPopover executes first and then Tooltip actions, as a result MenuPopover closes everything. The old version (v8) of Tooltip attaches event to the direct componet instead of attaching keydown listener to the document.

There are a few approaching of fixing it:

  1. Move Tooltip keydown listener from document to the children component
  2. Change
      const onDocumentKeyDown = (ev: KeyboardEvent) => {
        if (ev.key === 'Escape' || ev.key === 'Esc') {
          thisTooltip.hide();
          ev.stopPropagation();
        }
      };

      targetDocument?.addEventListener('keydown', onDocumentKeyDown);

      return () => {
        if (context.visibleTooltip === thisTooltip) {
          context.visibleTooltip = undefined;
        }

        targetDocument?.removeEventListener('keydown', onDocumentKeyDown);
      };
 to
      const onDocumentKeyDown = (ev: KeyboardEvent) => {
        if (ev.key === 'Escape' || ev.key === 'Esc') {
          thisTooltip.hide();
          ev.stopPropagation(); <--------
        }
      };

      targetDocument?.addEventListener('keydown', onDocumentKeyDown, true); <--------

      return () => {
        if (context.visibleTooltip === thisTooltip) {
          context.visibleTooltip = undefined;
        }

        targetDocument?.removeEventListener('keydown', onDocumentKeyDown, true); <--------
      };

packages\react-components\react-tooltip\src\components\Tooltip\useTooltip.tsx The number two looks easier than the first one because it is hard to attach an event listener to the children + there are a few more approaches to fix it which maybe I do not see now.

I hope my research of the issue will help somebody :)

StepanNaryshkov avatar Sep 22 '22 06:09 StepanNaryshkov

Probably related to this issue: when tabbing and you come to a component with a tooltip, if esc is pressed to close the tooltip, the component you were focused on also loses focus. Here's a video of this behavior on the v9 playground page. In the video, I hit tab to focus on the button and then esc to close the tooltip. Untitled video - Screen Recording - 10_14_2022, 3_17_11 PM.webm

aslynblohm avatar Oct 14 '22 22:10 aslynblohm

I actually soft-disagree with the proposal to stop propagation of escape for tooltips for a few reasons:

  1. A user may be unaware they have a tooltip open, either because they're using a screen reader, or they're just not paying attention to the tiny popup. In these cases, it would be weird to have escape not dismiss the primary popup the user is interacting with.
  2. Tooltips need to be dismissable regardless of where keyboard focus is (i.e. hover tooltips when focus is elsewhere). This needs to happen without moving the mouse cursor, so essentially the keyboard dismissal needs to work with hover tooltips. If we choose escape for this, accidentally hovering over a thing with a tooltip means that escape doesn't dismiss your dialog (or menu, callout, etc.).
  3. In a user study, I've found people are conservative about using escape to dismiss tooltips when they are within dialogs and other popups.

I'd recommend we let escape propagate from the tooltip, and use the control key and shift + F11 (or whatever that function key was) as the alternate means to dismiss tooltips without also dismissing the popup.

smhigley avatar Nov 04 '22 22:11 smhigley

This already fixed with this commit https://github.com/microsoft/fluentui/commit/81de40f0df0dfa8bac4e4fb522bf830d9fe8e066

tummalasujith avatar Nov 04 '22 23:11 tummalasujith