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

fix(ToolTip): better toggling logic #885

Merged
merged 3 commits into from
Nov 7, 2021
Merged

fix(ToolTip): better toggling logic #885

merged 3 commits into from
Nov 7, 2021

Conversation

malinowskip
Copy link
Contributor

Proposal to fix #872

@metonym
Copy link
Collaborator

metonym commented Nov 1, 2021

Thank you for taking the time. I'm still encountering the following behavior.

Clicking the button once will double trigger the tooltip – it will open and close it in fast succession.

carbon-tooltip.mov

@malinowskip
Copy link
Contributor Author

malinowskip commented Nov 1, 2021

Thanks for reviewing. How can I open that example? I don’t know what the route should be.

This example from the documentation works fine for me:

Zrzut ekranu 2021-11-1 o 23 19 38

@metonym
Copy link
Collaborator

metonym commented Nov 1, 2021

Thanks for reviewing. How can I open that example? I don’t know what the route should be.

The route should be http://localhost:3000/framed/Tooltip/TooltipReactive

I tested it with the previous code (before the commit that modified the example):

<!-- TooltipReactive.svelte -->
<script>
  import { Tooltip, Button } from "carbon-components-svelte";
  let open = true;
</script>

<style>
  div {
    margin-top: var(--cds-spacing-05);
  }
</style>

<Tooltip bind:open triggerText="Resource list" align="start">
  <p>Resources are provisioned based on your account's organization.</p>
</Tooltip>

<div style="margin-top: var(--cds-spacing-12);">
  <Button size="small" on:click="{() => (open = !open)}">
    {open ? 'Close tooltip' : 'Open tooltip'}
  </Button>
</div>

<div>Open: {open}</div>

@malinowskip
Copy link
Contributor Author

Okay, I think I get it now. The "Open tooltip" button opens the tooltip, but it still triggers this mousedown handler in the ToolTip component:

<svelte:window
  on:mousedown="{({ target }) => {
    if (open) {
      if (target.contains(refTooltip)) {
        if (refIcon) {
          refIcon.focus();
        } else if (ref) {
          ref.focus();
        }
      }

      if (!ref.contains(target)) open = false;
    }
  }}"
/>

@malinowskip
Copy link
Contributor Author

Removing this line seems to fix the issue without breaking anything.

@metonym
Copy link
Collaborator

metonym commented Nov 2, 2021

Unfortunately, it reintroduces this issue where multiple tooltips can be open at the same time: #869

Repro:

<script>
  import { Tooltip } from "carbon-components-svelte";

  let open = true;
</script>

<style>
  .wrapper {
    display: flex;
    justify-content: center;
  }
  .trigger {
    padding: 10px;
    margin: 10px;
    border: 1px solid black;
  }
</style>

<div class="wrapper">
  <Tooltip hideIcon>
    <div slot="triggerText" class="trigger">TEST1</div>
    <div>TOOLTIP1</div>
  </Tooltip>
  <Tooltip hideIcon>
    <div slot="triggerText" class="trigger">TEST2</div>
    <div>TOOLTIP2</div>
  </Tooltip>
</div>

<ol style="margin-top: 100px; margin-left: 20px;">
  Error description:
  <li>1. Click on TEST1</li>
  <li>2. Click on TOOLTIP1</li>
  <li>3. Click on TEST2.</li>
  Result: Tooltip1 and Tooltip2 are both open
</ol>

@malinowskip
Copy link
Contributor Author

Thank you for reviewing. I tried a bunch of different things, but it seems like each fix introduces a regression one way or another – in most cases due to event handlers firing at different times, doing conflicting things. For example, in the reactive example, the mousedown event on the button closes the tooltip, so the “Close tooltip” button becomes an “Open tooltip” button mid-click.

The only thing I can think of at this point is to use a store to ensure that only one ToolTip is open at any given time, but that might be overkill if there’s a simpler solution.

- a click event closes the tooltip if the target is not the tooltip or
  the tooltip icon.
- the event is registered in the capture phase and a `setTimeout` is used to
  deal with edge cases in which a button is used to toggle the tooltip
  on or off.
@malinowskip
Copy link
Contributor Author

malinowskip commented Nov 6, 2021

I have tested this with all the examples in the docs, the reactive example, as well as the “multiple tooltips” example, in Firefox and Chrome. To deal with the race conditions, I registered the new click event in the capture phase, and used a timeout. @metonym when you get a chance, can you have a look? I hope I didn’t miss anything obvious this time.

There is an unrelated minor issue where if you open a tooltip, click on it, and then tab out, then it remains open.

@metonym metonym self-requested a review November 6, 2021 18:55
Copy link
Collaborator

@metonym metonym left a comment

Choose a reason for hiding this comment

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

Well done!

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.

[Regression] Reactive tooltip example doesn't work
2 participants