shadcn-svelte icon indicating copy to clipboard operation
shadcn-svelte copied to clipboard

bug: Dialog overlay overlaps Combobox

Open yb-cs opened this issue 1 year ago • 26 comments

Current Behavior

When I add "<Command.Group class="overflow-y-auto h-40">" 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 avatar Dec 11 '23 14:12 yb-cs

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

yb-cs avatar Dec 11 '23 14:12 yb-cs

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

huntabyte avatar Dec 11 '23 15:12 huntabyte

This is interesting I added the following "<Command.Group class="overflow-y-auto h-40 z-50">" initially it is not blurry but once you scroll it becomes blurry again.

yb-cs avatar Dec 11 '23 15:12 yb-cs

1 2 See the difference before and after scrolling.

yb-cs avatar Dec 11 '23 15:12 yb-cs

Add it to the Command.List

huntabyte avatar Dec 11 '23 17:12 huntabyte

it is actually worse than group. Somehow group fixed it before scrolling but this is blurry in both cases: 3 4 <Command.List class="overflow-y-auto h-40 z-50"> <Command.Group>

and

    <Command.List>
      <Command.Group>

yb-cs avatar Dec 11 '23 17:12 yb-cs

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 avatar Dec 11 '23 17:12 yb-cs

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 avatar Dec 11 '23 17:12 yb-cs

@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 :)

yb-cs avatar Dec 13 '23 02:12 yb-cs

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

huntabyte avatar Feb 21 '24 03:02 huntabyte

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?

slinso avatar Apr 29 '24 10:04 slinso

@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.

huntabyte avatar Apr 29 '24 13:04 huntabyte

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.

JoshuaHintze avatar Apr 29 '24 13:04 JoshuaHintze

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 avatar Apr 30 '24 09:04 slinso

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.

slinso avatar May 08 '24 05:05 slinso

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

david-plugge avatar May 08 '24 16:05 david-plugge

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:

  • nest the DialogPrimitive.Content inside Dialog.Overlay
  • remove "fixed left-[50%] top-[50%] translate-x-[50%] translate-y-[50%]" from DialogPrimitive.Content

dialog-overlay.svelte:

  • add "flex items-center justify-center"
  • add to DialogPrimitive.Overlay

slinso avatar May 10 '24 10:05 slinso

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>

david-plugge avatar May 12 '24 19:05 david-plugge

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

JoshuaHintze avatar May 13 '24 13:05 JoshuaHintze

@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.

slinso avatar May 13 '24 13:05 slinso

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

david-plugge avatar May 13 '24 13:05 david-plugge

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

Dart353 avatar May 14 '24 17:05 Dart353

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.

slinso avatar May 15 '24 21:05 slinso

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.

wesharper avatar May 21 '24 22:05 wesharper

@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!

deronek avatar Jul 03 '24 17:07 deronek