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

Add .key_up event listener to gr.Dropdown() #7404

Merged
merged 29 commits into from
Feb 14, 2024
Merged

Add .key_up event listener to gr.Dropdown() #7404

merged 29 commits into from
Feb 14, 2024

Conversation

abidlabs
Copy link
Member

@abidlabs abidlabs commented Feb 12, 2024

Closes: #7388 cc @apolinario

Note: I had initially called it .type() but that conflicts with the Dropdown.type attribute. Also, because typing does not automatically update the value of the Dropdown component, added a new KeyDownData class to transmit the relevant data.

@abidlabs abidlabs added the v: minor A change that requires a minor release label Feb 12, 2024
@abidlabs abidlabs changed the base branch from main to dropdown-bugs February 12, 2024 22:03
@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Feb 12, 2024

🪼 branch checks and previews

Name Status URL
Spaces ready! Spaces preview
Website ready! Website preview
Storybook ready! Storybook preview
🦄 Changes detecting...

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/8d5e6b4de07c0460290f4fd77eef6c76d2eedfc7/gradio-4.18.0-py3-none-any.whl

Install Gradio Python Client from this PR

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

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Feb 12, 2024

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
@gradio/app minor
@gradio/dropdown minor
@gradio/utils minor
gradio minor
  • Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

Add .key_up event listener to gr.Dropdown()

Maintainers or the PR author can modify the PR title to modify this entry.

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 abidlabs changed the title Add .type event listener to gr.Dropdown() Add .key_down event listener to gr.Dropdown() Feb 12, 2024
@abidlabs abidlabs marked this pull request as ready for review February 12, 2024 22:48
"gradio": minor
---

fix:Add `.type` event listener to `gr.Dropdown()`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fix:Add `.type` event listener to `gr.Dropdown()`
fix:Add `.key_down` event listener to `gr.Dropdown()`

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the changeset didnt get triggered here

Copy link
Member Author

Choose a reason for hiding this comment

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

Weird, maybe b/c this wasn't initially targeting main, now it is

@aliabid94
Copy link
Collaborator

Keep in mind key_down is triggered on any key press, including arrow keys, shift, etc. which do not affect the actual typed value. Also, key_down is triggered before the value of an input field changes (unlike key up, which triggers after the input value changes)

Base automatically changed from dropdown-bugs to main February 12, 2024 23:52
@abidlabs
Copy link
Member Author

Keep in mind key_down is triggered on any key press, including arrow keys, shift, etc. which do not affect the actual typed value.

Yes, I considered excluding those but figured that it would be better to just provide the .key data and let the developer handle it themselves

Also, key_down is triggered before the value of an input field changes (unlike key up, which triggers after the input value changes)

Yup but again I think this is fine because we provide both the .input_value which is the state of the component before the key press, and the key data and the developer can put them together.

@aliabid94
Copy link
Collaborator

Also, key_down is triggered before the value of an input field changes (unlike key up, which triggers after the input value changes)

Yup but again I think this is fine because we provide both the .input_value which is the state of the component before the key press, and the key data and the developer can put them together.

Why not just use the key up method

@abidlabs
Copy link
Member Author

Why not just use the key up method

Tbh no strong reason, I can change it

@abidlabs abidlabs changed the title Add .key_down event listener to gr.Dropdown() Add .key_up event listener to gr.Dropdown() Feb 13, 2024
@abidlabs
Copy link
Member Author

Ok I've changed it to use .key_up(). The modified version of your demo @aliabid94:

import gradio as gr

def test(value, key_up_data: gr.KeyUpData):
    input_val = key_up_data.input_value
    return gr.Dropdown(value=input_val, choices=[f"{input_val} {i}" for i in range(10)])

with gr.Blocks() as demo:
    d = gr.Dropdown(["abc", "def"], allow_custom_value=True)
    t = gr.JSON()
    d.key_up(test, d, d, show_progress="hidden")

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

still doesn't work, but for a different reason. The problem is the issue that @pngwn brought up before which is when a component is used as an output component, it it set to be static and then converted back to interactive, so it loses its state. In this case, it means that the Dropdown choices are hidden and the Dropdown is blurred. This leads to an unusuable UX.

There are a few different ways we could fix it, the simplest being that we don't do this interactive/static switch when show_progress="hidden". But I believe that @pngwn has a more comprehensive fix planned soon, so I'll leave that out of the scope of this PR.

@abidlabs
Copy link
Member Author

Ok I've made a temporary workaround by not disabling output components when the event is of type key_up (we do the same thing for focus actually) so that we can get this in. We're discussing a more comprehensive solution internally.

@aliabid94 the demo I shared above should work now. Look ok to merge?

Comment on lines +263 to +267
on:keyup={(e) =>
dispatch("key_up", {
key: e.key,
input_value: input_text
})}
Copy link
Member

Choose a reason for hiding this comment

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

I would just use keypress here, the issue with keyup is that it won't fire if you hold the key down even though the text will be displayed which isn't very intuitive.

Comment on lines +534 to +537
key_up = EventListener(
"key_up",
doc="This listener is triggered when the user presses a key while the {{ component }} is focused.",
)
Copy link
Member

Choose a reason for hiding this comment

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

I think keypress is a little more intuitive, partly because we might want to change precisely when this even is fired (keypresses have multiple stages) but also because 'press' is a little more generic. key press phases are pretty close to an implementation detail.

@aliabid94
Copy link
Collaborator

I remember users have asked for adding keyboard shortcuts to their apps before, this api could lead to something like:

with gr.Blocks() as demo:
  def keylistener(evt: KeyPressEvent):
    if evt.keyCode == ...

  demo.keypress(keylistener)

allowing keypress listeners to a whole app

@pngwn
Copy link
Member

pngwn commented Feb 13, 2024

Sometimes they want the keypress to do some discreet UI thing that isn't really controllable via python atm though. We have had lots of image editor related keyless request which would be almost impossible to implement via a backend function and would lead to a really bad experience anyway.

@@ -3,4 +3,4 @@
"gradio": patch
---

fix:Retain dropdown value if choices have been changed
fix:Retain dropdown value if choices have been changed
Copy link
Collaborator

Choose a reason for hiding this comment

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

again, not the correct message

Copy link
Collaborator

@aliabid94 aliabid94 left a comment

Choose a reason for hiding this comment

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

LGTM, agree that we should be using KeyPress everywhere instead of KeyUp

@abidlabs
Copy link
Member Author

Thanks for the reviews @aliabid94 and @pngwn. I'll replace with .keypress then.

@abidlabs
Copy link
Member Author

abidlabs commented Feb 13, 2024

Actually @aliabid94 @pngwn the keypress event is deprecated and keydown is the recommended alternative: https://developer.mozilla.org/en-US/docs/Web/API/Element/keypress_event.

In most of demos though (including the demo described by @apolinario) I think, keyup is preferable to keydown as it includes the latest value of the input. I tested the demo with keyup and it works quite well. Let me know if I'm missing anything!

@abidlabs abidlabs merged commit 065c5b1 into main Feb 14, 2024
8 checks passed
@abidlabs abidlabs deleted the dropdown-type branch February 14, 2024 01:14
@pngwn pngwn mentioned this pull request Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v: minor A change that requires a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.change or .input event isn't triggered while typing in gr.Dropdown when filterable=True
5 participants