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

sl-tooltip Element's Popup In Beta 80 Being Covered By Other Elements #854

Closed
Zozman opened this issue Aug 12, 2022 · 5 comments
Closed
Assignees
Labels
bug Things that aren't working right in the library.

Comments

@Zozman
Copy link

Zozman commented Aug 12, 2022

Describe the bug

Inside of beta 80 after the sl-tooltip refactor, the popup part of the tooltip is not always appearing over the rest of the content in a page. In a variety of scenarios it is appearing under other content.

Upon looking into it, it seems that the popup part of the sl-tooltip has no z-index set. In addition, the body of the sl-popup inside the sl-tooltip does have a z-index set, but it's not enough to get it in front of other content.

To Reproduce

Steps to reproduce the behavior:

  1. Create a sl-tooltip element
  2. Create another shoelace element such as a sl-card that will appear where the popup would appear
  3. Observe it's covered up.

Demo

https://codepen.io/zozman/pen/bGvxvGm?editors=1100

Screenshots

Current behavior
unknown

Missing z-index in the popup part of sl-tooltip
unknown (1)

z-index value inside the body part of the sl-popup
unknown (2)

Browser / OS

  • OS: Mac
  • Browser: Chrome
  • Browser version: 104.0.5112.79
  • Shoelace Version: 2.0.0-beta.80
@Zozman Zozman added the bug Things that aren't working right in the library. label Aug 12, 2022
@CetinSert
Copy link

CetinSert commented Aug 13, 2022

This also causes a regression where we were using <sl-tooltip> on the handle of a <sl-split-panel> before.

Please see:

✔️ https://rt.ht/y4 (.78) (Edge 104 on Windows 11)
image

@CetinSert
Copy link

CetinSert commented Aug 13, 2022

Further investigation revealed the two issues with .80:

  1. z-index
  2. mis-positioning

https://rt.ht/y4x/#se=oi (.80) (switching panels to reveal mis-positioned, low z-index the tooltip)
image

@CetinSert
Copy link

CetinSert commented Aug 14, 2022

Improved positioning by refactoring the host page DOM tree.

✔️ .79
.80

<sl-tooltip   id=st slot=handle placement=left hoist>
        <style></style>
        <flex slot=content t-a=r style=max-width:max-content;padding-block:.4rem></flex>
        <span id=sh style=pointer-events:none;opacity:0 aria-hidden=true>x</span>
      </sl-tooltip>

✔️ .79
✔️ .80

<sl-tooltip   id=st slot=handle placement=left hoist>
        <flex slot=content t-a=r style=max-width:max-content;padding-block:.4rem>
          <style></style></flex>
        <span id=sh style=pointer-events:none;opacity:0 aria-hidden=true>x</span>
      </sl-tooltip>

Can we have a replacement for <sl-tooltip>.updatePositioner() or better yet the same function re-exposed?
We were actively using it to make the tooltip track the position of the panel handle.
Watch this GIF to see what we mean: ✔️ .79 (thanks to <sl-tooltip>.updatePositioner())
Sync𝗛𝗧𝗠𝗟-•-Instant-Web-Playground-–-Edit-View-Live-


Discovered and switched to <sl-tooltip>.popup.reposition()d14b9e1#diff-a7ad7628e7dd91c0aa178970c52f005b888266b71b1f09f440f1ecb8ec4e97adR120


SyncHTML.io is back in good shape now! Thanks a lot everyone ^o^!

  • fix initial position (move <style> further in; was placed there for proximity when editing)
  • fix repositioning (use the new <sl-tooltip>.popup.reposition())
  • fix z-index (issue disappeared by itself o__O after the first two)

Just a quick note, repositioning performance is slightly worse and position tracking is stuttery in .80 (much smoother in .79).

Performing the gif in this comment with

(This is very likely a non-issue, just sharing here fyi.)

@claviska
Copy link
Member

Good catch. The z-index should have been applied to the popup, but it was still being applied to the tooltip's body. Fixed in 2372309.

Discovered and switched to .popup.reposition()

This is a much better approach than relying on tooltip.updatePositioner(), which was private.

@CetinSert
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Things that aren't working right in the library.
Projects
None yet
Development

No branches or pull requests

3 participants