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

[Bug]: body scroll remains locked if open dialog from another dialog using v-if #474

Closed
enkot opened this issue Oct 27, 2023 · 9 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@enkot
Copy link
Contributor

enkot commented Oct 27, 2023

Environment

Node version: 16.17.0
Package manager: yarn@1.22.19
Radix Vue version: 1.0.1
Vue version: 3.3.4
Browser: Chrome 118.0.5993.88 (Official Build) (arm64)

Link to minimal reproduction

https://codesandbox.io/p/sandbox/radix-vue-select-forked-4yf8xy?file=%2Fsrc%2FApp.vue%3A1%2C1

Steps to reproduce

  1. Click "Open A"
  2. Click "Open B"
  3. Click "Open A"
  4. Close on Esc or click on overlay

Describe the bug

Found another bug with body scroll :) My use case is not very common, I need to open/close dialogs using v-if (in real project it is handled by createTemplatePromise).
Body scroll remains locked if open dialog B from another dialog A, then open again dialog A from dialog B. overflow: hidden; remains on body after closing the last dialog (B).

Expected behavior

Page scroll works

Conext & Screenshots (if applicable)

Partially it is a regression from my previous fix #456 🙈 , but I think it should work in all scenarios anyway:)

@enkot enkot added the bug Something isn't working label Oct 27, 2023
@enkot
Copy link
Contributor Author

enkot commented Oct 27, 2023

Small observation
With this changes it works:

DialogOverlay.vue

const rootContext = injectDialogRootContext()

- const isLocked = useBodyScrollLock()
+ const isLocked = useBodyScrollLock(rootContext.open.value)
watch(
  rootContext.open,
  (isOpen) => {
    isLocked.value = isOpen
  },
- { immediate: true },
)

useBodyScrollLock.ts

if (stack.value === 0) {
  document.body.style.paddingRight = ''
  document.body.style.marginRight = ''
  document.body.style.pointerEvents = ''
  document.body.style.removeProperty('--scrollbar-width')
+ nextTick(() => {
+   document.body.style.overflow = ''
+ })
}

Looks like useScrollLock is setting initialOverflow based on overflow set by previous dialog, but not sure.

@enkot
Copy link
Contributor Author

enkot commented Nov 1, 2023

@zernonia Could you please share your thoughts? I'm not sure if this should be handled by radix-vue or in userland.

@zernonia
Copy link
Collaborator

zernonia commented Nov 1, 2023

Thanks for the issue @enkot . I just tested the link above and cant seems to reproduce it 🤔

Screen.Recording.2023-11-01.at.9.37.55.PM.mov

@enkot
Copy link
Contributor Author

enkot commented Nov 1, 2023

@zernonia Sorry, the repro was incorrect, updated it. Could you please check one more time?)

@zernonia zernonia self-assigned this Nov 2, 2023
@zernonia
Copy link
Collaborator

zernonia commented Nov 3, 2023

I've spend some time looking into this.. I believe it's due to useScrollLock internally checking the initialOverflow, and because the quick succession of lock/unlock, it captures the wrong initialOverflow.

I've tried using nextTick and it seems to work fine.

  dialogBOpen.value = false
  nextTick(() => {
    dialogAOpen.value = true
  })

It's abit challenging to tackle 😅

@enkot
Copy link
Contributor Author

enkot commented Nov 3, 2023

Yep, already using nextTick, so my observation was correct 😄
Sad, but as you said, it is really challenging to properly handle it in radix-vue and make some general solution, because we should always set initial overflow to body, which is done by useScrollLock, but we don't know if previous dialog already removed overflow from body.

@enkot enkot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 3, 2023
@enkot
Copy link
Contributor Author

enkot commented Nov 22, 2023

@zernonia do you plan to upgrade VueUse to ^10.6.0?
Looks like this PR vueuse/vueuse#3527 can potentially fix this problem 🙂

@zernonia
Copy link
Collaborator

I did a refactor here #516, (which drop the useScrollLock from Vueuse.) forgot to link this ticket to that.

Could you check if you use case is fixed too? 😁

@enkot
Copy link
Contributor Author

enkot commented Nov 30, 2023

@zernonia Yes, now it works! Thank you 💚

@enkot enkot reopened this Dec 7, 2023
@enkot enkot closed this as completed Dec 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants