gradio-app / gradio

Build and share delightful machine learning apps, all in Python. 🌟 Star to support our work!
http://www.gradio.app
Apache License 2.0
33.41k stars 2.52k forks source link

`.change()` event dependencies not triggered if target element is contained inside closed `gr.Accordion` #3738

Closed space-nuko closed 1 year ago

space-nuko commented 1 year ago

Describe the bug

If an element that is the target of a .change() event has the event triggered while inside a gr.Accordion that is closed, any dependent events fired by that element will not be reflected when the accordion is opened

Common use case is an "enabled" checkbox that toggles the visibility of some elements inside the accordion, and an external event that controls the value of the enabled checkbox

Is there an existing issue for this?

Reproduction

  1. Launch demo, open accordion
  2. Click "Set Enabled" to toggle, then close accordion and click "Set Enabled"
  3. Open accordion again, note element visibility
  4. Click "Set Enabled" several times to see updates
#!/usr/bin/env python

import gradio as gr

with gr.Blocks() as demo:
    is_enabled = False
    with gr.Accordion("Accordion", open=False):
        with gr.Row():
            checkbox2 = gr.Checkbox(value=is_enabled)
            with gr.Column():
                checkbox = gr.Checkbox(value=False, visible=is_enabled)
                slider = gr.Slider(value=0, minimum=0, maximum=100, visible=is_enabled)
                dropdown = gr.Dropdown(["a", "b", "c"], value="a", visible=is_enabled)

    checkbox.change(lambda i: print(f"checkbox changed {i}"), inputs=[checkbox], outputs=[])
    slider.change(lambda i: print(f"slider changed {i}"), inputs=[slider])
    dropdown.change(lambda i: print(f"dropdown changed {i}"), inputs=[dropdown], outputs=[])

    changeall = gr.Button(value="Change All")
    changeall.click(lambda: [True, 42, "c"], inputs=[], outputs=[checkbox, slider, dropdown])

    checkbox2.change(lambda x: [gr.update(visible=x)] * 3, inputs=[checkbox2], outputs=[checkbox, slider, dropdown])
    changeall2 = gr.Button(value="Set Enabled", variant="primary")
    changeall2.click(lambda x: not x, inputs=[checkbox2], outputs=[checkbox2])

if __name__ == "__main__":
    demo.queue().launch()

Screenshot

No response

Logs

N/A

System Info

3.23.0, Windows, Chrome

Severity

annoying

muerrilla commented 1 year ago

That's because the target element (inside the accordion) does not exist until the accordion is opened, and then gets deleted again after it's closed. This causes a lot of trouble. At the very least I think we should get .opened() and .closed() events, but even then we should keep track of a lot of stuff (isn't this why #3198 is happening?).

I wonder if there are any benefits to the way accordions are handled, as opposed to the way tabs are (which is to make inactive ones invisible). Is it for performance? Why not do the same to tabs then (please don't 😄), cuz e.g. in cases like SD-webui, tabs are on a lower level in the hierarchy and accordions are just a tiny subset of them, so the bulk of the elements of the page are on the invisible tabs, not accordions.

P.S: If it helps to know the problem better, My own experience was, I needed to change the "step" attribute of the number inputs in an accordion to "0.00" by JS, but looks like the only way to do it is to create a mutation observer for the accordion, so I gave up and removed the arrows on the number input instead, which creates very bad UX.

abidlabs commented 1 year ago

Yup, and this is the same underlying issue that causes #3461 and #3657. Can we change the behavior of accordions so that the inner elements are not destroyed @aliabid94 @pngwn @dawoodkhan82?

anapnoe commented 1 year ago

this is a serious issue removing the elements from the DOM removes also any custom event listener added to the elements inside the component. I have no words to express my frustration,, please at least consider to have both the option to hide or destroy the elements inside

aliabid94 commented 1 year ago

@pngwn can we make these just "display: none" when hidden, instead of using ifs to to remove them from the DOM? Would that cause any issues?

pngwn commented 1 year ago

We can make it display: none for now to solve this issue but there are no guarantees that DOM element will be on the page in the future and we should make a point of this when discussing custom JS.

The actual issue is a bug that should be fixed but custom event handlers not working as expected is not a bug because we do not version the DOM, nor the mechanism by which parts of the interface are 'hidden' or 'shown'.

There are certain usecases that gradio is currently not fit for due to performance issues that arise from keeping everything on the page at all times. If we keep this limitation it will hugely limit what can be done with gradio.

space-nuko commented 1 year ago

Trying to go back to why this issue is important in the first place:

https://github.com/pkuliyi2015/multidiffusion-upscaler-for-automatic1111/blob/860f8a405193bcd992e21d82e43fa18137bc4923/javascript/bboxHint.js#L378

One case is because an extension author wants to create a custom interface that gradio doesn't support: a bounding box with sliders that update when you drag around the box. I don't think there's been progress actually adding such a component to base gradio yet. So really this whole issue is to work around the fact that there is no custom component support in gradio, hence no way to access the Svelte engine workings

If custom components were available then the kinds of hacks that necessitate events in the DOM could mostly be solved in better ways

My own case however, is sending data between gradio components in two different blocks contexts. It's to copy some dropdown values between tabs, which broke with 3.23.0. This kind of hack is due to the way webui is architected and is unlikely to ever change, hence my letdown when the dropdowns were changed to not have <select>s internally rendered in them. And the dropdown is inside an accordion also so I'd need another workaround in the DOM anyway

That case is solvable with <select>s in the dropdowns for me but unless webui's extension API undergoes breaking changes it will always need some kind of DOM event handler to keep working