Bug 27336

Summary: JS error in Administration - System preferences page
Product: Koha Reporter: Lucas Gass <lucas>
Component: Staff interfaceAssignee: Julian Maurice <julian.maurice>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andrewfh, caroline.cyr-la-rose, dcook, fridolin.somers, gmcharlt, julian.maurice
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.02
Attachments: Bug 27336: Fix JS error on preferences.pl
Bug 27336: Fix JS error on preferences.pl
Bug 27336 (alternative): Sanitize correctly HTML id
Bug 27336: (alternative) Sanitize correctly HTML id
Bug 27336: (alternative) Sanitize correctly HTML id
Bug 27336: Sanitize correctly HTML id
Bug 27336: Add POD for new package
Bug 27336: Fix xt/find-missing-filters.t

Description Lucas Gass 2021-01-04 23:09:47 UTC
In the system preference there is a console error when you select the 'Searching' tab or go to:

http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=searching


Uncaught Error: Syntax error, unrecognized expression: #collapse_Did_you_mean/spell_checking
Comment 1 Lucas Gass 2021-01-04 23:30:49 UTC
Created attachment 114826 [details] [review]
Bug 27336: Fix JS error on preferences.pl

To test:
1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=searching
2. If you are on the "Searching" tab you will see a console error
3. Apply patch
4. Make sure the error is gone
Comment 2 Lucas Gass 2021-01-04 23:34:23 UTC
This error is caused by the line 'Did you mean/spell checking:' in searching.pref

The '/' is the problem so I changed the line to 'Did you mean and spell checking'
Comment 3 David Cook 2021-01-05 01:52:37 UTC
Created attachment 114830 [details] [review]
Bug 27336: Fix JS error on preferences.pl

To test:
1. Go to http://localhost:8081/cgi-bin/koha/admin/preferences.pl?tab=searching
2. If you are on the "Searching" tab you will see a console error
3. Apply patch
4. Make sure the error is gone

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 4 David Cook 2021-01-05 01:54:59 UTC
I don't love this solution, since it's more of a workaround than a fix, but it solves the problem.

Another solution would've been to further sanitize the ID in preferences.tt but that's also a bit of a pain to do comprehensively.
Comment 5 David Cook 2021-01-05 02:40:24 UTC
It looks like HTML 4 was strict about the value of ID (https://www.w3.org/TR/html4/types.html#type-id). However, HTML 5 doesn't seem to be (https://www.w3.org/TR/2011/WD-html5-20110525/elements.html#the-id-attribute).

It looks like the issue arises in the jquery-2.2.3 library in fa.tokenize. Reading the minified JS even with pretty print turned on is a bit of a nightmare. I feel like Chrome might not be processing things quite right in the debugger. 

In any case, Jquery and CSS2 get more specific about what they allow and do not allow:

https://api.jquery.com/category/selectors/
https://www.w3.org/TR/CSS21/syndata.html#value-def-identifier

Wow CSS specs are a bigger mess than one would think...

For CSS3... I can't make head or tail of the spec in terms of allowed characters. It says how to escape characters but that's about it (https://www.w3.org/TR/css-syntax-3/#escaping). This is vague: https://www.w3.org/TR/css-syntax-3/#typedef-ident-token

"Property names and at-rule names are always identifiers, which have to start with a letter or a hyphen followed by a letter, and then can contain letters, numbers, hyphens, or underscores. You can include any code point at all, even ones that CSS uses in its syntax, by escaping it."

https://www.w3.org/TR/css-syntax-3/#syntax-description

So I suppose another solution would be to implement a template toolkit for CSS selectors...

Anyway, enough of this digression... heh
Comment 6 Julian Maurice 2021-01-08 10:10:34 UTC
Created attachment 114949 [details] [review]
Bug 27336 (alternative): Sanitize correctly HTML id

Valid characters are alphanumeric characters (a-zA-Z0-9), hyphen (-) and
underscore (_)

https://www.w3.org/TR/CSS21/syndata.html#value-def-identifier

All invalid characters will be replaced by '_'

Test plan:
1. Go to Administration » System preferences and click on 'Searching'
tab
2. You should see a console error (Uncaught Error: Syntax error,
unrecognized expression: #collapse_Did_you_mean/spell_checking)
3. Apply patch
4. Make sure the error is gone
5. prove t/Koha_Template_Plugin_HtmlId.t
Comment 7 Julian Maurice 2021-01-08 10:11:22 UTC
Instead of working around the bug, let's fix it.
Back to needs signoff
Comment 8 Owen Leonard 2021-01-08 11:48:27 UTC
Created attachment 114952 [details] [review]
Bug 27336: (alternative) Sanitize correctly HTML id

Valid characters are alphanumeric characters (a-zA-Z0-9), hyphen (-)
and underscore (_)

https://www.w3.org/TR/CSS21/syndata.html#value-def-identifier

All invalid characters will be replaced by '_'

Test plan:
1. Go to Administration » System preferences and click on 'Searching'
   tab
2. You should see a console error (Uncaught Error: Syntax error,
   unrecognized expression: #collapse_Did_you_mean/spell_checking)
3. Apply patch
4. Make sure the error is gone
5. prove t/Koha_Template_Plugin_HtmlId.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Owen Leonard 2021-01-08 11:53:24 UTC
I think Julian's patch is a better long-term solution. The QA tool will need to be updated to recognize the new filter.
Comment 10 Lucas Gass 2021-01-08 15:13:49 UTC
(In reply to Owen Leonard from comment #9)
> I think Julian's patch is a better long-term solution. The QA tool will need
> to be updated to recognize the new filter.

Agreed, and thank you Julian!
Comment 11 Nick Clemens 2021-01-08 18:50:42 UTC
Created attachment 114975 [details] [review]
Bug 27336: (alternative) Sanitize correctly HTML id

Valid characters are alphanumeric characters (a-zA-Z0-9), hyphen (-)
and underscore (_)

https://www.w3.org/TR/CSS21/syndata.html#value-def-identifier

All invalid characters will be replaced by '_'

Test plan:
1. Go to Administration » System preferences and click on 'Searching'
   tab
2. You should see a console error (Uncaught Error: Syntax error,
   unrecognized expression: #collapse_Did_you_mean/spell_checking)
3. Apply patch
4. Make sure the error is gone
5. prove t/Koha_Template_Plugin_HtmlId.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Jonathan Druart 2021-01-11 09:23:10 UTC
Created attachment 115006 [details] [review]
Bug 27336: Sanitize correctly HTML id

Valid characters are alphanumeric characters (a-zA-Z0-9), hyphen (-)
and underscore (_)

https://www.w3.org/TR/CSS21/syndata.html#value-def-identifier

All invalid characters will be replaced by '_'

Test plan:
1. Go to Administration » System preferences and click on 'Searching'
   tab
2. You should see a console error (Uncaught Error: Syntax error,
   unrecognized expression: #collapse_Did_you_mean/spell_checking)
3. Apply patch
4. Make sure the error is gone
5. prove t/Koha_Template_Plugin_HtmlId.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Jonathan Druart 2021-01-11 09:23:13 UTC
Created attachment 115007 [details] [review]
Bug 27336: Add POD for new package
Comment 14 Jonathan Druart 2021-01-11 09:23:16 UTC
Created attachment 115008 [details] [review]
Bug 27336: Fix xt/find-missing-filters.t
Comment 15 Jonathan Druart 2021-01-11 09:30:16 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 16 Fridolin Somers 2021-01-11 13:05:32 UTC
Pushed to 20.11.x for 20.11.02
Comment 17 Andrew Fuerste-Henry 2021-01-20 01:17:40 UTC
I don't see the reported JS error in 20.05. Not backported. Please ask if required.