jquery-ui icon indicating copy to clipboard operation
jquery-ui copied to clipboard

Datepicker: Trigger "input" event after select date

Open worpet opened this issue 3 years ago • 13 comments

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

worpet avatar May 25 '22 18:05 worpet

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: worpet / name: Bill Orpet (8a60315d50c4e2a5ef3020e6913153831db9d8e9)

Hey @worpet , thanks for the contribution!

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

fnagel avatar Jun 20 '22 21:06 fnagel

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.

worpet avatar Jun 21 '22 13:06 worpet

@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?

fnagel avatar Jun 24 '22 12:06 fnagel

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.

dmethvin avatar Jun 24 '22 13:06 dmethvin

@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.

worpet avatar Jun 24 '22 22:06 worpet

@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.

dmethvin avatar Jun 25 '22 12:06 dmethvin

@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?

mgol avatar Jun 27 '22 20:06 mgol

@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.

fnagel avatar Jul 07 '22 23:07 fnagel

@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.

worpet avatar Jul 08 '22 13:07 worpet

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.

mgol avatar Jul 11 '22 10:07 mgol

@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 :-(

fnagel avatar Jul 18 '22 21:07 fnagel

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

mgol avatar Jul 20 '22 12:07 mgol