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

Fixes SVG Icons #9765

Merged
merged 16 commits into from
Oct 21, 2024
Merged

Fixes SVG Icons #9765

merged 16 commits into from
Oct 21, 2024

Conversation

freddyaboulton
Copy link
Collaborator

@freddyaboulton freddyaboulton commented Oct 18, 2024

Description

Closes: #9587

SVG icons could not be displayed because the backend would return them with content_disposition_type="attachment". Developers could not set them in allowed_paths because they get moved to the cache and so the actual path fetched by the client is different to what the developer would know.

My proposed fix is to mark svg files moved to the cache before the app is running as "allowed" since they are developer supplied paths.

You can run this demo to verify that arbitrary svgs are not marked as allowed after the app is running

import gradio as gr


with gr.Blocks() as demo:
    button = gr.Button("button", icon="clean.svg")
    button.click(lambda: gr.Button("button", icon="arrow.svg"), None, button)

demo.launch()

🎯 PRs Should Target Issues

Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.

Not adhering to this guideline will result in the PR being closed.

Tests

  1. PRs will only be merged if tests pass on CI. To run the tests locally, please set up your Gradio environment locally and run the tests: bash scripts/run_all_tests.sh

  2. You may need to run the linters: bash scripts/format_backend.sh and bash scripts/format_frontend.sh

@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Oct 18, 2024

🪼 branch checks and previews

Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
🦄 Changes detected! Details

Install Gradio from this PR

pip install https://gradio-pypi-previews.s3.amazonaws.com/0aad8d21ea3b92a7e4257a1c31a9ba89845572e7/gradio-5.1.0-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@0aad8d21ea3b92a7e4257a1c31a9ba89845572e7#subdirectory=client/python"

Install Gradio JS Client from this PR

npm install https://gradio-npm-previews.s3.amazonaws.com/0aad8d21ea3b92a7e4257a1c31a9ba89845572e7/gradio-client-1.7.0.tgz

Use Lite from this PR

<script type="module" src="https://gradio-lite-previews.s3.amazonaws.com/0aad8d21ea3b92a7e4257a1c31a9ba89845572e7/dist/lite.js""></script>

@gradio-pr-bot
Copy link
Collaborator

gradio-pr-bot commented Oct 18, 2024

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
gradio patch

With the following changelog entry.

Fixes bug where SVG icons could not be used in Buttons/Chatbots.

⚠️ The changeset file for this pull request has been modified manually, so the changeset generation bot has been disabled. To go back into automatic mode, delete the changeset file.

Something isn't right?

  • Maintainers can change the version label to modify the version bump.
  • If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

@abidlabs
Copy link
Member

Could we just treat the icon of a gr.Button as a static file automatically, like we do with avatar images?

self.serve_static_file(avatar_images[0]),

@freddyaboulton
Copy link
Collaborator Author

I think button icons are already processed via serve_static_file. The problem is that serve_static_file treats files as created instead of allowed so svg's can't be loaded inline

@abidlabs
Copy link
Member

Could we check if a file was specified before the app was running, in which case serve the file inline? I think that would be a better, more general fix

@freddyaboulton
Copy link
Collaborator Author

freddyaboulton commented Oct 18, 2024

I don't follow. That's what I'm doing but only if the file is an svg. We could remove the svg mimetype check from this PR but I wanted to modify the file serving logic as little as possible for security reasons (I'm paranoid now) hehe

@freddyaboulton freddyaboulton marked this pull request as ready for review October 18, 2024 21:44
@freddyaboulton freddyaboulton added v: patch A change that requires a patch release t: fix A change that implements a fix labels Oct 18, 2024
@abidlabs
Copy link
Member

Thanks @freddyaboulton this indeed works, but I think there's a more efficient way, which is to not move such files to the cache altogether.

Currently, inside the async_move_files_to_cache, we mark svgs as safe if they are are created before the app as running by adding them to the set of static files. But in that case, why did we move them to the cache at all? We have to do this chain:

Component constructor --> serve_static_file --> async_move_files_to_cache --> _mark_svg_as_safe, i.e.

Instead, I'd suggest that we just mark svgs as safe directly in the serve_static_file file and not call async_move_files_to_cache for such files:

Component constructor --> serve_static_file --> _mark_svg_as_safe

Otherwise, this looks great. If possible, it'd be great to turn your demo into a test to ensure that arbitrary svgs are not marked as allowed after the app is running

@abidlabs
Copy link
Member

This is not ready right @freddyaboulton? You don't do the check to see if its running or not anymore.

e.g. In your original example

import gradio as gr


with gr.Blocks() as demo:
    button = gr.Button("button", icon="clean.svg")
    button.click(lambda: gr.Button("button", icon="arrow.svg"), None, button)

demo.launch()

The arrow.svg will now show up

Copy link
Member

@abidlabs abidlabs left a comment

Choose a reason for hiding this comment

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

the above needs to be fixed for security reasons

@abidlabs abidlabs dismissed their stale review October 21, 2024 22:22

nvm nvm

Copy link
Member

@abidlabs abidlabs left a comment

Choose a reason for hiding this comment

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

lgtm thanks so much @freddyaboulton!

@freddyaboulton
Copy link
Collaborator Author

Thank you @abidlabs!!

@freddyaboulton freddyaboulton merged commit df34f58 into main Oct 21, 2024
19 of 22 checks passed
@freddyaboulton freddyaboulton deleted the fix-svg-icons branch October 21, 2024 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: fix A change that implements a fix v: patch A change that requires a patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Icons are not shown properly after update to 4.44.1
3 participants