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

Prevent broken scroll_to_output in Spaces #4822

Merged
merged 4 commits into from Jul 7, 2023
Merged

Prevent broken scroll_to_output in Spaces #4822

merged 4 commits into from Jul 7, 2023

Conversation

aliabid94
Copy link
Collaborator

@aliabid94 aliabid94 commented Jul 6, 2023

Previously, a simple demo like below was broken on mobile on spaces, because Interface would force a scroll to output, and some glitch in iframeResizer would block scrolling back. Removed scroll_to_output on spaces

import gradio as gr

def do(a):
    import time
    time.sleep(5)
    return a

gr.Interface(do, "text", "text").launch()

@vercel
Copy link

vercel bot commented Jul 6, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
gradio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 6, 2023 10:18pm

@gradio-pr-bot
Copy link
Contributor

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

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Jul 6, 2023

Chromatic build successful 🎉

@abidlabs
Copy link
Member

abidlabs commented Jul 6, 2023

Can't reproduce the issue on gradio==3.35.2: https://huggingface.co/spaces/abidlabs/test-scroll

Seeing if something broke on main

Edit: I can't repro on a recent PR deploy either:

https://huggingface.co/spaces/abidlabs/test-scroll-main

@@ -283,7 +283,7 @@ def set_event_trigger(
"js": js,
"queue": False if fn is None else queue,
"api_name": api_name,
"scroll_to_output": scroll_to_output,
"scroll_to_output": False if utils.get_space() else scroll_to_output,
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't change it here -- otherwise even if a developer explicitly sets scroll_to_output=True in the event, it won't take effect in Spaces. I would change the the default value in set_event_trigger()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Disagree, it will break the app on Spaces, so even if it's explicitly allowed we stop it on Spaces. If a user sets it locally and then uploads it to spaces, it shouldn't break the app on spaces.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@freddyaboulton
Copy link
Collaborator

IMG_6056.MOV

I can recreate on main (see above) and it’s fixed in this or (see below):

IMG_6057.MOV

So this looks good to me from that standpoint!

@freddyaboulton
Copy link
Collaborator

Really confused why we’re seeing different behavior though. Maybe I have some ancient cookie in my safari browser? Or maybe my safari version is really old?

@aliabid94
Copy link
Collaborator Author

Thanks for testing!

@aliabid94 aliabid94 merged commit 5dc445b into main Jul 7, 2023
14 checks passed
@aliabid94 aliabid94 deleted the fix_scroll branch July 7, 2023 00:25
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.

None yet

4 participants