In modern browsers (personally tested in Chrome and Firefox), input elements with type="number" show often show a "spinner" widget in the input box. However, when one of these number input elements has filled/empty as state condition, the JavaScript does not respond when one uses the spinner widget.
1. Create form structure with a dependee number form element:
$form['number'] = [
'#type' => 'number',
'#title' => $this->t('Number'),
];
$form['test'] = [
'#type' => 'item',
'#title' => $this->t('Test field'),
'#states' => [
'visible' => [
':input[name="number"]' => ['filled' => TRUE],
],
],
];
2. Render the form in a modern browser with JavaScript enabled.
3. Use the spinner widget to update the number field; notice how the test field is not made visible.
The empty/filled state trigger only listens for the keyup event, add implementation to listen to the change event too.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | 10.1.x-3181634-11.patch | 2.73 KB | wongjn |
| #7 | 3181634-07.patch | 3.21 KB | wongjn |
| #2 | 3181634-02.patch | 753 bytes | wongjn |
Comments
Comment #2
wongjn commentedComment #5
awset commentedThanks for the patch. I've reviewed the issue summary, and patch,. Based on the following, I'm marking RTBC.
Comment #6
lauriiiWould it be possible to add test coverage for this?
Comment #7
wongjn commentedHere we go with a test. A bit more synthetic than I would like but as far as I know, there is no API to use the spinner widget programmatically.
Comment #8
joshua1234511I've reviewed the issue summary, and the latest patch #7 . Based on the following, I'm marking RTBC.
Patch applied to 9.4
Test passes
Results:
Before batch: https://nimb.ws/OyYeod
After Patch: https://nimb.ws/Z3lqzP
Comment #10
alexpottWe need a 10.x version of the patch. There's no .es6 in D10 (it's not needed) and IE support is dropped so JS standards have changed.
Comment #11
wongjn commentedHere's a patch based on 10.1.x.
Comment #13
smustgrave commentedTested on Drupal 10.1 with a standard install
Took the code snippet from the issue summary and added to a random form, in my case it was admin_toolbar.
Confirmed using number widget the field did not appear
With the patch it does.
Comment #17
lauriiiCommitted 8542a65 and pushed to 10.1.x. Also cherry-picked to 10.0.x, and committed #7 to 9.5.x. Thanks!
Comment #18
idebr commentedComment #20
kristen polI was reviewing this and don't understand why no one who tested this got issue credit: awset, joshua1234511, smustgrave
Comment #21
lauriiiI didn't credit @awset or @joshua1234511 because it wasn't sure based on their comments what had been tested. There wasn't anything specific about this issue in their comments. There may have been additional information behind the external links on #8 but since the website was unfamiliar to me, I decided not to visit them.
The comment from @smustgrave on #13 provides a helpful explanation to what had been tested. I don't recall why I decided not to give credit for that, but it could be because there were automated tests already part of the patch. States API changes tend to be something that are helpful to test manually, but usually that would be outside of the specific change that we are introducing to make sure there wasn't regressions to untested functionality outside of the change.
Comment #22
kristen polThank you for the explanation. I’ll try to share with my coworkers so they can do better in the future.