Update from v0.9.1 to v0.9.3 CSS bug

[vartdalen]

"bug" 1:
Causes "background: black" to be applied to body by JavaScript (not with className) on Drawer open.

Applying black to background must be reverted or at least should be opt in.

bug 2:
Causes "pointer events: none" to be applied to by JavaScript (not with className) on Drawer open.

If I open the Drawer and quickly close it by clicking on the background right away, pointer events: none is not removed from body, rendering the app uninteractive until page refresh or manual disabling of the CSS modifier.

I have reverted to v0.9.1 until further notice.

This is a great library, much love from a big fan

[emilkowalski]

I'm unable to reproduce both of the bugs. Can you record a video and post it here so I can see exactly what you are doing in order to repro it?

[vartdalen]

[vartdalen]

Here is from working code in v0.9.1, it does not have background black and it does never allow pointer css to remain on body after closing.

[vartdalen]

Here I have reproduced bug 1 in v0.9.3.

Notice that the background: black css modifier is overriding my background image shown in previous images where it is only dimmed. It is not good that your library overrides the background on body like this.

[vartdalen]

Here I have reproduced bug 2,

I have opened and closed the drawer very quickly. It does not happen every time and is sometimes hard to reproduce.
It never happens on v0.9.1.

As you can see, pointer-events: none remains on body, even though my drawer is closed, rendering the app uninteractive until page reload

[vartdalen]

I hope you revert the changes please!

[hedbladucf]

I have the exact same issue. It drove me crazy for a few minutes thinking I had some other issue as my page seemed unresponsive! This a quite a nasty little bug, please fix asap. I reverted back and I no longer see the issue.

[emilkowalski]

Can you provide a demo with reproduction? The exact props you are using etc.

The black background won't be added unless you have shouldScaleBackground to create an iOS-like effect.

This codesandbox runs on 0.9.3, it's the default setup. https://codesandbox.io/p/devbox/drawer-with-scale-forked-nx2glp

Are you able to reproduce it there?

[maiconcarraro]

@emilkowalski

Code sandbox using v0.9.0

Code sandbox using v0.9.3

The condition to this to happen is:

• Remove vaul-drawer-wrapper or data-vaul-drawer-wrapper from your document
• Set shouldScaleBackground to true

You can spot the black background between contents on the >>> example

I know it's confusing, it was a "hidden" bug for people using shadcn for example that already had this prop true and maybe no reference to the wrapper, and now using the new version it makes the bug visible.

It's a combination of misconfiguration that leads to a bug.

[emilkowalski]

@maiconcarraro Thank you so much for this reproductions and explanations.

I changed vaul-drawer-wrapper to data-vaul-drawer-wrapper in version 0.9.3, so people that updated are probably still using the old attribute. I made it backwards compatible in this PR #412, will make sure to release a new version later today and close this issue when it's released.

Once again thanks @maiconcarraro for this great explanation.

[maiconcarraro]

@emilkowalski for this specific bug your change related to vaul-drawer-wrapper -> data-vaul-drawer-wrapper makes no difference.

The problem is the difference for this logic (from your PR: https://github.com/emilkowalski/vaul/pull/409/files)

Having no wrapper would return early and apply no styling, whereas the new version applies style even if wrapper is null.

[maiconcarraro]

I don't think it's really a problem of vaul 0.9.3, the real problem is the misconfiguration using shouldScaleBackground+ no wrapper which is a conflict. It does have side effects now, but it's mainly a configuration issue.

[emilkowalski]

I agree that this is a configuration issue. Consequences of this misconfiguration where avoided before, because we returned early if there was no wrapper even if shouldScaleBackground was true. This PR brings back the early return. @maiconcarraro Does this PR make sense to you? Want to double-check before I merge.

[maiconcarraro]

@emilkowalski same page ✅

[vartdalen]

@emilkowalski for this specific bug your change related to vaul-drawer-wrapper -> data-vaul-drawer-wrapper makes no difference.

The problem is the difference for this logic (from your PR: https://github.com/emilkowalski/vaul/pull/409/files)

Having no wrapper would return early and apply no styling, whereas the new version applies style even if wrapper is null.

Could the bug 2 (issue of failing to remove the modifier on drawer close) be attributed to the use of React.useEffect()?

[emilkowalski]

of failing to remove the modifier on drawer

What useEffect? I'd say change the data attribute and see if the problem persists.

[vartdalen]

of failing to remove the modifier on drawer

What useEffect? I'd say change the data attribute and see if the problem persists.

You are setting background: black inside React.useEffect() inside the code highlighted in green.

If this is new, meaning you did not set the CCS modifier inside React.UseEffect() before, but you are now, in v0.9.3, I suggest that this is the cause for bug 2.

[maiconcarraro]

@vartdalen this library has no directly relation to the "pointer-events: none" logic, this is set by radix dialog and this bug might happen if you are using different versions of radix, you just try to follow up there, example of a very similar issue there: [Dialog] body pointer-events: none remains after closing #1241

[vartdalen]

@vartdalen this library has no directly relation to the "pointer-events: none" logic, this is set by radix dialog and this bug might happen if you are using different versions of radix, you just try to follow up there, example of a very similar issue there: [Dialog] body pointer-events: none remains after closing #1241

The depicted reproduction of bug 2 does not involve bumping radix dialog, only vaul from v0.9.1 to v0.9.3

[vartdalen]

I underline that in v0.9.1, opening and closing the drawer is not only avoiding the pointer event bug. It is also snappier and more reactive to quickly opening and closing the drawer in succession.

[emilkowalski]

@vartdalen Again, please provide a codesandbox demo with reproduction. I don't know what your exact setup is and this is critical for me to debug it.

[maiconcarraro]

the only thing that could be related to this is this specific bump to radix dialog version: https://github.com/emilkowalski/vaul/pull/408/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519L66

so depending on what you have in your project you may have conflicts if you are not enforcing the same version, in my project I also have (using this since vaul 0.9.0 to solve a problem w/ cmdk and other libs)

@vartdalen can you reproduce using the sandbox I sent few messages above?

[sshmaxime]

@vartdalen to remove the weird black background, add setBackgroundColorOnScale to the root element.

cc @emilkowalski #431

[navid-az]

can confirm. adding setBackgroundColorOnScale to the root element will in fact fix unexpected black background when Drawer is open

[emilkowalski]

I've updated Vaul's version to 0.9.4 where you can keep vaul-drawer-wrapper instead of changing it to data-vaul-drawer-wrapper. This was what caused the issue on 0.9.3. Waiting on a repro demo for pointer events before closing this one.

[hedbladucf]

Thanks for updating, Emil. Cheers!

[vartdalen]

2024-09-26_10-05-13-2.mp4

@emilkowalski

[emilkowalski]

2024-09-26_10-05-13-2.mp4
@emilkowalski

This is on purpose. It is now using CSS animations instead of CSS transitions and pointer events none are set on the body until the drawer fully closes, that's how Radix's Dialog work.