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

Dropdown cutoff fix #4691

Merged
merged 16 commits into from Jun 28, 2023
Merged

Dropdown cutoff fix #4691

merged 16 commits into from Jun 28, 2023

Conversation

abidlabs
Copy link
Member

@abidlabs abidlabs commented Jun 26, 2023

Closes: #4690 as can be verified by kitchen sink demo:

image

Thanks @aliabid94 for the suggestion on how to debug this

@gradio-pr-bot
Copy link
Contributor

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4691-all-demos

@@ -23,7 +23,7 @@
border: var(--block-border-width) solid var(--border-color-primary);
border-radius: var(--block-radius);
background: var(--border-color-primary);
overflow: hidden;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does overflow-y also work? Seems like it would be a more targeted fix if so

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made the change

@freddyaboulton
Copy link
Collaborator

@abidlabs The corners of the forms in the blocks_layout demo are cut off in the deployed demos:

This PR

image

Main

image

@aliabid94
Copy link
Collaborator

@abidlabs The corners of the forms in the blocks_layout demo are cut off in the deployed demos:

This is a good catch. I'm trying to think of the best way to fix this.

@aliabid94
Copy link
Collaborator

Ok I came up with a solution, somewhat hacky. We create 4 "corner elements", absolutely positioned, that are just the rounded corner itself at each of the corners of the form element. It is just the rounded border and nothing else.

Let me know if you have better ideas @pngwn @hannahblair @abidlabs, happy to implement this if there's no better idea.

@abidlabs
Copy link
Member Author

abidlabs commented Jun 27, 2023

That approach makes sense to me @aliabid94. Keeping in mind that some themes like monochrome don't have the rounded corners at all

@abidlabs
Copy link
Member Author

In the meantime I'll address the other suggestions, thanks @freddyaboulton!

@aliabid94
Copy link
Collaborator

actually this approach feels quite hacky because I have to figure out the background color behinf the corners too - I'll wait to see if @pngwn has a better approach

@aliabid94
Copy link
Collaborator

@pngwn what if dropdown options had position: fixed, with position calculated at open time? That should make it break out of the overflow parent, right?

@aliabid94
Copy link
Collaborator

actually pushed a fix using position: fixed, not sure if there might be unintended consequences of it. Changes here: cb8dd45

@pngwn
Copy link
Member

pngwn commented Jun 27, 2023

I'm not fan of making things position:fixed, i feel like that can cause lots of issues.

This CSS seems to work:

	.form > :global(*) {
		box-shadow: none;
		border-width: 0px;
		border-radius: 0px;
	}

	.form > :global(*):first-child {
		border-top-left-radius: var(--block-radius);
		border-top-right-radius: var(--block-radius);
	}

	.form > :global(*):last-child {
		border-bottom-left-radius: var(--block-radius);
		border-bottom-right-radius: var(--block-radius);
	}

The descendant selector > ensures that we will only select direct children of the .form element so it is pretty limited in what it can affect.

For context these articles are my inspiration here: https://alistapart.com/article/axiomatic-css-and-lobotomized-owls/ and https://every-layout.dev/layouts/stack/ and inform the approach of letting the parent take care of this via CSS as it is cheaper and easier to maintain than either JS solutions or applying css to every child.

@aliabid94
Copy link
Collaborator

I don't think this is guaranteed to work. If the form is in Row layout, then it would be different. And if the Row wraps, then it could be 4 different elements at each corner.

@pngwn
Copy link
Member

pngwn commented Jun 27, 2023

Ah yeah, you're right, even if we accounted for row/col the wrapping would cause issues.

Let me try one last idea.

@pngwn
Copy link
Member

pngwn commented Jun 27, 2023

Lets go with the position fixed thing for now. I don't love it but it definitely works and I havent seen any issues just yet.

I think there may be some other options but we can revisit.

Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation isn't correct, we need to recalculate the fixed position whenever we scroll or the dropdown will not move with the rest of the page. It works on spaces because the way the iframe resizer break position fixed but outside of spaces this does not work correctly.

Screen.Recording.2023-06-27.at.17.36.05.mov

@aliabid94
Copy link
Collaborator

If it moves with the page, that would be done with a JS scroll listener right? not native CSS

@pngwn
Copy link
Member

pngwn commented Jun 27, 2023

Yeah, you'll need to listen to the window's scroll event and adjust as necessary but you'll need to throttle it or it will kill the performance of the page. Svelte has window scroll bindings you can use https://svelte.dev/docs/special-elements#svelte-window

You'll also need to double check spaces when you implement it as that is a weird environment.

@aliabid94
Copy link
Collaborator

Made the changes 37b7727 - I used window directly, should I use svelte:window instead?

@aliabid94 aliabid94 requested a review from pngwn June 27, 2023 17:40
@pngwn
Copy link
Member

pngwn commented Jun 27, 2023

Made the changes 37b7727 - I used window directly, should I use svelte:window instead?

Yeah, preferably, it is a neater abstraction with no extra code.

js/form/src/DropdownOptions.svelte Outdated Show resolved Hide resolved
js/form/src/DropdownOptions.svelte Outdated Show resolved Hide resolved
aliabid94 and others added 2 commits June 27, 2023 13:53
Co-authored-by: Aarni Koskela <akx@iki.fi>
Co-authored-by: Aarni Koskela <akx@iki.fi>
Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, although some strange behaviour, have left a few comments that should resolve things.

js/form/src/DropdownOptions.svelte Outdated Show resolved Hide resolved
Comment on lines -37 to 59
bottom = `${input_height}px`;
bottom = `${distance_from_bottom + input_height}px`;
max_height = distance_from_top - input_height;
top = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently this is the only branch that is triggering because distance_from_bottom is always identical to distance_from_top. The above change should fix this.

js/form/src/DropdownOptions.svelte Outdated Show resolved Hide resolved
js/form/src/DropdownOptions.svelte Outdated Show resolved Hide resolved
@aliabid94 aliabid94 requested a review from pngwn June 28, 2023 16:31
@aliabid94
Copy link
Collaborator

thanks for the suggestions @pngwn, should all be addressed.

Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, looks great. Thanks for taking care of this @aliabid94!

@aliabid94 aliabid94 merged commit dd97ee9 into main Jun 28, 2023
8 checks passed
@aliabid94 aliabid94 deleted the dropdown-fix branch June 28, 2023 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdown is cut off at the bottom of Form
6 participants