Bug 13272 - Many inputs lack a type attribute
Summary: Many inputs lack a type attribute
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Target Milestone: ---
Assignee: M. Tompsett
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-11-17 17:36 UTC by Christopher Brannon
Modified: 2018-10-24 09:13 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13272: Adds type="text" to inputs missing it (20.23 KB, patch)
2018-09-16 22:08 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 13272: Followup on comment #5 (2.07 KB, patch)
2018-09-19 16:49 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 13272: Followup on tab issues (2.07 KB, patch)
2018-09-19 18:53 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 13272: Adds type="text" to inputs missing it (20.23 KB, patch)
2018-09-20 12:02 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 13272: Followup on comment #5 (2.07 KB, patch)
2018-09-20 12:02 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 13272: Followup on tab issues (2.07 KB, patch)
2018-09-20 12:02 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 13272: Follow-up adding other missing types (40.00 KB, patch)
2018-09-24 18:37 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 13272: Follow up add missing type="text" (20.35 KB, patch)
2018-09-26 22:58 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 13272: Adds type="text" to inputs missing it (20.28 KB, patch)
2018-09-28 13:37 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 13272: Followup on comment #5 (2.12 KB, patch)
2018-09-28 13:37 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 13272: Followup on tab issues (2.12 KB, patch)
2018-09-28 13:38 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 13272: Follow up add missing type="text" (20.40 KB, patch)
2018-09-28 13:38 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 13272: Adds type="text" to inputs missing it (20.45 KB, patch)
2018-09-29 20:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13272: (follow-up) fix concerns from comment #5 (2.23 KB, patch)
2018-09-29 20:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13272: (follow-up) Replace tabs with spaces (2.22 KB, patch)
2018-09-29 20:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13272: (follow-up) add missing type="text" (20.55 KB, patch)
2018-09-29 20:07 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Brannon 2014-11-17 17:36:02 UTC
While applying some CSS styling to Koha's textboxes, I notice that the Check in textbox on the Koha search bar (mainpage.pl, circulation-home.pl, circulation.pl, renew.pl, branchtransfers.pl) as well as the primary textbox on the Check in page (returns.pl) and the Renew page (renew.pl) is lacking the type="text" attribute, and is therefore inconsistent with the other textboxes.

I am also seeing this attribute missing from the barcode textbox on the Transfers page (branchtransfers.pl).

There may be other textboxes with the same issue.  It would be preferable to have these textboxes consistently attributed with type.

Christopher
Comment 1 Owen Leonard 2014-11-17 17:59:04 UTC
Using this regex: <input(?:\s+(?!type=)\w+="[^"]*")*\s*/>
(https://stackoverflow.com/questions/3472772/need-regex-for-all-html-input-tags-which-lack-a-specific-attribute) I found 23 matches in the staff client templates:

prog\en\includes\borrower_debarments.inc
prog\en\includes\cat-search.inc
prog\en\includes\circ-search.inc
prog\en\includes\home-search.inc
prog\en\includes\patron-search.inc
prog\en\modules\acqui\z3950_search.tt
prog\en\modules\admin\smart-rules.tt
prog\en\modules\cataloguing\z3950_auth_search.tt
prog\en\modules\cataloguing\z3950_search.tt
prog\en\modules\circ\branchtransfers.tt
prog\en\modules\circ\offline.tt
prog\en\modules\circ\renew.tt
prog\en\modules\circ\returns.tt
prog\en\modules\members\memberentrygen.tt
prog\en\modules\members\paycollect.tt
prog\en\modules\patroncards\members-search.tt
prog\en\modules\reserve\request.tt
prog\en\modules\tools\import_borrowers.tt

Looks like the type attribute isn't strictly required since it defaults to "text," but there's no reason why we shouldn't add an explicit type to enable consistent styling.
Comment 2 Christopher Brannon 2018-09-16 22:08:44 UTC
Created attachment 79018 [details] [review]
Bug 13272: Adds type="text" to inputs missing it
Comment 3 Christopher Brannon 2018-09-16 22:15:43 UTC Comment hidden (obsolete)
Comment 4 Christopher Brannon 2018-09-16 22:19:54 UTC
Sorry, didn't include the test plan.

TEST PLAN
1) Install the patch.
2) Track the changes in the patch and confirm that the change doesn't alter the behavior of the box or page.
Comment 5 M. Tompsett 2018-09-18 22:16:13 UTC
Comment on attachment 79018 [details] [review]
Bug 13272: Adds type="text" to inputs missing it

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

::: koha-tmpl/intranet-tmpl/prog/en/includes/borrower_debarments.inc
@@ +59,4 @@
>              <fieldset class="rows" id="manual_restriction_form">
>                  <legend>Add manual restriction</legend>
>                  <ol>
> +                    <li><label for="rcomment">Comment:</label> <input type="text" id="rcomment" name="comment" type="text" /></li>

Oops... duplicate type.

::: koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt
@@ +225,5 @@
>  	[% END %]
>  
>  	<li>
>          <label for="to">Hold expires on date:</label>
> +        <input name="expiration_date" id="to" size="10" class="datepickerto" / type="text" >

Oops... type="text" /> -- Though I don't think it is necessary (HTML5 vs XML thing, I believe)
Comment 6 Owen Leonard 2018-09-19 14:35:25 UTC
(In reply to M. Tompsett from comment #5)
> Oops... type="text" /> -- Though I don't think it is necessary (HTML5 vs XML
> thing, I believe)

FYI, Current practice is to follow XHTML rules for attributes ('disabled="disabled"') and self-closing tags.
Comment 7 M. Tompsett 2018-09-19 16:49:19 UTC
Created attachment 79120 [details] [review]
Bug 13272: Followup on comment #5

Correct the two issues I pointed out.
Comment 8 Owen Leonard 2018-09-19 17:33:03 UTC
These patches look good, but there are a couple of warnings from the QA script that need to be addressed:

 FAIL   koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
   FAIL   forbidden patterns
                forbidden pattern: tab char (line 614)
                forbidden pattern: tab char (line 616)
                forbidden pattern: tab char (line 618)


Processing additional checks

        * Commit title does not contain 'follow-up' correctly spelt - 76008a7
Comment 9 M. Tompsett 2018-09-19 18:53:22 UTC
Created attachment 79123 [details] [review]
Bug 13272: Followup on tab issues

Cleaned up whitespace issues tab vs. space in template.
After applying all patches:
    git diff -w origin/master
For the koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
only the changed type="text" lines will show, but without the
-w, there will be more whitespace changes.
Comment 10 Owen Leonard 2018-09-20 12:02:41 UTC
Created attachment 79162 [details] [review]
Bug 13272: Adds type="text" to inputs missing it
Comment 11 Owen Leonard 2018-09-20 12:02:44 UTC
Created attachment 79163 [details] [review]
Bug 13272: Followup on comment #5

Correct the two issues I pointed out.
Comment 12 Owen Leonard 2018-09-20 12:02:48 UTC
Created attachment 79164 [details] [review]
Bug 13272: Followup on tab issues

Cleaned up whitespace issues tab vs. space in template.
After applying all patches:
    git diff -w origin/master
For the koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
only the changed type="text" lines will show, but without the
-w, there will be more whitespace changes.
Comment 13 Jonathan Druart 2018-09-21 17:19:52 UTC
Reusing the regex from comment 1 I still found several occurrences left:

git grep -P '<input(?:\s+(?!type=)\w+="[^"]*")*\s*/>' **/*.tt **/*.inc 

I think we should fix all of them and add a test (I am willing to write the test if needed).
Comment 14 Christopher Brannon 2018-09-24 17:42:40 UTC
Owen, I owe you an apology.  I know I posted the bug, but I just noticed you had claimed this one.  I didn't mean to hijack this.  Do you want to continue with it, or do you want me to keep running with it?
Comment 15 Owen Leonard 2018-09-24 18:03:36 UTC
I didn't claim it. I'm the default assignee for the template component. Since it was never marked "assigned" to anyone it was free for the taking.

As a rule, though, you should assign bugs to yourself if you're submitting a patch for it.
Comment 16 Christopher Brannon 2018-09-24 18:09:41 UTC
I will assign it to myself then.  Thanks for getting back to me.
Comment 17 M. Tompsett 2018-09-24 18:37:08 UTC
Created attachment 79333 [details] [review]
Bug 13272: Follow-up adding other missing types

barcodes are clearly type="text".
datepicker stuff... guessing type="text".
Modified based on the grep expression in comment #13.
Comment 18 M. Tompsett 2018-09-26 22:58:45 UTC
Created attachment 79464 [details] [review]
Bug 13272: Follow up add missing type="text"

This addresses comment #13.
This also applies cleanly.
Comment 19 M. Tompsett 2018-09-26 23:00:49 UTC
Sorry if I stepped on anyone's toes.
Comment 20 Christopher Brannon 2018-09-26 23:10:38 UTC
(In reply to M. Tompsett from comment #19)
> Sorry if I stepped on anyone's toes.

I was trying to figure out what was going on, since I was working on this.  :)  I'm just going to assign it to you, since you are running with it more than I can at the moment.
Comment 21 M. Tompsett 2018-09-27 02:15:19 UTC
(In reply to Christopher Brannon from comment #20)
> (In reply to M. Tompsett from comment #19)
> > Sorry if I stepped on anyone's toes.
> 
> I was trying to figure out what was going on, since I was working on this. 
> :)  I'm just going to assign it to you, since you are running with it more
> than I can at the moment.

Feel free to sign off then. :)
Comment 22 Owen Leonard 2018-09-28 13:37:50 UTC
Created attachment 79593 [details] [review]
Bug 13272: Adds type="text" to inputs missing it

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 23 Owen Leonard 2018-09-28 13:37:56 UTC
Created attachment 79594 [details] [review]
Bug 13272: Followup on comment #5

Correct the two issues I pointed out.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 24 Owen Leonard 2018-09-28 13:38:00 UTC
Created attachment 79595 [details] [review]
Bug 13272: Followup on tab issues

Cleaned up whitespace issues tab vs. space in template.
After applying all patches:
    git diff -w origin/master
For the koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
only the changed type="text" lines will show, but without the
-w, there will be more whitespace changes.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 25 Owen Leonard 2018-09-28 13:38:04 UTC
Created attachment 79596 [details] [review]
Bug 13272: Follow up add missing type="text"

This addresses comment #13.
This also applies cleanly.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 26 Jonathan Druart 2018-09-29 20:07:10 UTC
Created attachment 79622 [details] [review]
Bug 13272: Adds type="text" to inputs missing it

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2018-09-29 20:07:14 UTC
Created attachment 79623 [details] [review]
Bug 13272: (follow-up) fix concerns from comment #5

Correct the two issues I pointed out.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2018-09-29 20:07:18 UTC
Created attachment 79624 [details] [review]
Bug 13272: (follow-up) Replace tabs with spaces

Cleaned up whitespace issues tab vs. space in template.
After applying all patches:
    git diff -w origin/master
For the koha-tmpl/intranet-tmpl/prog/en/modules/circ/returns.tt
only the changed type="text" lines will show, but without the
-w, there will be more whitespace changes.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2018-09-29 20:07:23 UTC
Created attachment 79625 [details] [review]
Bug 13272: (follow-up) add missing type="text"

This addresses comment #13.
This also applies cleanly.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Nick Clemens 2018-10-01 16:31:57 UTC
Awesome work all!

Pushed to master for 18.11
Comment 31 Martin Renvoize 2018-10-05 15:36:51 UTC
Pushed to 18.05.x for 18.05.05
Comment 32 Fridolin SOMERS 2018-10-11 12:59:18 UTC
Pushed to 17.11.x for 17.11.11