KingSora / OverlayScrollbars

A javascript scrollbar plugin that hides the native scrollbars, provides custom styleable overlay scrollbars, and preserves the native functionality and feel.
https://kingsora.github.io/OverlayScrollbars
MIT License
3.89k stars 215 forks source link

iOS <=16.1.1 - Scrolling becomes locked when overlayscrollbars is initialized #682

Closed lincolnthree closed 3 weeks ago

lincolnthree commented 1 month ago

Describe the bug When overlay scrollbars are initialized, scrolling is locked even though there is overflow. The same code works in newer iOS/Safari mobile environments and on other platforms, so I am not sure what is happening exactly. I believe it has to do with the measurement of overflow in Safari of this version.

Is there any way to disable scroll locking through the config?

overflowAmount is always 0, hasOverflow is always false: image

Other devices with the same code report overflow correctly: image

I think it may be also related to this change, though I am not super familiar with the internals of OLS: https://github.com/KingSora/OverlayScrollbars/commit/082e275c342eb0b9ebcb3a4c3f773c9b6d63550d#diff-e23943117c0ab8a43c6f67973a7535bceee73babb7404d15118e8ad52df12776R136

To Reproduce

  1. Visit https://www.topdecked.com/news (or other affected screens like /decks or /cards) on an affected iOS device,
  2. Attempt to scroll. It won't work.

Note: https://www.topdecked.com itself works because overlayscrollbars is not initialized on the home screen (same for some other screens.)

Expected behavior Scrolling should work and should not be locked.

Examples

Apologies. It will be difficult to create a sample of this as we have a custom integration and are using Ionic framework, but the bug does not appear to be present in overlayscrollbars 1.8.x

But here is our init code:

    const scrollbars = OverlayScrollbars({
      target: elements.viewport,
      elements: {
        viewport: elements.viewport,
        content: elements.fixed,
        host: elements.content,
        padding: null
      },
      scrollbars: {
        slot: elements.background
      }
    }, {
      overflow: {
        x: 'scroll',
        y: 'scroll'
      },
      update: {
        debounce: 30,
        ignoreMutation: _mutation => {
          return false;
        }
      },
      scrollbars: {
        autoHide: options.utils.platform.isDesktop() ? 'never' : 'scroll',
        autoHideDelay: 1000,
        autoHideSuspend: false,
        dragScroll: true,
        visibility: 'visible',
        clickScroll: true,
        theme: 'os-theme-topdecked'
      },
      showNativeOverlaidScrollbars: false
    }

Environment

Additional context Add any other context about the problem here.

KingSora commented 1 month ago

Good day @lincolnthree :)

I'll take a look as soon as possible.. could you check whether this behavior exists only on iOS or is also present on MacOS Safari?

KingSora commented 1 month ago

@lincolnthree I've looked at it and it seems that its not directly related to OverlayScrollbars since other apps (Demo examples and Docs) are working.

I've digged into your markup and code and analyzed it. First of all I've never really tested the library with ShadowDOM but it seems to work properly.

Then there a few things I noticed regarding your target options:

{
  target: elements.viewport,
  elements: {
    viewport: elements.viewport,
    content: elements.fixed,
    host: elements.content,
    padding: null
  },
  scrollbars: {
    slot: elements.background
  }
}

I would strongly recommend to not set the content element here as this is mostly intended as a backwards compatibility feature for v1 users. If your setup works without it, dont use it. Something similar applies to the host element, as it was intended for initializations of a textarea element, but plans have changed here.. so this value is essentially ignored. I'm also planning to mark both fields as deprecated in future versions.

Then you have the target and viewport element which are the same element.. Thats completely valid and works best if your scrollbars.slot element is a different element (which is the case in your app).. just wanted to mention that.


I've compared the DOM between an iPhone13 / Safari15 and an iPhone15 / Safari17:

Safari15: ![grafik](https://github.com/user-attachments/assets/c8c1c6ce-569c-46f1-99df-49380266ad19)
Safari17: ![grafik](https://github.com/user-attachments/assets/85800d74-0875-4434-8cdb-6dbcc3148d09)

If you compare both and take a look at the box-model on the right you can see that on Safari17 the element is 538px in height and on Safari15 the element is 2193px in height. So on Safari15 the viewport is growing with its content and thus its overflow will always be 0. On Safari17 the viewport is fixed and only its content grows thus its overflow will be the difference between those two.

It seems like in older Safari versions the styling of .inner-scroll isn't working as expected and the bottom: calc(var(--offset-bottom) * -1) style has not the same effect. I was able to fix this issue with the following style change:

.inner-scroll {
  height: calc(100% - var(--offset-bottom) * -1);
  bottom: auto;
}
lincolnthree commented 1 month ago

Hey @KingSora!

First. Wow. I was absolutely stunned to see your response on Friday. It was both incredibly quick AND you already figured out a solution. Seriously impressive for any software these days, even (or especially) open source. Thank you. I would have replied sooner but weekends are tough! Your library is incredibly helpful by the way, so thank you again.

I should have loaded a demo on the iPhone simulator. My apologies for not thinking of that!

Config: Thanks for your feedback about the configuration. I was not entirely sure what all of those parameters did, and since I was working with Shadow DOM/custom init, I just sort of played with them until I found something that worked. It makes sense, though, since it seemed a number of different attempts all worked the same way. I was under the impression that all those parameters needed a value for some reason so I just filled them in and left them if it worked. Happy to know I can simplify that and get a few bytes back. It does indeed work without those defined.

Style issue with .inner-scroll: This is interesting. That's an Ionic library style. I'll have to override that programmatically as it's in the lovely shadow DOM. What I don't understand is why overlayscrollbars blocks scrolling of this element in that scenario. Shouldn't the element still scroll but no scrollbars would show up?

For whatever reason, once overlayscrollbars is initialized, if the content is not long enough to scroll, the scrolling becomes locked, and then when more content is added and overflow occurs, the scroll remains locked until I manually update the overlayscrollbars instance, which I currently do using a combination content observer and a backoff interval to make sure the screen is responsive for users as quickly as possible in a variety of situations.

I had been using throttled scroll events themselves to update the overlayscrollbars instance, but since scrolling is locked initially if content is too short, scroll events would never fire to update the instance and let it know there is more content and to start showing scroll bars. This resulted in a situation where scrolling is permanently locked even if there is enough content.

I've found this scroll locking after init occurs regardless of browser, and it seems that Safari <=16.1.1 issue is the same issue, but appears to always cause this scenario due to the calculations you discovered above. Scrolling works fine until overlayscrollbars is initialized (but obviously there are no scroll bars.)

Thoughts? Thanks again!

lincolnthree commented 1 month ago

Small update. That style does force things into a working state (yay!), but in this case I think we also need to include the --offset-top. It doesn't appear that bottom needs to change.

.inner-scroll {
   height: calc(100% + var(--offset-bottom) + var(--offset-top));
}
KingSora commented 1 month ago

@lincolnthree Thanks for your kind words, I really appreciate them! :) I always try to do my best in terms of support, since I would like to receive it myself as soon as possible, so I'm trying to lead by example.

I just found out that the [data-overlayscrollbars] selector of the library overwrites a style of the .inner-scroll style. Its the position: absolute; style of the .inner-scroll selector, its overwritten by position: relative. This actually happens in all safari versions, but only the older ones will change the height of the element because of that.. So you could also overwrite the position: relative style with position: absolute like this:

/* has higher specificity than the "[data-overlayscrollbars]" selector */
.inner-scroll[data-overlayscrollbars] {
  position: absolute;
}

This should have the same outcome and be closer to the intended style.

Regarding the issue of OverlayScrollbars not updating.. I guess this might be a side effect of the ShadowDOM.. I'm using a MutationObserver to observe any changes of nodes inside of the target element. The problem with your setup is that the content of the target element is just a slot element which then has its content outside of the target element.. so any changes are not picked up and you have to call the update() function manually every time something changes.

I guess thats something I could improve in a future release (possibly with a Plugin), but for now you'll have to continue with your manual updates workaround (or you don't use the shadow dom).

Edit after a little bit of research: It might be really hard to provide something out of the box to correctly observe slots inside of ShadowDOM, because like in your case, those slots have then again nested ShadowDOM slots which then need to observed separately again.. I'll try my best to check whether there is a viable solution, but I can't promise anything in that regard, since crossing ShadowDOM boundaries is something that is hard by design.

lincolnthree commented 1 month ago

@lincolnthree Thanks for your kind words, I really appreciate them! :) I always try to do my best in terms of support, since I would like to receive it myself as soon as possible, so I'm trying to lead by example.

It is appreciated! I try my best with my own libraries but I need more hours in the day.

I just found out that the [data-overlayscrollbars] selector of the library overwrites a style of the .inner-scroll style. Its the position: absolute; style of the .inner-scroll selector, its overwritten by position: relative. This actually happens in all safari versions, but only the older ones will change the height of the element because of that.

Oh. Yeah. Look at that. That would definitely do it. Hah! In addition to Safari being Safari of course. Did that style change in a recent OverlayScrollbars update? Just wondering what happened as this was just revealed when I updated and people started complaining to me.

This should have the same outcome and be closer to the intended style.

It does indeed! Great catch.

It might be really hard to provide something out of the box to correctly observe slots inside of ShadowDOM, because like in your case, those slots have then again nested ShadowDOM slots which then need to observed separately again.. I'll try my best to check whether there is a viable solution, but I can't promise anything in that regard, since crossing ShadowDOM boundaries is something that is hard by design.

Yeah. Agreed. Not sure you should even try, to be honest. Every situation would be different unless you want to maintain plugins for various frameworks... You already provide all of the hooks necessary for people to be able to get handles to their elements and do the init. Maybe add a section to the docs to call out this scenario? Might be the most practical option, thorough explanation of the issue & caveats.

One other thing I found with the Shadow DOM is that I have to programmatically inject the entire OverlayScrollbars stylesheet into a <style> element in the the shadow DOM root element. (In addition to my own OSSB theme styles) Nothing penetrates the Shadow DOM boundary from the main app's stylesheet. Perhaps obvious, but worth noting as a required step, unless there's something I'm missing.

<style>
// .os-size-observer,.os-size-observer-listener{box-sizing:border-........ rest of stylesheet

  .os-theme-topdecked {
  --os-handle-bg: rgba(var(--ion-color-primary-rgb), 0.44);
  --os-handle-bg-hover: var(--ion-color-primary);
  --os-handle-bg-active: var(--ion-color-primary-shade);

  --os-size:8px;
  --os-padding-perpendicular:2px;
  --os-padding-axis:2px;
  --os-track-border-radius:4px;
  --os-handle-interactive-area-offset:4px;
  --os-handle-border-radius:0px;
  box-sizing:border-box;

  z-index: 10000;
</style>

EDIT: Actually, a config option for injecting the stylesheet into an element might be useful. Right now I have to do that by copying the entire OverlayScrollbars stylesheet into my code, and update it whenever you update yours. Made a note in the upgrade checklist. I tried doing this dynamically but got into webpack/tyepscript nightmares and gave up as copying works means I can keep working... "don't look at me like that!" lol

KingSora commented 1 month ago

Did that style change in a recent OverlayScrollbars update? Just wondering what happened as this was just revealed when I updated and people started complaining to me.

The style itself didn't change, but the specificity of the selector did.. The reasoning was to better support frameworks like react, vue and angular etc. because they have their own ways of updating and setting class names which would overwrite the libraries styles I've decided to not use class names anymore and move to attributes. Attributes have a higher specificity so the previous selector was .os-target and the new one is [data-overlayscrollbars="something"].

I'll take a look what utils / plugins I could provide in order to better support ShadowDOM (such as injecting styles, observing elements etc.).

Related: https://github.com/KingSora/OverlayScrollbars/issues/402

lincolnthree commented 1 month ago

Makes sense. Thank you again :) I'll keep tabs on that issue!

EDIT: Just kidding I commented on it last year before I figured all this out.

KingSora commented 3 weeks ago

I'm closing this issue since your specific case should be resolved.. The "Shadow-DOM support" issue will keep track of that specific feature

lincolnthree commented 3 weeks ago

Perfect, thanks!