Bug 35157 - The searchfieldstype select element produces invalid HTML
Summary: The searchfieldstype select element produces invalid HTML
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Lucas Gass
QA Contact: Victor Grousset/tuxayo
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-10-25 20:53 UTC by Lucas Gass
Modified: 2023-12-14 21:06 UTC (History)
8 users (show)

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


Attachments
Bug 35157: Remove spans from patronfields.inc (8.91 KB, patch)
2023-10-25 21:04 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35157: Remove spans from patronfields.inc (8.95 KB, patch)
2023-10-27 22:05 UTC, David Nind
Details | Diff | Splinter Review
Bug 35157: Remove spans from patronfields.inc (11.17 KB, patch)
2023-11-01 23:06 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35157: Remove spans from patronfields.inc (11.21 KB, patch)
2023-11-17 20:20 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 35157: Remove spans from patronfields.inc (11.04 KB, patch)
2023-11-22 20:29 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass 2023-10-25 20:53:02 UTC
Run any page that contains the "Search patrons" search bar through an HTML validator and you will see a lot of:

Stray start tag span.
Stray end tag span.

This is because <option> tags contain spans. <span> is not a valid child element of <option>

The HTML5 spec:

https://html.spec.whatwg.org/multipage/form-elements.html#the-option-element


There are essentially two options for what can be put inside an <option> element:

1. Test
2. Nothing

These spans are added via the patronfields.inc
Comment 1 Lucas Gass 2023-10-25 21:04:28 UTC Comment hidden (obsolete)
Comment 2 Lucas Gass 2023-10-25 21:05:36 UTC
Katrin, is there something that can be done here to help improve translations?
Comment 3 David Nind 2023-10-27 22:05:17 UTC Comment hidden (obsolete)
Comment 4 David Nind 2023-10-27 23:03:53 UTC
Testing notes (using KTD):

1. I used the HTML Validator extension (https://www.gueury.com/mozilla/) with the Chromium browser, which shows under Developer Tools. (It is available on Firefox as well, but for some reason Firefox seemed to strip the <span> tags from the option element.)
Comment 5 Katrin Fischer 2023-10-28 12:46:52 UTC
As it stands, this will move all those words back into one single string, hurting translations. I believe we can use the same trick as in the reworked title tags instead. Have a look at bug 33077.

It would also be worth checking if we haven't added spans to other strings appearing inside options.

I think we only need to think of where to place the:
+[% PROCESS 'i18n.inc' %]
Comment 6 Lucas Gass 2023-11-01 23:04:53 UTC
(In reply to Katrin Fischer from comment #5)
> As it stands, this will move all those words back into one single string,
> hurting translations. I believe we can use the same trick as in the reworked
> title tags instead. Have a look at bug 33077.
> 
> It would also be worth checking if we haven't added spans to other strings
> appearing inside options.
> 
> I think we only need to think of where to place the:
> +[% PROCESS 'i18n.inc' %]

If my grepping is correct we need to make sure the i18n include is in the following files:

koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt:        [% INCLUDE 'cataloging-search.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbooks.tt:    [% INCLUDE 'cataloging-search.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt:    [% INCLUDE 'cataloging-search.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/cataloging-home.tt:        [% INCLUDE 'cataloging-search.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/concerns.tt:    [% INCLUDE 'cataloging-search.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/merge.tt:    [% INCLUDE 'cataloging-search.inc' %]q
koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/advsearch.tt:    [% INCLUDE 'adv-search.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/itemsearch.tt:    [% INCLUDE 'home-search.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt:        [% INCLUDE 'home-search.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/accountline-details.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/alert-subscriptions.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/apikeys.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/deletemem.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/discharge.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/discharges.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/files.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/holdshistory.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/housebound.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/ill-requests.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/mancredit.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/maninvoice.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/member-flags.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/member-password.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/member.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt:        [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/members-update.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/merge-patrons.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt:        [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/notices.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/purchase-suggestions.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/recallshistory.tt:        [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/routing-lists.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/statistics.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/members/two_factor_auth.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/tools/import_borrowers.tt:    [% INCLUDE 'patron-search-header.inc' %]
koha-tmpl/intranet-tmpl/prog/en/modules/tools/picture-upload.tt:    [% INCLUDE 'patron-search-header.inc' %]


So we need to add it to these 2 templates: 

koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt
koha-tmpl/intranet-tmpl/prog/en/modules/members/alert-subscriptions.tt
Comment 7 Lucas Gass 2023-11-01 23:06:25 UTC
Created attachment 158226 [details] [review]
Bug 35157: Remove spans from patronfields.inc

To test:
1. Run any page that contains the "Search patrons" search bar through an HTML validator and you will see a lot of:

        Stray start tag span.
        Stray end tag span.

2. Apply patch
3. Try step 1 again, no more errors from the HTML validator.
4. Make sure the patron searches still work when limiting by 'Search field'.
Comment 8 Brendan Lawlor 2023-11-17 20:20:54 UTC
Created attachment 159112 [details] [review]
Bug 35157: Remove spans from patronfields.inc

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 9 Brendan Lawlor 2023-11-17 20:30:49 UTC
I tested this using this plugin: https://chrispederick.com/work/web-developer/

before the patch there were 56 stray start and end tags found tag span. After the patch there were 0.

Just a note there was one other invalid html error and one warning found by the validator that maybe need another patch?

I ran the validator on http://localhost:8081/cgi-bin/koha/members/members-home.pl

Error: Stray end tag td.

From line 1010, column 39; to line 1010, column 43

Expires on</td>↩     

Warning: Empty heading.

From line 1037, column 17; to line 1037, column 64

          <h4 class="modal-title" id="patronPreviewLabel"></h4>↩
Comment 10 Victor Grousset/tuxayo 2023-11-22 16:52:22 UTC
(In reply to Lucas Gass from comment #6)
> If my grepping is correct we need to make sure the i18n include is in the
> following files:
> [...]
> So we need to add it to these 2 templates: 
> 
> koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt
> koha-tmpl/intranet-tmpl/prog/en/modules/members/alert-subscriptions.tt

After double checking, it's correct :)
In the last weeks, intranet-main.tt also got the i18n include from another ticket so it's end it's just alert-subscriptions.tt
Comment 11 Victor Grousset/tuxayo 2023-11-22 18:10:05 UTC
*in the end
Comment 12 Victor Grousset/tuxayo 2023-11-22 20:29:54 UTC
Created attachment 159197 [details] [review]
Bug 35157: Remove spans from patronfields.inc

To test:
1. Run any page that contains the "Search patrons" search bar through an HTML validator and you will see a lot of:

        Stray start tag span.
        Stray end tag span.

2. Apply patch
3. Try step 1 again, no more errors from the HTML validator.
4. Make sure the patron searches still work when limiting by 'Search field'.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 13 Victor Grousset/tuxayo 2023-11-22 20:30:50 UTC
Works, makes sense, QA script happy, code looks good, passing QA :)

(@Lucas: somehow your patch attached on comment 7 didn't have the test plan in the commit message. But it was only in the BZ comment. So it was lost on signoff.)a
Comment 14 Katrin Fischer 2023-12-11 18:46:52 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 15 Fridolin Somers 2023-12-12 07:29:14 UTC
Pushed to 23.11.x for 23.11.01