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

Custom JavaScript breaks when accordion is closed #3657

Closed
1 task done
pkuliyi2015 opened this issue Mar 28, 2023 · 10 comments · Fixed by #4073
Closed
1 task done

Custom JavaScript breaks when accordion is closed #3657

pkuliyi2015 opened this issue Mar 28, 2023 · 10 comments · Fixed by #4073
Labels
bug Something isn't working

Comments

@pkuliyi2015
Copy link

pkuliyi2015 commented Mar 28, 2023

Describe the bug

When I use change() to change the component values:

  1. The slider will call its js twice.
  2. When close the accordion, all inside js will be broken.

Is there an existing issue for this?

  • I have searched the existing issues

Reproduction

Simply run this code:

import gradio as gr

with gr.Blocks() as demo:
	checkbox = gr.Checkbox(label='Will change the label inside', interactive=True)
	with gr.Accordion(label='Test Accordion'):
		checkbox_js = gr.Checkbox(label='Custom Js for Checkbox', interactive=True)
		slider_js = gr.Slider(label='Custom Js Slider', minimum=0, maximum=1, step=0.01, value = 0.66, interactive=True)
		checkbox_js.change(fn=lambda x:x, inputs=checkbox_js, outputs=checkbox_js, _js='(value)=> alert("Checkbox Js is working") === null ? value : value')
		slider_js.change(fn=lambda x:x, inputs=slider_js, outputs=slider_js, _js='(value)=>  (alert("Slider Js is working") === null && value == 0) ? 0.2 : 0.8')
	checkbox.change(fn=lambda x: (x, 0 if x else 1), inputs=checkbox, outputs=[checkbox_js, slider_js])
demo.launch()

When you check the outside checkbox, the checkbox js will run once but the slider js will run twice.
When you check the inside checkbox its js run normally, but the slider js will still run twice.
When you close the accordion, none of the js will work.

Screenshot

No response

Logs

No errors, but it breaks all custom javascripts inside the accordion, hence all calculation breaks. 

I suspected it is because you removed the DOM when accordion close and custom js becomes no longer available.

I can't find anyway to bypass the bug and continue, so have to downgrade to an older version.

System Info

Gradio==3.23.0, Python = 3.10.9. Clean environment with no other packages installed.

Note: on gradio==3.16.0 the accordion won't break the js, but checkbox js won't work.

Severity

blocking upgrade to latest gradio version

@pkuliyi2015 pkuliyi2015 added the bug Something isn't working label Mar 28, 2023
@space-nuko
Copy link
Contributor

space-nuko commented Mar 28, 2023

It's because the lack of custom components means state has to be tracked through frontend kludges, the only way for it to work without keeping the elements on the page would be #1432 being implemented, so the state can be saved to the backend

It's surely possible to make the DOM removing behavior configurable but the only purpose it would serve is a workaround for an implementation detail that's subject to change at any time

@pkuliyi2015
Copy link
Author

Well, I'm using the official feature that documented by your official doc.

I'm not doing anything complex, I'm not getting or setting values through JavaScript.

I don't implement any custom components. I just use the features provided by you.

Then why will my code break across versions? Your updates mentioned nothing about this. I waste a whole day on this problem and finally discovered that it's your update deleting DOM.

IS THAT MY FAULT and I should be responsible for this?

@abidlabs
Copy link
Member

Hi @pkuliyi2015 we'll take a look at this. Just to clarify, @space-nuko is a community member who has graciously tried to answer your question and I don't think the tone in your response is appropriate directed at any maintainer of this repo, let alone a community member. Feedback is always appreciated, but please being kind and respectful when giving it is also important. Don't forget there are people at the other side of the screen please 🤗

@pkuliyi2015
Copy link
Author

pkuliyi2015 commented Mar 28, 2023

Well, admittedly I got quite annoying at the response time. I becomes quite calm now and sincerely apologize for any impolite from my words.

But I just want to clarify this is a new bug from the updating, instead of some "custom components" related to a issue half years ago. All problems emerge with only your well-documented official features.

@abidlabs
Copy link
Member

Thanks @pkuliyi2015.

But I just want to clarify this is a new bug from the updating, instead of some "custom components" related to a issue half years ago. All problems emerge with only your well-documented official features.

That is true, and we do recognize this issue -- that updating the versions can break custom js / css. We've discussed this issue internally and while we cannot guarantee that DOM will not change between versions, we will better document the risks of using custom css and js and potential mitigation strategies.

@abidlabs
Copy link
Member

If it's all right @pkuliyi2015 maybe you can rename this issue to say that we need to document custom css / js better, and we'd be happy to do that

@pkuliyi2015
Copy link
Author

pkuliyi2015 commented Mar 28, 2023

Great thanks for your efficient response and I wonder if there can be a guide to bypass the bug.

@pkuliyi2015
Copy link
Author

If it's all right @pkuliyi2015 maybe you can rename this issue to say that we need to document custom css / js better, and we'd be happy to do that

Oh this is not a document problem I think. the binding javascript will be invalid when it's DOM is deleted, which means that the js become unreliable. Such behavior has affected many functions in the stable diffusion webUI and its extensions where js is necessary, for example it's image browser, and we are all finding ways to circumvent it.

If in the future version you removed the DOM for tabs, the problem will become even more severe. I sincerely hope that custom js will remain stable. Invisible doesn't means non-exist, from my perspective.

@pkuliyi2015
Copy link
Author

By the way I'm not quite clear which commit are you deleting the DOM. Could you please guide me to the place where you make such change? I'm willing to do a PR to fix this with some commonly acceptable strategies.

@space-nuko
Copy link
Contributor

Unsure which commit changed it since 3.16 but it looks like these lines

{#if open}
<slot />
{/if}

{#if} svelte directive seems to modify the DOM if the conditional check is false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants