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-dropdown> content overflow regression introduced in 2.0.0-beta.80 #856

Closed
CetinSert opened this issue Aug 16, 2022 · 13 comments
Closed
Assignees
Labels
bug Things that aren't working right in the library. needs response Waiting for a response or feedback from the user.

Comments

@CetinSert
Copy link

CetinSert commented Aug 16, 2022

Describe the bug

  • ✔️ 0.78 <sl-dropdown> ... <div part=base> ... <div part=panel style="max-width: #px; max-height: #px">
    ✔️ 0.78 → overflows are nicely contained.
  • 0.80 <sl-dropdown> ... <sl-popup part=base> ... <div part=panel> !! no max-width, max-height.
    0.80 → overflows are not contained.

To Reproduce

Steps to reproduce the behavior:

  1. Go to the 2 × //rt.ht/.. permalinks given below in the table header
  2. Press Ctrl + + several times to zoom in
  3. Press Alt + F to open the Sync menu
  4. Press to focus the last <sl-menu-item>
  5. Notice the overflow regression introduced in .80
✔️ .78 //rt.ht/y4 .80 //rt.ht/y6
image image
image image

Demo

See the links above.

Screenshots

See the screenshots above.

Browser / OS

  • OS: all
  • Browser: all
  • Browser version: all

Additional information

This is a regression in .80. The old behavior is preferred.

@CetinSert CetinSert added the bug Things that aren't working right in the library. label Aug 16, 2022
@CetinSert CetinSert changed the title <sl-dropdown> content overflow regression introduced in 2.0.0-beta.80 <sl-dropdown> content overflow regression introduced in 2.0.0-beta.80 Aug 16, 2022
@claviska
Copy link
Member

I believe this has been resolved per #854, as the tooltip was not correctly setting a z-index after the popup refactor.

I'll be releasing beta.81 shortly, so if the issue persists, please open a new issue and reduce your test case so I can better understand what's happening.

@CetinSert
Copy link
Author

CetinSert commented Aug 17, 2022

@claviska – this is independent from z-index.

Here a dropdown with long contents makes the page viewport larger!
image

Earlier max-height, max-width was explicitly calculated in the library preventing this and overflow, thus scrolling was contained inside the dropdown itself.


The steps to reproduce, along with the screenshots is as clear as I can get here. Please look at the Sync menu inside the dropdown and how it differs between .79 and .80 when we zoom into make it not fit into the viewport.

@CetinSert
Copy link
Author

@claviska – I have marked the issue on the screenshot.

@claviska
Copy link
Member

I see that, but I can't replicate it outside of your environment. Both popup and dropdown are working as expected for me. I'm specifically testing dropdowns with lots of menu items so they exceed the viewport, and the auto-size feature of popup appears to be working fine.

CleanShot 2022-08-17 at 09 18 49

I'm not going to dig into your custom code to make sure it is a problem with the library. It very well might be, but I don't have the time to weed through it for you. A minimal repro will make the problem very obvious and it will give me a way to more accurately observe and verify a fix for the problem.

Thanks!

Update: I just noticed you filed #859 with the same screenshot as above. That's not very helpful. I'll reopen this and wait for a better repro (one that I can actually verify and test) and close that one so we'll have this chat history available.

@claviska claviska reopened this Aug 17, 2022
@CetinSert
Copy link
Author

@claviska – thank you for the prompt response! I will let you know if it ever re-appears. Currently I have applied a quick fix myself and I also don't have more time to take a deeper look.

@claviska claviska added the needs response Waiting for a response or feedback from the user. label Aug 17, 2022
@claviska
Copy link
Member

OK, should I close this then? There are some updates that you may not be testing with that may very well solve it, but if you can't be bothered to make a test case then this issue is going to sit and rot.

@CetinSert
Copy link
Author

Let's not make it rot. I will close this for now then.

@CetinSert
Copy link
Author

CetinSert commented Aug 17, 2022

Actually I could reproduce it minimally between .79 and .80.

  1. Visit this link: https://rt.ht/⋯
  2. Make the white output part smaller than the menu
  3. ✔️ .79 → scrollbar in dropdown; ❌ .80 → spills over and makes the body overflow!!

@CetinSert CetinSert reopened this Aug 17, 2022
@CetinSert
Copy link
Author

CetinSert commented Aug 17, 2022

✔️ .79

image


vs


.80

image

@CetinSert
Copy link
Author

CetinSert commented Aug 17, 2022

Manually selecting the <sl-popup> and applying ($0.autoSize = true; ($0.autoSize was false)) in the shadow root of the <sl-dropdown> does not help either.

@claviska
Copy link
Member

Since beta.81 was just released, can you make sure to update to that version? Thanks!

@CetinSert
Copy link
Author

Yeah, one second.

@CetinSert
Copy link
Author

✔️ .81 – yahoo! It is fixed.
image

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. needs response Waiting for a response or feedback from the user.
Projects
None yet
Development

No branches or pull requests

2 participants