Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tooltip] Implement fix for MouseOver event not firing when cursor enters after leaving a disabled element #1783

Merged
merged 2 commits into from Jul 9, 2019

Conversation

rexmac
Copy link
Contributor

@rexmac rexmac commented Jul 4, 2019

WHY are these changes introduced?

Fixes #1782.

MouseEnter events are not triggered when cursor enters from a disabled element.

WHAT is this pull request doing?

Uses MouseOver with a "mouseEntered" boolean flag rather than MouseEnter.

Before

polaris-tooltio-mouseenter-bug

After

polaris-tooltip-mouseenter-fix

Copy-paste this code in playground/Playground.tsx:
import * as React from 'react';
import {Page, Tooltip} from '../src';

interface State {}

export default class Playground extends React.Component<{}, State> {
  render() {
    return (
      <Page title="Playground">
        <button type="button" disabled>
          Previous
        </button>
        <Tooltip content="Next">
          <button type="button">Next</button>
        </Tooltip>
      </Page>
    );
  }
}

🎩 checklist

@ghost
Copy link

ghost commented Jul 4, 2019

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

@BPScott BPScott temporarily deployed to polaris-react-pr-1783 July 4, 2019 13:33 Inactive
@rexmac rexmac force-pushed the 1782-tooltip-mouseenter-fix branch from b47a915 to f01c0d8 Compare July 4, 2019 13:36
@BPScott BPScott temporarily deployed to polaris-react-pr-1783 July 4, 2019 13:37 Inactive
@rexmac rexmac self-assigned this Jul 4, 2019
@rexmac rexmac added the Bug Something is broken and not working as intended in the system. label Jul 4, 2019
@ry5n ry5n requested a review from dpersing July 4, 2019 17:35
Copy link
Contributor

@ry5n ry5n left a comment

Choose a reason for hiding this comment

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

I asked @dpersing to take a look at this since it has accessibility implications. Keyboard and screen reader users already won’t be able to read the tooltip, so I’m not sure this change makes anything worse. I do wonder if this might open up tooltips to be used in more situations where maybe they shouldn’t be…

@dpersing
Copy link
Contributor

dpersing commented Jul 5, 2019

Hi @rexmac! Could you include the Playground code you show in the gifs so we can manually text your example? There should be a section of the PR template for it:

<!--
  Give as much information as needed to experiment with the component
  in the playground.
-->

<details>
<summary>Copy-paste this code in <code>playground/Playground.tsx</code>:</summary>

```jsx
import * as React from 'react';
import {Page} from '@shopify/polaris';

interface State {}

export default class Playground extends React.Component<never, State> {
  render() {
    return (
      <Page title="Playground">
        {/* Add the code you want to test here */}
      </Page>
    );
  }
}
```

</details>

@rexmac
Copy link
Contributor Author

rexmac commented Jul 5, 2019

@dpersing Added. 😃

@dpersing
Copy link
Contributor

dpersing commented Jul 5, 2019

This looks fine to me @ry5n and @rexmac! Hover/focus tooltips are still weird for folks on mobile and there's a known issue with how tooltips are read by NVDA, but on desktop this appears to work just as I'd expect for the current implementation.

@ry5n ry5n self-requested a review July 5, 2019 22:04
Copy link
Contributor

@ry5n ry5n left a comment

Choose a reason for hiding this comment

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

@dpersing Thanks! Approved based on your input

@rexmac rexmac force-pushed the 1782-tooltip-mouseenter-fix branch from f01c0d8 to eb75f62 Compare July 8, 2019 13:04
@BPScott BPScott temporarily deployed to polaris-react-pr-1783 July 8, 2019 13:04 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1783 July 8, 2019 14:04 Inactive
Copy link
Member

@AndrewMusgrave AndrewMusgrave left a comment

Choose a reason for hiding this comment

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

Everything looks good 🎉 🎩

it('renders on mouseEnter', () => {
wrapperComponent.simulate('mouseEnter');
it('renders on mouseOver', () => {
wrapperComponent.simulate('mouseOver');
Copy link
Member

Choose a reason for hiding this comment

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

Do we need the extra simulate and expect?

Copy link
Member

Choose a reason for hiding this comment

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

Just saw the commit message was for codecov. Maybe we should break the assertion into its own test and expect mouseOver is called once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya, I was testing to see if it would satisfy codecov, which it did. It definitely feels hacky, but I'm not sure how else to satisfy codecov. I think ideally we'd assert that handleMouseEnter is only called once, but it's a private method.

Maybe we should break the assertion into its own test and expect mouseOver is called once?

I don't follow. Can you elaborate?

Copy link
Member

Choose a reason for hiding this comment

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

Ignore that, I was under the assumption we exposed a mouseEnter handler 😅 These changes may give you codecov

  private handleMouseEnter = () => {
    this.mouseEntered = true;
    this.setState({active: true});
  };

  private handleMouseEnterFix = () => {
    !this.mouseEntered && this.handleMouseEnter();
  };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! I'll give that a shot. 👍

@rexmac rexmac force-pushed the 1782-tooltip-mouseenter-fix branch from 6fc8816 to b91dbe8 Compare July 8, 2019 15:13
@BPScott BPScott temporarily deployed to polaris-react-pr-1783 July 8, 2019 15:13 Inactive
@rexmac rexmac force-pushed the 1782-tooltip-mouseenter-fix branch from b91dbe8 to de9020a Compare July 9, 2019 13:02
@BPScott BPScott temporarily deployed to polaris-react-pr-1783 July 9, 2019 13:02 Inactive
@rexmac rexmac force-pushed the 1782-tooltip-mouseenter-fix branch from de9020a to a017ab4 Compare July 9, 2019 13:04
@BPScott BPScott temporarily deployed to polaris-react-pr-1783 July 9, 2019 13:04 Inactive
@rexmac rexmac force-pushed the 1782-tooltip-mouseenter-fix branch from a017ab4 to c4d432a Compare July 9, 2019 18:23
@BPScott BPScott temporarily deployed to polaris-react-pr-1783 July 9, 2019 18:24 Inactive
@rexmac rexmac merged commit 6365a8d into master Jul 9, 2019
@ghost
Copy link

ghost commented Jul 9, 2019

🎉 Thanks for your contribution to Polaris React!

@kaelig kaelig deleted the 1782-tooltip-mouseenter-fix branch July 12, 2019 21:36
@dleroux dleroux temporarily deployed to production July 16, 2019 16:11 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken and not working as intended in the system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tooltip] Not rendered when entering from a disabled neighbor
6 participants