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

Datepicker: Trigger "input" event after select date #2081

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

worpet
Copy link

@worpet worpet commented May 25, 2022

This is a small change to fix #2078. The input's "input" event is triggered in addition to the "change" event.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 25, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: worpet / name: Bill Orpet (8a60315)

@fnagel
Copy link
Member

fnagel commented Jun 20, 2022

Hey @worpet , thanks for the contribution!

Can you explain why this would be needed? It would definitely need tests though.

@worpet
Copy link
Author

worpet commented Jun 21, 2022

Triggering the "input" event is needed for the same reason it currently triggers the "change" event: because the contents of the input have changed and anything watching that input for changes should be notified.

There are interoperability problems without this. For example, we are working with a system that watches all form inputs for changes via each of their "input" events. Using the Datepicker, it has created a situation where the input it is attached to does not trigger the "input" event after its value has changed via a date being chosen.

Is there currently a test testing that .trigger("change") triggers the change event? If so, I can duplicate that test to test that .trigger("input") triggers the "input" event.

@fnagel
Copy link
Member

fnagel commented Jun 24, 2022

@worpet Thanks for the input! To me, this change sounds legit but I'm unsure if this will breaks stuff for people.

Regarding tests, take a look here: https://github.com/jquery/jquery-ui/blob/main/tests/unit/datepicker/events.js#L29

@mgol What do you thin about this?

@dmethvin
Copy link
Member

For example, we are working with a system that watches all form inputs for changes via each of their "input" events.

Is this a jQuery-based system? Triggering an input event in jQuery only effects jQuery. The input event arrived a few years after jQuery was written so, for AFAIK, there's no use of it in jQuery core or UI.

@worpet
Copy link
Author

worpet commented Jun 24, 2022

@dmethvin Yes, it is primarily a jQuery system. I understand that jQuery UI stopped active development before this event became widely supported. In jQuery core, the "input" event is supported the same as any other event (you can add listeners for it or trigger it). We are using the Datepicker as a convenient way to add a calendar to relevant inputs. It works well, except it does not trigger the "input" event after changing the input's value.

I understand if you're not interested in adding this, but it seemed like an easy and straightforward change that can extend the useful life of Datepicker.

@dmethvin
Copy link
Member

@worpet I am trying to understand the situation. If we determine a way to do this without changes to jQuery UI then you can use the current version and don't need to wait for a new release. Is this a blocker to your work? What are you doing at the moment? If the other parts of the system are jQuery it seems like you could use the change event since it's already triggered at the exact moment that input would be triggered.

@mgol
Copy link
Member

mgol commented Jun 27, 2022

@fnagel You marked it as Behavior Change; do you consider it requiring a minor version bump?

@dmethvin In many cases it's probably the easiest to just listen for the change event. But if one's working with a generic wrapper system accepting multiple components and reacting for changes, it may be less trivial to special-case datepicker.

@worpet In general, semantics of the input & change event are different; input fires more often. Is datepicker the only place needing such a change? In the case of datepicker, is it enough to just trigger both events here?

@fnagel
Copy link
Member

fnagel commented Jul 7, 2022

@mgol Adding an event looks like a change that could break things for people. So a minor release would be a good thing in theory. Not sure if it's worth the wait though.

@worpet
Copy link
Author

worpet commented Jul 8, 2022

@dmethvin Our workaround is to use Datepicker's "onSelect" option to fire the "input" event, since that is when the calendar's value is synced to the input's value. So this change is not critical for us, but I thought I would try contributing since it would be nice if it worked out of the box.

@mgol This is a good point that adding "input" to only Datepicker may make it inconsistent with the other widgets that remain without it.

@mgol
Copy link
Member

mgol commented Jul 11, 2022

The jQuery UI codebase seems largely unaware of the input event but there's one place where it's been taken into account, in autocomplete: #275. However, that change just listenes for the event, it doesn't trigger one.

In many cases, adding support for the input event won't be a straight addition to where the change is being fired. But it seems in case of datepicker those are largely interchangeable in my testing, a few test native inputs: https://jsfiddle.net/m_gol/cj80m3g7/10/.

It might be fine to do this change just for the datepicker; folks interested in other widgets could submit separate PRs. What do you think, @fnagel?

@worpet before we can accept a PR, it would still need tests.

@fnagel
Copy link
Member

fnagel commented Jul 18, 2022

@mgol I'm against changing this for Datepicker only, making it the only widget triggering the input event. I would like to keep a little consistency here. I'm fine with a PR changing it for all, but this seems a more complex task.

As @worpet mentioned, the workaround here is pretty straight forward, so I'm more likely to vote against this PR. Sorry @worpet :-(

@mgol
Copy link
Member

mgol commented Jul 20, 2022

@fnagel OK, makes sense, thanks for your input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Datepicker does not fire input "input" event
5 participants