search

huntabyte / shadcn-svelte

shadcn/ui, but for Svelte. ✨
https://shadcn-svelte.com
MIT License
5.04k stars 310 forks source link

bug: Dialog overlay overlaps Combobox #534

Open yb-cs opened 9 months ago

yb-cs commented 9 months ago

Current Behavior

When I add "" to the combobox to make it scrollable it causes the text to become blurry and low resolution. However, in the case when it does not scroll the text is fine.

Expected Behavior

Same text style when the area is not scrollable.

Steps To Reproduce

  1. Use combobox
  2. Use options large enough to scroll
  3. Notice the blur / degraded text
  4. Please check the image attatched. combobox_scroll_blur

Link to Reproduction / Stackblitz (reproduction template)

No response

More Information

No response

yb-cs commented 9 months ago

Forgot to mention it was used inside a dialog. It could be the blur styling leaking in? I am not sure...

huntabyte commented 9 months ago

If you increase the z-index to something like 50 of the Content does the issue persist?

yb-cs commented 9 months ago

This is interesting I added the following "" initially it is not blurry but once you scroll it becomes blurry again.

yb-cs commented 9 months ago

1 2 See the difference before and after scrolling.

huntabyte commented 9 months ago

Add it to the Command.List

yb-cs commented 9 months ago

it is actually worse than group. Somehow group fixed it before scrolling but this is blurry in both cases: 3 4

and
yb-cs commented 9 months ago

I figured out the issue. It is 100% because using it with dialog. Probably a css class contamination or something of that sort since if I do it on a normal page it is not blury, however the dialog has the blur effect and it is causing this.

yb-cs commented 9 months ago

How would you advice on this? is it because in a list they are not in focus and it causes the blur effect to bleed in?

yb-cs commented 9 months ago

@huntabyte could you advice how to proceed further with this? should I avoid using the dialog with combobox or is there a way to prevent the style bleeding for the blur effect?

Thanks in advance :)

huntabyte commented 7 months ago

Related: https://github.com/shadcn-ui/ui/issues/2363 https://github.com/shadcn-ui/ui/pull/2638

slinso commented 5 months ago

I looked at a few different Dialog implementations and it seems others are using (IMO) a simpler approach to the centering/placing of the dialog.

Instead of:

<Dialog.Portal>
    <Dialog.Overlay />
    <DialogPrimitive.Content class="fixed translate..." />
</Dialog.Portal>

use this and get rid of the translate css:

<Dialog.Portal>
    <Dialog.Overlay class="fixed flex justify-center items-center>
        <DialogPrimitive.Content />
    </Dialog.Overlay>
</Dialog.Portal>

I tried the simplest solution and just added a slot to the Dialog.Overlay, but melt/bits does not work with with nested markup. I think a change in bits-ui could make that work.

@huntabyte What do you think? Is it worth investing more time into this approach?

huntabyte commented 5 months ago

@huntabyte What do you think? Is it worth investing more time into this approach?

I believe this would mess with how transitions happen since the overlay and content should kick off their transitions at the same time. This feels more like a CSS styling issue than anything else tbh.

GimpMaster commented 5 months ago

Not sure if it helps much but I found out that in web browsers the blurryness in selects is present.

However in my capacitor app running on Safari it’s gone. Kind of strange.

slinso commented 5 months ago

Ok, it is some kind of CSS issue, but not really a simple one to solve. If you look around stackedoverflow there are a few questions regarding this issue (e.g. SO: Blurry Text

Here is a good explanation and a possible fix: GPU Text Rendering in Webkit. So for dialogs which have the maximum width we could just change "sm:max-w[425px]" to something even like "sm:max-w[426px]".

But my main concern is, that this problem is only introduced by using the transform function translate to center the dialog. I tried playing around with anti-aliasing and other approaches I found on stackedoverflow, but the problem still persists. I could get a hacky solution working on my machine, but rendering on a different machine/screen size/windowed mode still raises this issue.

So I took a look a the melt-ui examples und bits-ui code. I exaggerated the transition effect in all examples. In melt-ui the fix is pretty simple. Change

<div use:melt={$overlay} 
  class="fixed inset-0 z-50 bg-black/50" 
  transition:fade={{ duration: 500 }} />
<div
    class="fixed left-1/2 top-1/2 -translate-x-1/2 -translate-y-1/2
        z-50 max-h-[85vh] w-[90vw] max-w-[450px] rounded-xl bg-white p-6 shadow-lg"
    transition:flyAndScale={{ duration: 1000, y: 40, start: 0.96 }}
    use:melt={$content}>
    ...
</div>

to

<div use:melt={$overlay} 
  class="fixed inset-0 z-50 bg-black/50 flex items-center justify-center" 
  transition:fade={{ duration: 500 }}>
        <div
            class="z-50 max-h-[85vh] w-[90vw] max-w-[450px] rounded-xl bg-white p-6 shadow-lg"
            transition:flyAndScale={{ duration: 1000, y: 40, start: 0.96 }}
            use:melt={$content}>
            ...
        </div>
</div>

The transitions will be triggered at the same time, because they are not nested in different components. So for bits-ui I had to make to Dialog.Content transition global and I added a default slot to every if branch other than the "asChild" branch. Like this:

dialog-content.svelte:

        transition:transition|global={transitionConfig}

dialog-overlay.svelte:

    <div on:mouseup bind:this={el} transition:transition={transitionConfig} use:melt={builder} {...$$restProps}>
        <slot />
    </div>

Then I used the code with the nested Dialog.Content from my https://github.com/huntabyte/shadcn-svelte/issues/534#issuecomment-2082412655 to use flex for centering the dialog.

This issue is not really shadcn/svelte specific, but rather a CSS issue that is present the dialog examples for melt-ui, which was then reused in the other 2 projects. IMO it is best to get rid of the transform functions.

I could write proper pull requests for each project, if you want me to. The question would just be if this fix should be applied to all 3 projects?

Related: https://github.com/huntabyte/cmdk-sv/issues/54

slinso commented 5 months ago

I found a really old (2018) Ember issue regarding this topic. The solution is always the same: use flex (or grid) layout to solve rendering bugs (or work around rendering behavior).

@huntabyte, if you do not want to apply the change in all three projects, would you be fine with a pull request for bits-ui, so I can apply my fix locally? Without that fix, I would need to maintain a copy of bits-ui, which is obviously not my intention.

david-plugge commented 5 months ago

Is there a workaround that i can use today until this gets fixed?

slinso commented 4 months ago

Hey @david-plugge I just submitted the PR for the necessary changes to bits-ui. You can of course use my fork, but I would advise against that, as I do not plan to keep the fork up to date. I hope the PR gets merged.

You would have to make these additional changes to your local shadcn code:

dialog-content.svelte:

dialog-overlay.svelte:

david-plugge commented 4 months ago

My workaround looks like this now:

<script lang="ts">
    import { Dialog as DialogPrimitive, builderActions, getAttrs } from 'bits-ui';
    import * as Dialog from '.';
    import { cn, flyAndScale } from '$lib/utils';
    import { X } from 'lucide-svelte';

    type $$Props = DialogPrimitive.ContentProps;

    let className: $$Props['class'] = undefined;
    export let transition: $$Props['transition'] = flyAndScale;
    export let transitionConfig: $$Props['transitionConfig'] = {
        duration: 200,
    };
    export { className as class };

    $: _transition = transition || flyAndScale;
</script>

<Dialog.Portal>
    <Dialog.Overlay />

    <DialogPrimitive.Content asChild let:builder>
        <div class="fixed inset-0 z-50 grid place-items-center">
            <div
                transition:_transition={transitionConfig}
                use:builderActions={{ builders: [builder] }}
                {...getAttrs([builder])}
                {...$$restProps}
                class={cn(
                    'grid w-full max-w-lg gap-4 border bg-background p-6 shadow-lg sm:rounded-lg md:w-full',
                    className,
                )}
            >
                <slot />

                <DialogPrimitive.Close
                    class="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground"
                >
                    <X class="h-4 w-4" />
                    <span class="sr-only">Close</span>
                </DialogPrimitive.Close>
            </div>
        </div>
    </DialogPrimitive.Content>
</Dialog.Portal>
GimpMaster commented 4 months ago

@david-plugge - Does your work around require changes to bits-ui like @slinso mentioned?

slinso commented 4 months ago

@david-plugge clickOutside to close the dialog is not working for me. Otherwise a nice solution as well. I haven't looked at what is missing. I'll invest a little more time into my approach.

david-plugge commented 4 months ago

@GimpMaster no, you only need to change dialog-content.svelte from shadcn-svelte @slinso I updated my example so it works with clicking outside.

Dart353 commented 4 months ago

I made some adjustments to @david-plugge solution to align it with our code style. However, when I added the border-radius to <DialogPrimitive.Content />, I noticed that it caused blurring issues with the select options.

To resolve this, I used <DialogPrimitive.Content /> as a wrapper to handle positioning. Then, I added a single child <div> inside it and passed the props, slots, and styling to it - just like @david-plugge did.

This approach solved the blurring issues with the select.

My code: 

<Dialog.Portal>
    <Dialog.Overlay />
    <DialogPrimitive.Content
        {transition}
        {transitionConfig}
        class={cn(
            small ? 'max-w-[343px]' : 'max-w-[690px]',
            'fixed inset-0 left-[50%] top-[50%] z-50 grid h-fit max-h-[80%] w-full translate-x-[-50%] translate-y-[-50%] place-items-center ',
            className
        )}
    >
        <div
            {...$$restProps}
            class={cn(
                'bg-background shadow-lg md:w-full grid w-full max-w-lg gap-4 rounded-16 bg-background-surface p-16',
                className
            )}
        >
            <slot />
        </div>
    </DialogPrimitive.Content>
</Dialog.Portal>

image

slinso commented 4 months ago

Thanks again @david-plugge for your workaround. I had some issues with my nested-dialog-content approach, so I switched to your approach and it just works. I closed my pull request and want to leave this topic behind.

@Dart353 You are still using the translate CSS functions, which david removed in his code. As long, as you are using translate... a user of your app may have blurry text. It just depends on the actual pixel sizes at which the dialog is rendered.

wesharper commented 4 months ago

I've searched around and I think I found the root cause.

The TL;DR is that the font is not aligned with the pixel grid, causing webkit to render it strangely when antialiasing. Translating in either x or y direction can cause this misalignment depending on the size of the thing being translated.

My solution is similar to those above, and uses flex for centering. It only requires editing the shad-cn files and touches no other libraries.

  1. Render DialogPrimitive.Overlay as a child div that centers its children and allows for passing of a slot.
  2. Wrap DialogPrimitive.Content in the new DialogOverlay.
  3. Add 'relative' class to DialogPrimitive.Content to ensure the close button gets put in the right spot.
  4. Update transitions for dialog-overlay.

dialog-content.svelte

<script lang="ts">
  import { Dialog as DialogPrimitive } from 'bits-ui';
  import Cross2 from 'svelte-radix/Cross2.svelte';
  import * as Dialog from './index.js';
  import { cn, flyAndScale } from '$lib/utils.js';

  type $$Props = DialogPrimitive.ContentProps;

  let className: $$Props['class'] = undefined;
  export let transition: $$Props['transition'] = flyAndScale;
  export let transitionConfig: $$Props['transitionConfig'] = {
    duration: 200
  };
  export { className as class };
</script>

<Dialog.Portal>
  <Dialog.Overlay>
    <DialogPrimitive.Content
      class={cn(
        'relative grid w-[90%] gap-4 rounded-lg border bg-background p-6 shadow-lg sm:max-w-lg',
        className
      )}
      {transition}
      {transitionConfig}
      {...$$restProps}
    >
      <slot />
      <DialogPrimitive.Close
        class="absolute right-4 top-4 rounded-sm opacity-70 ring-offset-background transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:pointer-events-none data-[state=open]:bg-accent data-[state=open]:text-muted-foreground"
      >
        <Cross2 class="h-4 w-4" />
        <span class="sr-only">Close</span>
      </DialogPrimitive.Close>
    </DialogPrimitive.Content>
  </Dialog.Overlay>
</Dialog.Portal>

dialog-overlay.svelte

<script lang="ts">
  import { Dialog as DialogPrimitive } from 'bits-ui';
  import { fade } from 'svelte/transition';
  import { cn } from '$lib/utils.js';

  type $$Props = DialogPrimitive.OverlayProps;

  let className: $$Props['class'] = undefined;
  export let transitionConfig: $$Props['transitionConfig'] = {
    duration: 150
  };
  export { className as class };
</script>

<DialogPrimitive.Overlay asChild let:builder>
  <div
    use:builder.action
    transition:fade={transitionConfig}
    {...$$restProps}
    class={cn(
      'fixed inset-0 z-50 flex items-center justify-center bg-background/80 backdrop-blur-sm ',
      className
    )}
    {...builder}
  >
    <slot />
  </div>
</DialogPrimitive.Overlay>

One possible improvement could be modifying bits so that the overlay accepts a slot, which could then be passed down without having to delegate rendering of DialogPrimitive.Overlay to the child, which would require less manual work for custom transitions.

deronek commented 3 months ago

@wesharper Your solution did not work for me, since it changed something with the animation of the opening dialog.

@david-plugge Your workaround worked almost perfectly for me, it's just that "X" close button moved to the edge of the viewport (not edge of the dialog) after the dialog finished opening. Quick fix of adding relative to the div with builders fixed this. Thank you!