Bug 36774 - Flatpickr clear() adds unintentional clear button
Summary: Flatpickr clear() adds unintentional clear button
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Pedro Amorim
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 34263 36804
  Show dependency treegraph
 
Reported: 2024-05-03 14:04 UTC by Pedro Amorim
Modified: 2024-05-28 21:56 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.06,23.05.12


Attachments
Bug 36774: [DONT PUSH] TEST ONLY (1.43 KB, patch)
2024-05-03 14:18 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36774: Dont add wrapper if already exists (1.13 KB, patch)
2024-05-03 14:18 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36774: [DONT PUSH] TEST ONLY (1.47 KB, patch)
2024-05-04 20:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 36774: Dont add wrapper if already exists (1.18 KB, patch)
2024-05-04 20:32 UTC, David Nind
Details | Diff | Splinter Review
Bug 36774: Ensure we are looking for a parent with the given class (1.04 KB, patch)
2024-05-07 12:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36774: [DONT PUSH] TEST ONLY (1.47 KB, patch)
2024-05-07 16:08 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36774: Dont add wrapper if already exists (1.42 KB, patch)
2024-05-07 16:08 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36774: (QA Follow-up): Use flatpickr clear (1.17 KB, patch)
2024-05-07 16:08 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36774: [DONT PUSH] TEST ONLY (1.47 KB, patch)
2024-05-08 11:03 UTC, David Nind
Details | Diff | Splinter Review
Bug 36774: Dont add wrapper if already exists (1.42 KB, patch)
2024-05-08 11:03 UTC, David Nind
Details | Diff | Splinter Review
Bug 36774: (QA Follow-up): Use flatpickr clear (1.22 KB, patch)
2024-05-08 11:03 UTC, David Nind
Details | Diff | Splinter Review
Bug 36774: Dont add wrapper if already exists (1.49 KB, patch)
2024-05-08 12:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36774: (QA Follow-up): Use flatpickr clear (1.29 KB, patch)
2024-05-08 12:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36774: [DONT PUSH] TEST ONLY (1.47 KB, patch)
2024-05-08 15:46 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36774: Remove wrapper if already exists (1.96 KB, patch)
2024-05-08 15:46 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36774: Add cypress tests (1.57 KB, patch)
2024-05-08 15:46 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36774: Add cypress tests (1.64 KB, patch)
2024-05-08 15:50 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 36774: Remove wrapper if already exists (2.02 KB, patch)
2024-05-09 14:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36774: Add cypress tests (1.71 KB, patch)
2024-05-09 14:41 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2024-05-03 14:04:46 UTC

    
Comment 1 Pedro Amorim 2024-05-03 14:18:37 UTC
Created attachment 166138 [details] [review]
Bug 36774: [DONT PUSH] TEST ONLY

Test plan:
1) Apply only the TEST patch
2) Visit http://localhost:8081/cgi-bin/koha/serials/serials-home.pl
3) Click the "Expires before" input and insert any date
4) Click the "TEST" link under. It calls the native flatpickr.clear():
    https://flatpickr.js.org/instance-methods-properties-elements/#clear
5) Notice it clears the date value. But it also appends a new 'x' icon to clear the input.
6) Apply 2nd patch. Repeat. Notice it works as expected
Comment 2 Pedro Amorim 2024-05-03 14:18:39 UTC
Created attachment 166139 [details] [review]
Bug 36774: Dont add wrapper if already exists
Comment 3 Pedro Amorim 2024-05-03 14:24:43 UTC
Context at bug 34263
Comment 4 Pedro Amorim 2024-05-03 14:28:20 UTC
There is also calendar.inc at opac-tmpl/bootstrap/en/includes/calendar.inc and this change further diverges both files.

It's a dupe of this file but for the OPAC (see bug 35716 if you're interested in this topic i.e. stop duping assets for OPAC+INTRA)
Comment 5 David Nind 2024-05-04 20:32:39 UTC
Created attachment 166162 [details] [review]
Bug 36774: [DONT PUSH] TEST ONLY

Test plan:
1) Apply only the TEST patch
2) Visit http://localhost:8081/cgi-bin/koha/serials/serials-home.pl
3) Click the "Expires before" input and insert any date
4) Click the "TEST" link under. It calls the native flatpickr.clear():
    https://flatpickr.js.org/instance-methods-properties-elements/#clear
5) Notice it clears the date value. But it also appends a new 'x' icon to clear the input.
6) Apply 2nd patch. Repeat. Notice it works as expected

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2024-05-04 20:32:41 UTC
Created attachment 166163 [details] [review]
Bug 36774: Dont add wrapper if already exists

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2024-05-04 20:35:25 UTC
Added assignee
Comment 8 Jonathan Druart 2024-05-05 19:16:22 UTC
Bug 30718: Prevent 'reset' icon to appear twice

-            $(instance.input).siblings('input.flatpickr')
+            $(instance.input).find('~input.flatpickr:first')

Maybe we need to adjust this line as well then because it was supposed to deal with that problem.
Comment 9 Pedro Amorim 2024-05-07 08:45:28 UTC
(In reply to Jonathan Druart from comment #8)
> Bug 30718: Prevent 'reset' icon to appear twice
> 
> -            $(instance.input).siblings('input.flatpickr')
> +            $(instance.input).find('~input.flatpickr:first')
> 
> Maybe we need to adjust this line as well then because it was supposed to
> deal with that problem.

I don't know. From my testing, that change from bug 30718 didn't fix the problem it said it fixed, i.e. the problem is there before and after that change.
However, reverting it back to the previous code makes the "x" button stop working, it doesn't clear anymore.
I don't have more time to dig deeper here but unless we come up with a test plan highlighting why we need to adjust that line I'd say it's a non-issue imo.
Comment 10 Jonathan Druart 2024-05-07 09:15:15 UTC
I guess it was supposed to fix this:

From bug 30718 comment 22
"""
I notice also that paired date fields (start date & end date) now show an extra "X" icon after the second field, e.g. serial claims or catalog statistics.
"""
Comment 11 Pedro Amorim 2024-05-07 12:01:06 UTC
(In reply to Jonathan Druart from comment #10)
> I guess it was supposed to fix this:
> 
> From bug 30718 comment 22
> """
> I notice also that paired date fields (start date & end date) now show an
> extra "X" icon after the second field, e.g. serial claims or catalog
> statistics.
> """

Tested both entries of the JS line below on the ILL requests page (there are 4 flatpickr instances on the left side filters):
> -            $(instance.input).siblings('input.flatpickr')
> +            $(instance.input).find('~input.flatpickr:first')

I couldn't spot any difference before and after. Mind you I didn't test the clear itself because those filters 'clear' functionality needs to be updated, but the rendering of the inputs is fine, no additional "x" anywhere are rendered. Also looked at /tools/holidays.pl but can't find any issue.
Comment 12 Jonathan Druart 2024-05-07 12:44:01 UTC
On main, if I revert 
  commit 97a4b3c61da522f35628f80382d21bbff56b76ba
  Bug 30718: Prevent 'reset' icon to appear twice

I am seeing: https://snipboard.io/XLKMVg.jpg on /cgi-bin/koha/serials/claims.pl?supplierid=1 KO (2 clears for "to")


On main, I am seeing: https://snipboard.io/u728qI.jpg OK 1 clear for "from", 1 for "to"

On main + your patch: https://snipboard.io/6BusJT.jpg KO 1 clear for "from" but no clear for "to"
Comment 13 Jonathan Druart 2024-05-07 12:54:56 UTC
Created attachment 166278 [details] [review]
Bug 36774: Ensure we are looking for a parent with the given class

And not one of its children
Comment 14 Jonathan Druart 2024-05-07 12:55:08 UTC
I think you want that.
Comment 15 Pedro Amorim 2024-05-07 16:08:20 UTC
Comment on attachment 166278 [details] [review]
Bug 36774: Ensure we are looking for a parent with the given class

Hi Joubu, thank you very much for the follow-up, it fixes the claims page issue but it causes my test plan to fail.
Comment 16 Pedro Amorim 2024-05-07 16:08:39 UTC
Created attachment 166303 [details] [review]
Bug 36774: [DONT PUSH] TEST ONLY

Test plan:
1) Apply only the TEST patch
2) Visit http://localhost:8081/cgi-bin/koha/serials/serials-home.pl
3) Click the "Expires before" input and insert any date
4) Click the "TEST" link under. It calls the native flatpickr.clear():
    https://flatpickr.js.org/instance-methods-properties-elements/#clear
5) Notice it clears the date value. But it also appends a new 'x' icon to clear the input.
6) Apply 2nd patch. Repeat. Notice it works as expected

Signed-off-by: David Nind <david@davidnind.com>
Comment 17 Pedro Amorim 2024-05-07 16:08:45 UTC
Created attachment 166304 [details] [review]
Bug 36774: Dont add wrapper if already exists

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 Pedro Amorim 2024-05-07 16:08:48 UTC
Created attachment 166305 [details] [review]
Bug 36774: (QA Follow-up): Use flatpickr clear

Im not sure why, but with the way it was before, there was a bug in my test plan.
If you cleared using the TEST link, and then input some date and clear with the 'x', a JS error would occurr.
Comment 19 Pedro Amorim 2024-05-07 16:13:27 UTC
(In reply to Jonathan Druart from comment #12)
> On main, if I revert 
>   commit 97a4b3c61da522f35628f80382d21bbff56b76ba
>   Bug 30718: Prevent 'reset' icon to appear twice
> 
> I am seeing: https://snipboard.io/XLKMVg.jpg on
> /cgi-bin/koha/serials/claims.pl?supplierid=1 KO (2 clears for "to")
> 
> 
> On main, I am seeing: https://snipboard.io/u728qI.jpg OK 1 clear for "from",
> 1 for "to"
> 
> On main + your patch: https://snipboard.io/6BusJT.jpg KO 1 clear for "from"
> but no clear for "to"

Thanks for this. I've submitted a squashed follow-up to the original patch and a follow-up to fix a bug I discovered while re-testing this, explanation in the commit message.
I had a hard time following your claims use case, but in the end I was able to and I think I discovered a bug in it as well. Filed bug 36804.
Comment 20 David Nind 2024-05-08 11:03:17 UTC
Created attachment 166383 [details] [review]
Bug 36774: [DONT PUSH] TEST ONLY

Test plan:
1) Apply only the TEST patch
2) Visit http://localhost:8081/cgi-bin/koha/serials/serials-home.pl
3) Click the "Expires before" input and insert any date
4) Click the "TEST" link under. It calls the native flatpickr.clear():
    https://flatpickr.js.org/instance-methods-properties-elements/#clear
5) Notice it clears the date value. But it also appends a new 'x' icon to clear the input.
6) Apply 2nd patch. Repeat. Notice it works as expected

Signed-off-by: David Nind <david@davidnind.com>
Comment 21 David Nind 2024-05-08 11:03:20 UTC
Created attachment 166384 [details] [review]
Bug 36774: Dont add wrapper if already exists

Signed-off-by: David Nind <david@davidnind.com>
Comment 22 David Nind 2024-05-08 11:03:24 UTC
Created attachment 166385 [details] [review]
Bug 36774: (QA Follow-up): Use flatpickr clear

Im not sure why, but with the way it was before, there was a bug in my test plan.
If you cleared using the TEST link, and then input some date and clear with the 'x', a JS error would occurr.

Signed-off-by: David Nind <david@davidnind.com>
Comment 23 Jonathan Druart 2024-05-08 12:19:15 UTC
Created attachment 166395 [details] [review]
Bug 36774: Dont add wrapper if already exists

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2024-05-08 12:19:18 UTC
Created attachment 166396 [details] [review]
Bug 36774: (QA Follow-up): Use flatpickr clear

Im not sure why, but with the way it was before, there was a bug in my test plan.
If you cleared using the TEST link, and then input some date and clear with the 'x', a JS error would occurr.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2024-05-08 12:20:22 UTC
It would be nice to have a cypress test for this, but not considering blocker.
Comment 26 Pedro Amorim 2024-05-08 14:00:34 UTC
There's a flaw in my code. It's changing the css class to flatpickr_wrapper_(id of the element), thus causing CSS rules to flatpickr_wrapper to not apply.
See issues caused at
<staff_url>/cgi-bin/koha/circ/overdue.pl

I opted for this approach intentionally in case the flatpickr input does not contain an id, but it's not great, need to revisit.
Comment 27 Pedro Amorim 2024-05-08 15:46:08 UTC
Created attachment 166423 [details] [review]
Bug 36774: [DONT PUSH] TEST ONLY

Test plan:
1) Apply only the TEST patch
2) Visit http://localhost:8081/cgi-bin/koha/serials/serials-home.pl
3) Click the "Expires before" input and insert any date
4) Click the "TEST" link under. It calls the native flatpickr.clear():
    https://flatpickr.js.org/instance-methods-properties-elements/#clear
5) Notice it clears the date value. But it also appends a new 'x' icon to clear the input.
6) Apply 2nd patch. Repeat. Notice it works as expected

Signed-off-by: David Nind <david@davidnind.com>
Comment 28 Pedro Amorim 2024-05-08 15:46:11 UTC
Created attachment 166424 [details] [review]
Bug 36774: Remove wrapper if already exists

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Pedro Amorim 2024-05-08 15:46:14 UTC
Created attachment 166425 [details] [review]
Bug 36774: Add cypress tests
Comment 30 Pedro Amorim 2024-05-08 15:50:11 UTC
Created attachment 166427 [details] [review]
Bug 36774: Add cypress tests

Inside k-t-d, run:
cypress run --spec t/cypress/integration/flatpickr_spec.ts
Comment 31 Pedro Amorim 2024-05-08 15:52:33 UTC
(In reply to Pedro Amorim from comment #18)
> Created attachment 166305 [details] [review] [review]
> Bug 36774: (QA Follow-up): Use flatpickr clear
> 
> Im not sure why, but with the way it was before, there was a bug in my test
> plan.
> If you cleared using the TEST link, and then input some date and clear with
> the 'x', a JS error would occurr.

I now know why, flatpickr().clear() is re-adding the 2nd input and triggering onReady but because onReady was returning early it was not re-wrapping the flatpickr_wrapper, causing this error.

In hindsight, I don't think we should be adding this clear button on onReady but use wrap from flatpickr instead:
https://flatpickr.js.org/examples/#flatpickr-external-elements
But I've invested enough time here and this is too much work for little gain.

Joubu, David, apologies for the back and forth here, this turned out to be a much deeper rabbit hole than I thought. I've added cypress tests (more tests on the claims page and patron hold tab could be added though) and went through the test plans in bug 34263 and bug 36804 again to make sure everything is working as expected, I believe it is.
Comment 32 Martin Renvoize 2024-05-09 14:41:01 UTC
Created attachment 166463 [details] [review]
Bug 36774: Remove wrapper if already exists

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2024-05-09 14:41:04 UTC
Created attachment 166464 [details] [review]
Bug 36774: Add cypress tests

Inside k-t-d, run:
cypress run --spec t/cypress/integration/flatpickr_spec.ts

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Katrin Fischer 2024-05-10 12:13:13 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 35 Fridolin Somers 2024-05-24 15:18:39 UTC
Pushed to 23.11.x for 23.11.06
Comment 36 Lucas Gass 2024-05-28 21:56:47 UTC
Backported to 23.05.x for upcoming 23.05.12