Bug 25954

Summary: Header search forms should be labeled
Product: Koha Reporter: Owen Leonard <oleonard>
Component: TemplatesAssignee: Owen Leonard <oleonard>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: minor    
Priority: P5 - low CC: andrewfh, bolshawh, caroline.cyr-la-rose, fridolin.somers, jonathan.druart, josef.moravec, lucas, martin.renvoize, victor
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,20.05.08
Bug Depends on:    
Bug Blocks: 27703, 27713    
Attachments: Bug 25954: Replace paragraph tags with labels in header search forms
Bug 25954: Replace paragraph tags with labels in header search forms
Bug 25954: Replace paragraph tags with labels in header search forms
Bug 25954: (Follow-up) Correct label "for" attribute
Bug 25954: (Follow-up) Correct label "for" attribute
Bug 25954: Replace paragraph tags with labels in header search forms
Bug 25954: (follow-up) Correct label "for" attribute
Bug 25954: (QA follow-up) Correct label "for" attribute in authorities search
Bug 25954: Replace paragraph tags with labels in header search forms
Bug 25954: (follow-up) Correct label "for" attribute
Bug 25954: (QA follow-up) Correct label "for" attribute in authorities search
Bug 25954: Prevent two elements with id="description" in the DOM

Description Owen Leonard 2020-07-08 13:36:34 UTC
The markup for the header search forms in the staff interface use a <p> tag to show descriptive text about the form. For instance:

<p class="tip">Enter patron card number or partial name:</p>

In most cases this can be replaced with a label:

<label class="tip" for="findborrower">Enter patron card number or partial name:</label>

This will improve accessibility and eliminate errors raised by WCAG checks.
Comment 1 Owen Leonard 2020-07-08 15:39:34 UTC Comment hidden (obsolete)
Comment 2 Martin Renvoize 2020-08-06 14:28:28 UTC
Created attachment 107896 [details] [review]
Bug 25954: Replace paragraph tags with labels in header search forms

This patch replaces instances of <p class="tip"> with <label
class="tip"> in header search forms where there is not already a label
tag corresponding to the first form field.

In cases where there is already a label on the first form field the <p>
is kept.

To test, apply the patch and rebuild the staff client CSS
(https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client)

Test the header search forms on the following pages:

 - Staff interface home page
 - Acquisitions home
 - Acquisitions -> Vendor -> Contracts
 - Acquisitions -> Vendor -> Add to basket -> From a suggestion
 - Advanced catalog search
 - Authorities
 - Administration
 - Administration -> Funds
 - Administration -> Desks (UseCirculationDesks must be enabled)
 - Administration -> Cities and towns
 - Administration -> Currencies and exchange rates
 - Administration -> Patron categories
 - Administration -> Z39.50/SRU servers
 - Catalog -> Search results
 - Cataloging
 - Circulation
 - Circulation -> Check in
 - Patrons
 - Serials
 - Tools -> Notices and Slips

In each case, check that the search form labels work as expected to move
focus to the corresponding form field.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2020-08-06 14:28:51 UTC
Excellent work, all looks good to me..

Signing off
Comment 4 Josef Moravec 2020-08-06 15:17:59 UTC
Comment on attachment 107896 [details] [review]
Bug 25954: Replace paragraph tags with labels in header search forms

Review of attachment 107896 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/includes/cat-search.inc
@@ +28,3 @@
>      <form action="/cgi-bin/koha/catalogue/search.pl"  method="get" id="cat-search-block">
> +        [% IF ( Koha.Preference('IntranetCatalogSearchPulldown') ) %]
> +            <label class="tip" for="idx">Enter search keywords:</label>

I am not sure about for="idx", I think the label is still for "search-form" not for indices select...
Comment 5 Lucas Gass 2020-08-06 16:10:10 UTC Comment hidden (obsolete)
Comment 6 Lucas Gass 2020-08-06 16:12:15 UTC
Ignore my sign off, I tested this and it all looks good to me, I ran each page through a WCAG checker.

I did not notice Josef's comment.
Comment 7 Owen Leonard 2020-08-06 19:13:43 UTC
Created attachment 107915 [details] [review]
Bug 25954: (Follow-up) Correct label "for" attribute

This patch updates a couple of search include forms to change the "for"
attribute of the label. Although "idx" is the first field in this
situation, the label's text applies to the keyword field so it should
give focus to the keyword field.
Comment 8 Josef Moravec 2020-08-06 19:22:44 UTC
Comment on attachment 107915 [details] [review]
Bug 25954: (Follow-up) Correct label "for" attribute

Review of attachment 107915 [details] [review]:
-----------------------------------------------------------------

Sorry being so pedantive...

::: koha-tmpl/intranet-tmpl/prog/en/includes/cat-search.inc
@@ +27,4 @@
>  <div id="catalog_search" class="residentsearch">
>      <form action="/cgi-bin/koha/catalogue/search.pl"  method="get" id="cat-search-block">
>          [% IF ( Koha.Preference('IntranetCatalogSearchPulldown') ) %]
> +            <label class="tip" for="search-form">Enter search keywords:</label>

The label could be before IF and you do not need the ELSE branch then...

also there are some more occurences, see:
git grep 'for="idx"'
Comment 9 Josef Moravec 2020-08-06 19:25:08 UTC
So it need probably more refactoring...

whole form with id "cat-search-block" could be in its own template?
Comment 10 Owen Leonard 2020-08-06 23:52:51 UTC
(In reply to Josef Moravec from comment #9)
> So it need probably more refactoring...

This patch was what prompted me to start the conversation on koha-devel about consolidating the staff interface header search forms.
Comment 11 Katrin Fischer 2020-08-07 08:40:04 UTC
(In reply to Owen Leonard from comment #10)
> (In reply to Josef Moravec from comment #9)
> > So it need probably more refactoring...
> 
> This patch was what prompted me to start the conversation on koha-devel
> about consolidating the staff interface header search forms.

I still like the idea - because it means we have less files to change something when needed.
Comment 12 Josef Moravec 2020-08-07 08:52:00 UTC
(In reply to Owen Leonard from comment #10)
> (In reply to Josef Moravec from comment #9)
> > So it need probably more refactoring...
> 
> This patch was what prompted me to start the conversation on koha-devel
> about consolidating the staff interface header search forms.

I see, sorry I missed the whole conversation on koha-devel

So, I think we could go with this patches and refactoring on another bug report?

Looks like, there is no clear consensus on that conversation. I do not have strong opinion on that other that it needs to be refactored somehow... But I could imagine one big file, or WRAPPER (as mantioned by Julian) approach...
Comment 13 Owen Leonard 2020-08-10 12:48:32 UTC
Created attachment 107986 [details] [review]
Bug 25954: (Follow-up) Correct label "for" attribute

This patch updates a couple of search include forms to change the "for"
attribute of the label. Although "idx" is the first field in this
situation, the label's text applies to the keyword field so it should
give focus to the keyword field.
Comment 14 Katrin Fischer 2020-08-10 20:24:29 UTC
With the patch applied some of the labels jump in front of the search field while others remain above, an example would be the administration module. I did run yarn build and restart_all.
Comment 15 Owen Leonard 2020-11-05 12:27:27 UTC
(In reply to Katrin Fischer from comment #14)
> With the patch applied some of the labels jump in front of the search field
> while others remain above

Could you look at this again? The problem you describe would happen if the CSS hasn't been regenerated. I know you said you did, but I'm hoping it was a glitch.
Comment 16 Josef Moravec 2020-11-17 17:39:27 UTC
Created attachment 113747 [details] [review]
Bug 25954: Replace paragraph tags with labels in header search forms

This patch replaces instances of <p class="tip"> with <label
class="tip"> in header search forms where there is not already a label
tag corresponding to the first form field.

In cases where there is already a label on the first form field the <p>
is kept.

To test, apply the patch and rebuild the staff client CSS
(https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client)

Test the header search forms on the following pages:

 - Staff interface home page
 - Acquisitions home
 - Acquisitions -> Vendor -> Contracts
 - Acquisitions -> Vendor -> Add to basket -> From a suggestion
 - Advanced catalog search
 - Authorities
 - Administration
 - Administration -> Funds
 - Administration -> Desks (UseCirculationDesks must be enabled)
 - Administration -> Cities and towns
 - Administration -> Currencies and exchange rates
 - Administration -> Patron categories
 - Administration -> Z39.50/SRU servers
 - Catalog -> Search results
 - Cataloging
 - Circulation
 - Circulation -> Check in
 - Patrons
 - Serials
 - Tools -> Notices and Slips

In each case, check that the search form labels work as expected to move
focus to the corresponding form field.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 17 Josef Moravec 2020-11-17 17:39:32 UTC
Created attachment 113748 [details] [review]
Bug 25954: (follow-up) Correct label "for" attribute

This patch updates a couple of search include forms to change the "for"
attribute of the label. Although "idx" is the first field in this
situation, the label's text applies to the keyword field so it should
give focus to the keyword field.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 18 Josef Moravec 2020-11-17 17:39:36 UTC
Created attachment 113749 [details] [review]
Bug 25954: (QA follow-up) Correct label "for" attribute in authorities search

Change it from first select box to text input for consistancy with other
search forms

Test plan:
1) Apply the patch
2) Go to authorities search
3) Confirm the labels in all search tabs are linked to text input field

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 19 Josef Moravec 2020-11-17 17:40:43 UTC
I would consider it PQA, but would like to somebody to look into my follow-up to ensure it does make sense.
Comment 20 Nick Clemens 2020-12-23 15:52:26 UTC
Created attachment 114681 [details] [review]
Bug 25954: Replace paragraph tags with labels in header search forms

This patch replaces instances of <p class="tip"> with <label
class="tip"> in header search forms where there is not already a label
tag corresponding to the first form field.

In cases where there is already a label on the first form field the <p>
is kept.

To test, apply the patch and rebuild the staff client CSS
(https://wiki.koha-community.org/wiki/Working_with_SCSS_in_the_OPAC_and_staff_client)

Test the header search forms on the following pages:

 - Staff interface home page
 - Acquisitions home
 - Acquisitions -> Vendor -> Contracts
 - Acquisitions -> Vendor -> Add to basket -> From a suggestion
 - Advanced catalog search
 - Authorities
 - Administration
 - Administration -> Funds
 - Administration -> Desks (UseCirculationDesks must be enabled)
 - Administration -> Cities and towns
 - Administration -> Currencies and exchange rates
 - Administration -> Patron categories
 - Administration -> Z39.50/SRU servers
 - Catalog -> Search results
 - Cataloging
 - Circulation
 - Circulation -> Check in
 - Patrons
 - Serials
 - Tools -> Notices and Slips

In each case, check that the search form labels work as expected to move
focus to the corresponding form field.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 21 Nick Clemens 2020-12-23 15:52:29 UTC
Created attachment 114682 [details] [review]
Bug 25954: (follow-up) Correct label "for" attribute

This patch updates a couple of search include forms to change the "for"
attribute of the label. Although "idx" is the first field in this
situation, the label's text applies to the keyword field so it should
give focus to the keyword field.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 22 Nick Clemens 2020-12-23 15:52:33 UTC
Created attachment 114683 [details] [review]
Bug 25954: (QA follow-up) Correct label "for" attribute in authorities search

Change it from first select box to text input for consistancy with other
search forms

Test plan:
1) Apply the patch
2) Go to authorities search
3) Confirm the labels in all search tabs are linked to text input field

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 23 Jonathan Druart 2021-01-04 13:10:21 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 24 Jonathan Druart 2021-01-04 14:06:28 UTC
Owen, the following changes is causing t/db_dependent/selenium/administration_tasks.t to fail:

diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/patrons-admin-search.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/patrons-admin-search.inc
index a77b92c6d5..8640c2268e 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/includes/patrons-admin-search.inc
+++ b/koha-tmpl/intranet-tmpl/prog/en/includes/patrons-admin-search.inc
@@ -3,18 +3,22 @@
 <h1 id="logo"><a href="/cgi-bin/koha/mainpage.pl">[% LibraryName | html %]</a></h1><!-- Begin Patrons Admin Resident Search Box -->
 <div id="header_search">
        <div id="patron_category_search" class="residentsearch">
-    <p class="tip">Search by patron category name:</p>
        <form action="[% script_name | html %]" method="post">
-        <input class="head-searchbox" type="text" name="description" size="40" value="[% searchfield | html %]" />
+        <label class="tip" for="description">Search by patron category name:</label>
+        <input class="head-searchbox" type="text" name="description" id="description" size="40" value="[% searchfield | html %]" />


There are now 2 elements with id="description" in the DOM.
Comment 25 Jonathan Druart 2021-01-04 14:08:47 UTC
The following patch would make the tests pass again.

-        <input class="head-searchbox" type="text" name="description" id="description" size="40" value="[% searchfield | html %]" />
+        <input class="head-searchbox" type="text" name="description" size="40" value="[% searchfield | html %]" />
Comment 26 Jonathan Druart 2021-01-04 14:12:05 UTC
Created attachment 114790 [details] [review]
Bug 25954: Prevent two elements with id="description" in the DOM

It also fixes t/db_dependent/selenium/administration_tasks.t
Comment 27 Jonathan Druart 2021-01-04 15:35:02 UTC
(In reply to Jonathan Druart from comment #26)
> Created attachment 114790 [details] [review] [review]
> Bug 25954: Prevent two elements with id="description" in the DOM
> 
> It also fixes t/db_dependent/selenium/administration_tasks.t

Pushed to master for 21.05.00
Comment 28 Fridolin Somers 2021-01-07 14:06:52 UTC
Pushed to 20.11.x for 20.11.02
Comment 29 Fridolin Somers 2021-01-07 14:07:44 UTC
Note that there is no change in appearance.
Comment 30 Andrew Fuerste-Henry 2021-01-11 22:21:59 UTC
Pushed to 20.08.x for 20.05.08
Comment 31 Victor Grousset/tuxayo 2021-01-21 19:50:21 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.