fix(tooltip): consistent cross-browser behavior with disabled elements #712

taifen · 2020-12-06T10:32:17Z

Thank you for contributing to Reach UI! Please fill in this template before submitting your PR to help us process your request more quickly.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code (Compile and run).
Add or edit tests to reflect the change (Run with yarn test).
Add or edit Storybook examples to reflect the change (Run with yarn start).
Ensure formatting is consistent with the project's Prettier configuration.
Add documentation to support any new features.

This pull request:

Creates a new package
Fixes a bug in an existing package
Adds additional features/functionality to an existing package
Updates documentation or example code
Other

Thanks for the great library!

This PR fixes #231 and #564

We use @reach/tooltip with disabled buttons and inputs and the need to create a wrapper around such components is rather inconvenient.

Pointer events API is widely supported and PR contains a fallback just in case.

Unfortunately couldn't update component tests because there is no support in jsdom yet jsdom/jsdom#2666, I tried to use the proposed workarounds but they didn't work for me. So tests actually check that component still works if there is no pointer events support.

It seems that Safari has incorrect implementation and triggers only onPointerEnter so I added a workaround for that. Checked on BrowserStack in IE11, Edge, Firefox, Chrome, and mobile Safari/Chrome and didn't notice a negative impact from that workaround.

codesandbox-ci · 2020-12-06T11:18:22Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0e7a14b:

Sandbox	Source
reach-ui-template	Configuration

taifen · 2020-12-10T15:24:52Z

Hi @chaance. Could you take a look, please?

raunofreiberg · 2020-12-11T22:04:07Z

Some context: #564 (comment)

As an alternative, you could drop down to the useTooltip hook and have your own Tooltip component use pointer events, which is what I do right now 🙃

Something like this would work:

import { TooltipPopup, useTooltip } from '@reach/tooltip';

function Tooltip() {
  const [trigger, tooltip, isVisible] = useTooltip();

  const { onMouseEnter, onMouseLeave, ...triggerProps } = trigger;

  return (
    <>
      {React.cloneElement(
        children,
        children.props.disabled
          ? {
              onPointerEnter: onMouseEnter,
              onPointerLeave: onMouseLeave,
              ...triggerProps,
            }
          : trigger,
      )}
      {isVisible && <TooltipPopup />}
    </>
  );
}

taifen · 2020-12-11T22:13:51Z

Some context: #564 (comment)

As an alternative, you could drop down to the useTooltip hook and have your own Tooltip component use pointer events, which is what I do right now 🙃

Something like this would work:

import { TooltipPopup, useTooltip } from '@reach/tooltip';

function Tooltip() {
  const [trigger, tooltip, isVisible] = useTooltip();

  const { onMouseEnter, onMouseLeave, ...triggerProps } = trigger;

  return (
    <>
      {React.cloneElement(
        children,
        children.props.disabled
          ? {
              onPointerEnter: onMouseEnter,
              onPointerLeave: onMouseLeave,
              ...triggerProps,
            }
          : trigger,
      )}
      {isVisible && <TooltipPopup />}
    </>
  );
}

Thanks! I need to support Safari though, so I use a forked version for now

chaance

This looks good to me, just a few questions and requests. Thanks so much for the time here!

chaance · 2020-12-24T05:38:25Z

packages/tooltip/src/index.tsx

+      let target = event.target as Element | null;
+      if (
+        target?.hasAttribute("data-reach-tooltip-trigger") ||
+        target?.closest("[data-reach-tooltip-trigger][aria-describedby]")


Can you explain the addition of [aria-describedby] in the selector here?

We can have multiple disabled elements (icon buttons, for example) with tooltips next to each other, so I wanted to make sure that GLOBAL_MOUSE_MOVE will be triggered if you quickly move to the next disabled element. Used this attribute since it is added only for elements with visible tooltip:

"aria-describedby": isVisible ? makeId("tooltip", id) : undefined,

And I missed aria-describedby in the first part of the condition initially, fixed that

At first, I wanted to use ownRef.current?.isEqualNode(target) || ownRef.current?.contains(target) instead, but that didn't work and I'm not sure why.

Ah I think that makes sense. That could be a buggy assumption though seeing as how you can arbitrarily pass aria-describedby as well. I might add a data selector to the trigger params. We already use data-state elsewhere, so maybe:

data-state={isVisible ? 'tooltip-visible' : 'tooltip-hidden'}

That might make the selector a bit more reliable! Also, you can just get away with ownRef.current.contains I think, that should return true even if the element itself matches the query.

ownRef.current.contains doesn't work in this case, unfortunately. I assume because of ref mutations.

But I like data-state, it's also more intuitive than the previous selector. Thanks! Also, closest returns the element itself so I removed checks for attributes on the target.

chaance · 2020-12-24T05:45:35Z

packages/tooltip/src/index.tsx

+
+    function listener(event: MouseEvent) {
+      // @ts-ignore `disabled` does not exist on HTMLDivElement but tooltips can be used with different elements
+      if (state !== VISIBLE || !ownRef.current?.disabled) return;


Instead of checking the ref's element here, have you considered accepting disabled as an additional prop for the hook? That would allow us to use it as a dependency for the effect and avoid attaching useless event listeners for non-disabled components.

Not sure if I understood correctly. Do you mean something like:

<Tooltip id="wow" label="Notifications" isTriggerDisabled={isDisabled}> <Button isDisabled={isDisabled} /> </Tooltip>

?

Since this whole event listener is a workaround only for one browser I wouldn't add any additional prop to the hook or component. One more event listener in addition to another 4 doesn't make a huge impact and it's created only once. And it can be safely removed when/if the issue will be fixed in Safari.

Yeah it's a bit awkward as for the component API, but I think it makes a little more sense for the hook API. It would compose pretty naturally on buttons since disabled is a built-in prop for JSX (less-so if you have your own button API with something like isDisabled, but still). I'd be comfortable leaving it out of the component for the time being and considering that API separately while supporting the core logic in the hook.

const CustomTooltipButton = React.forwardRef(({ label, ...props }, ref) => { // as long as `disabled` is a prop, this should work without changes to the existing API let [trigger, tooltip] = useTooltip({ ...props, ref }); return ( <React.Fragment> <button {...props} {...trigger} /> <CustomTooltipPopup label={label} {...tooltip} /> </React.Fragment> ); });

Actually, I just realized since we're already using cloneElement in Tooltip this is absolutely trivial to solve there as well. If disabled is passed to the child this should all just work with the addition of one line:

disabled: child.props.disabled

Pushed an update.

I'm a bit concerned about the meaning of disabled param here though. Knowing implementation details I understand that it belongs to trigger element params. Otherwise, I would expect the tooltip itself to be disabled when disabled: true is passed to useTooltip

Yeah that's a good point too. The obvious alternatives aren't clicking for me as good APIs. Let me think about it a bit, but if you have any other ideas I'm open. My main thing is, if we use the DOM as the source of truth we still need it to be reactive, so the effect would probably need a mutation observer or something.

The more I think about this, the less concerned I am about the distinction between a disabled trigger and a disabled tooltip. Here's why:

There really isn't a concept of a disabled tooltip IMO. W3C describes the aria-disabled state as an "element [that] is perceivable but disabled, so it is not editable or otherwise operable".

A tooltip is never really operable. It's not a control, but meant to provide supplementary content that is never supposed to be interactive anyway. Presumably if it were disabled, it wouldn't pop up at all which means its content would no longer be perceivable.

If for whatever reason someone decided that they had a valid use case for turning off a tooltip, they should probably compose their own with useTooltip and render it conditionally. Using disabled to describe a tooltip seems like it would always be semantically wrong.

Lastly, the other props accepted by useTooltip are composed by the trigger. Props intended for the tooltip itself can be passed directly to TooltipPopup. So to me it makes sense that disabled would be passed to the trigger in this case.

chaance · 2020-12-24T05:47:19Z

packages/tooltip/src/index.tsx

    transition(MOUSE_ENTER, { id });
  }

-  function handleMouseMove() {
+  function handleMouseOrPointerMove() {


Nit here, but can we keep all of these function names handleMouse instead of handleMouseOrPointer? I recognize why you changed these, but considering we're still only acting on mouse events I think the intent is better communicated as-is.

Updated ✅

karlhorky · 2021-01-29T11:12:10Z

packages/tooltip/src/index.tsx

+    ourHandler: (event: EventType) => any
+  ) {
+    // Use internal MouseEvent handler only if PointerEvent not supported
+    if ("PointerEvent" in window) return theirHandler;


Hmm... seems like this line is causing Gatsby to fail on server-side rendering:

$ cross-env NODE_ENV=production gatsby build failed Building static HTML for pages - 0.750s error "window" is not available during server side rendering. 246 | function wrapMouseEvent(theirHandler, ourHandler) { 247 | // Use internal MouseEvent handler only if PointerEvent is not supported > 248 | if ("PointerEvent" in window) return theirHandler; | ^ 249 | return wrapEvent(theirHandler, ourHandler); 250 | } 251 | WebpackError: ReferenceError: window is not defined - tooltip.esm.js:248 /[@reach]/tooltip/dist/tooltip.esm.js:248:1 - tooltip.esm.js:333 /[@reach]/tooltip/dist/tooltip.esm.js:333:1 not finished Generating image thumbnails - 147.857s

Reported in #738

taifen force-pushed the fix/tooltip-for-disabled-elements branch from 6b81725 to 784f6a0 Compare December 6, 2020 11:11

chaance added the Type: Bug Something isn't working label Dec 24, 2020

chaance requested changes Dec 24, 2020

View reviewed changes

chaance added the Status: Awaiting Response Requested and awaiting a response from the issue creator label Dec 24, 2020

taifen force-pushed the fix/tooltip-for-disabled-elements branch from 784f6a0 to 759c432 Compare December 24, 2020 13:15

tooltip: consistent behavior for disabled elements

8d457c7

taifen force-pushed the fix/tooltip-for-disabled-elements branch from 759c432 to 8d457c7 Compare December 24, 2020 13:44

taifen requested a review from chaance December 24, 2020 14:31

taifen added 2 commits December 24, 2020 20:23

tooltip: add 'data-state' attribte to trigger

fa92787

tooltip: attach global move handler only if trigger element is disabled

0e7a14b

chaance approved these changes Dec 27, 2020

View reviewed changes

chaance merged commit 2317407 into reach:develop Dec 27, 2020

taifen deleted the fix/tooltip-for-disabled-elements branch December 27, 2020 19:08

karlhorky reviewed Jan 29, 2021

View reviewed changes

taifen mentioned this pull request Feb 6, 2021

fix(tooltip): prevent crashing when using SSR #742

Merged

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tooltip): consistent cross-browser behavior with disabled elements #712

fix(tooltip): consistent cross-browser behavior with disabled elements #712

taifen commented Dec 6, 2020 •

edited

Loading

codesandbox-ci bot commented Dec 6, 2020 •

edited

Loading

taifen commented Dec 10, 2020

raunofreiberg commented Dec 11, 2020

taifen commented Dec 11, 2020

chaance left a comment

chaance Dec 24, 2020

taifen Dec 24, 2020 •

edited

Loading

chaance Dec 24, 2020

taifen Dec 24, 2020

chaance Dec 24, 2020

taifen Dec 24, 2020

chaance Dec 24, 2020

chaance Dec 24, 2020

taifen Dec 24, 2020

chaance Dec 27, 2020

chaance Dec 27, 2020

chaance Dec 24, 2020

taifen Dec 24, 2020 •

edited

Loading

karlhorky Jan 29, 2021 •

edited

Loading

fix(tooltip): consistent cross-browser behavior with disabled elements #712

fix(tooltip): consistent cross-browser behavior with disabled elements #712

Conversation

taifen commented Dec 6, 2020 • edited Loading

codesandbox-ci bot commented Dec 6, 2020 • edited Loading

taifen commented Dec 10, 2020

raunofreiberg commented Dec 11, 2020

taifen commented Dec 11, 2020

chaance left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taifen Dec 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taifen Dec 24, 2020 • edited Loading

Choose a reason for hiding this comment

karlhorky Jan 29, 2021 • edited Loading

Choose a reason for hiding this comment

taifen commented Dec 6, 2020 •

edited

Loading

codesandbox-ci bot commented Dec 6, 2020 •

edited

Loading

taifen Dec 24, 2020 •

edited

Loading

taifen Dec 24, 2020 •

edited

Loading

karlhorky Jan 29, 2021 •

edited

Loading