Bug 26727 - Fix <p/> appearing in the templates
Summary: Fix <p/> appearing in the templates
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: Katrin Fischer
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-10-18 16:29 UTC by Katrin Fischer
Modified: 2020-11-17 13:24 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06, 19.11.12


Attachments
Bug 26727: Fix <p/> appearing in the templates (5.10 KB, patch)
2020-10-18 16:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26727: Fix <p/> appearing in the templates (5.14 KB, patch)
2020-10-30 21:48 UTC, David Nind
Details | Diff | Splinter Review
Bug 26727: (follow-up) More markup corrections (17.01 KB, patch)
2020-11-03 18:15 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 26727: Fix <p/> appearing in the templates (5.19 KB, patch)
2020-11-03 22:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 26727: (follow-up) More markup corrections (17.06 KB, patch)
2020-11-03 22:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 26727: Fix <p/> appearing in the templates (5.17 KB, patch)
2020-11-05 13:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26727: (follow-up) More markup corrections (17.10 KB, patch)
2020-11-05 13:30 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 Katrin Fischer 2020-10-18 16:29:46 UTC
They don't appear to be valid in HTML5 and in some cases were actually typos.

The HTML5 validator doesn't like them:

Error: Self-closing syntax (/>) used on a non-void HTML element. Ignoring the slash and treating as a start tag.

From line 581, column 25; to line 581, column 28

          <p/>

See:
https://www.w3.org/TR/2010/WD-html-markup-20100624/p.html
Comment 1 Katrin Fischer 2020-10-18 16:36:30 UTC
Created attachment 111948 [details] [review]
Bug 26727: Fix <p/> appearing in the templates

The self-closing p is not valid in HTML5:

Error: Self-closing syntax (/>) used on a non-void HTML element.
Ignoring the slash and treating as a start tag.

To test:

1)
- Activate Article requests
- Search for a record to place an article request on in staff
- Look up the patron
- Verify the article request form looks ok
2)
- Search for a list of patrons
- Check 2 patrons and merge them using the button above the list
- Verify the distance between the button and the table looks ok
3)
- Create a new MARC modification template
- Create a new action
- Verify the distances between the first and second line of form
  elements looks ok
4)
- Place a hold in staff
- Click on the Suspend link in the holds table (not below)
- Verify the modal looks ok
Comment 2 David Nind 2020-10-30 21:48:49 UTC
Created attachment 112761 [details] [review]
Bug 26727: Fix <p/> appearing in the templates

The self-closing p is not valid in HTML5:

Error: Self-closing syntax (/>) used on a non-void HTML element.
Ignoring the slash and treating as a start tag.

To test:

1)
- Activate Article requests
- Search for a record to place an article request on in staff
- Look up the patron
- Verify the article request form looks ok
2)
- Search for a list of patrons
- Check 2 patrons and merge them using the button above the list
- Verify the distance between the button and the table looks ok
3)
- Create a new MARC modification template
- Create a new action
- Verify the distances between the first and second line of form
  elements looks ok
4)
- Place a hold in staff
- Click on the Suspend link in the holds table (not below)
- Verify the modal looks ok

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Martin Renvoize 2020-11-03 08:35:16 UTC
Comment on attachment 112761 [details] [review]
Bug 26727: Fix <p/> appearing in the templates

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
@@ +90,4 @@
>  
>                                  <fieldset class="rows">
>                                      <legend>Place article request from [% biblio.title | html %] for [% patron.firstname | html %] [% patron.surname | html %] ( [% patron.cardnumber | html %] )</legend>
> +                                    <p>

Hmm, not sure about this one.. won't it result in an unmatched tag? (i.e. I can't see a closing </p> anywhere beneath this opening <p>)

::: koha-tmpl/intranet-tmpl/prog/en/modules/tools/marc_modification_templates.tt
@@ +220,4 @@
>                                  </span>
>                              </span>
>  
> +                            <br><br>

Why a double <br> here compared to a single <br> in koha-tmpl/intranet-tmpl/prog/en/modules/members/merge-patrons.tt ?
Comment 4 Katrin Fischer 2020-11-03 12:06:29 UTC
(In reply to Martin Renvoize from comment #3)
> Comment on attachment 112761 [details] [review] [review]
> Bug 26727: Fix <p/> appearing in the templates
> 
> Review of attachment 112761 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
> @@ +90,4 @@
> >  
> >                                  <fieldset class="rows">
> >                                      <legend>Place article request from [% biblio.title | html %] for [% patron.firstname | html %] [% patron.surname | html %] ( [% patron.cardnumber | html %] )</legend>
> > +                                    <p>

We use HTML5 - we don't need closing <p>, see:

https://www.w3.org/TR/2010/WD-html-markup-20100624/p.html
A p element’s end tag may be omitted if the p element is immediately followed by an address, article, aside, blockquote, dir, div, dl, fieldset, footer, form, h1, h2, h3, h4, h5, h6, header, hr, menu, nav, ol, p, pre, section, table, or ul element, or if there is no more content in the parent element and the parent element is not an a element.

In this case, the following tag is <ul>.

> 
> Hmm, not sure about this one.. won't it result in an unmatched tag? (i.e. I
> can't see a closing </p> anywhere beneath this opening <p>)
> 
> :::
> koha-tmpl/intranet-tmpl/prog/en/modules/tools/marc_modification_templates.tt
> @@ +220,4 @@
> >                                  </span>
> >                              </span>
> >  
> > +                            <br><br>
> 
> Why a double <br> here compared to a single <br> in
> koha-tmpl/intranet-tmpl/prog/en/modules/members/merge-patrons.tt ?

I believe "looks". It was using a br br in another spot in the same file and I wanted to achieve the same 'distance'. It's not the cleanest way to do this, but I wanted to aim for making the HTML templates valid first.
Comment 5 Joonas Kylmälä 2020-11-03 13:56:13 UTC
(In reply to Katrin Fischer from comment #4)
> (In reply to Martin Renvoize from comment #3)
> > Comment on attachment 112761 [details] [review] [review] [review]
> > Bug 26727: Fix <p/> appearing in the templates
> > 
> > Review of attachment 112761 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt
> > @@ +90,4 @@
> > >  
> > >                                  <fieldset class="rows">
> > >                                      <legend>Place article request from [% biblio.title | html %] for [% patron.firstname | html %] [% patron.surname | html %] ( [% patron.cardnumber | html %] )</legend>
> > > +                                    <p>
> 
> We use HTML5 - we don't need closing <p>, see:
> 
> https://www.w3.org/TR/2010/WD-html-markup-20100624/p.html
> A p element’s end tag may be omitted if the p element is immediately
> followed by an address, article, aside, blockquote, dir, div, dl, fieldset,
> footer, form, h1, h2, h3, h4, h5, h6, header, hr, menu, nav, ol, p, pre,
> section, table, or ul element, or if there is no more content in the parent
> element and the parent element is not an a element.
> 
> In this case, the following tag is <ul>.

So then there is a paragraph with no content. That doesn't make any sense to me, in that case the paragraph tag should be just removed. If it is for styling reason it should be done in CSS instead of HTML.
Comment 6 Katrin Fischer 2020-11-03 14:00:30 UTC
I am out then, because I am not sure enough about best practices with the CSS. I think more developers are not as we will find a lot of <br> and similar if we start looking closer :)

Don't mistake me: I agree in general, but the goal here was just to fix the completely wrong tags.
Comment 7 Joonas Kylmälä 2020-11-03 14:09:03 UTC
(In reply to Katrin Fischer from comment #6)
> I am out then, because I am not sure enough about best practices with the
> CSS. I think more developers are not as we will find a lot of <br> and
> similar if we start looking closer :)
> 
> Don't mistake me: I agree in general, but the goal here was just to fix the
> completely wrong tags.

Ok, I'm inclined to mark this as FQA until there is a patch that fixes this issue in such a way that it won't cause another (smaller) problem.
Comment 8 Owen Leonard 2020-11-03 18:15:45 UTC
Created attachment 112945 [details] [review]
Bug 26727: (follow-up) More markup corrections

This patch makes more markup changes to the templates previously
modified in order to try to arrive at some agreeable solutions.

- In the value_builder EXAMPLE template, the <p> is unnecessary and is
  removed. Also removed is the script "type" attribute and obsolete
  CDATA markers.
- In the article request template an unnecessary <p> is removed.
- In the merge patrons template the submit button is now in a <fieldset
  class="action"> as is consistent with other forms.
- In the MARC modifications template I've added <div>s around each
  "line" in the form to add the line break, along with some CSS to add
  margins.
- The submit button is now inside a <fieldset class="action"> and the
  "Cancel" link now has the "cancel" class.
Comment 9 Katrin Fischer 2020-11-03 18:54:00 UTC
Thanks a lot, Owen! Wouldn't mind if you want my patch out for simplicity.
Comment 10 Katrin Fischer 2020-11-03 19:01:31 UTC
(In reply to Katrin Fischer from comment #9)
> Thanks a lot, Owen! Wouldn't mind if you want to throw my patch out for simplicity.
Comment 11 David Nind 2020-11-03 22:35:31 UTC
Created attachment 112961 [details] [review]
Bug 26727: Fix <p/> appearing in the templates

The self-closing p is not valid in HTML5:

Error: Self-closing syntax (/>) used on a non-void HTML element.
Ignoring the slash and treating as a start tag.

To test:

1)
- Activate Article requests
- Search for a record to place an article request on in staff
- Look up the patron
- Verify the article request form looks ok
2)
- Search for a list of patrons
- Check 2 patrons and merge them using the button above the list
- Verify the distance between the button and the table looks ok
3)
- Create a new MARC modification template
- Create a new action
- Verify the distances between the first and second line of form
  elements looks ok
4)
- Place a hold in staff
- Click on the Suspend link in the holds table (not below)
- Verify the modal looks ok

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2020-11-03 22:35:34 UTC
Created attachment 112962 [details] [review]
Bug 26727: (follow-up) More markup corrections

This patch makes more markup changes to the templates previously
modified in order to try to arrive at some agreeable solutions.

- In the value_builder EXAMPLE template, the <p> is unnecessary and is
  removed. Also removed is the script "type" attribute and obsolete
  CDATA markers.
- In the article request template an unnecessary <p> is removed.
- In the merge patrons template the submit button is now in a <fieldset
  class="action"> as is consistent with other forms.
- In the MARC modifications template I've added <div>s around each
  "line" in the form to add the line break, along with some CSS to add
  margins.
- The submit button is now inside a <fieldset class="action"> and the
  "Cancel" link now has the "cancel" class.

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2020-11-03 22:42:14 UTC
The updated forms and pages as per the test plan look okay to me - I checked using Firefox Developer Edition on Ubuntu 18.04.

I didn't do any HTML validation, but I think these two tools will be useful for future UI/accessibility work to help pick up and fix errors:

- WAVE Toolbar (for both Firefox and Google Chrome): https://wave.webaim.org/extension/
- HTML Validator (for Firefox): https://addons.mozilla.org/en-US/firefox/addon/html-validator/:
Comment 14 Martin Renvoize 2020-11-05 13:30:54 UTC
Created attachment 113110 [details] [review]
Bug 26727: Fix <p/> appearing in the templates

The self-closing p is not valid in HTML5:

Error: Self-closing syntax (/>) used on a non-void HTML element.
Ignoring the slash and treating as a start tag.

To test:

1)
- Activate Article requests
- Search for a record to place an article request on in staff
- Look up the patron
- Verify the article request form looks ok
2)
- Search for a list of patrons
- Check 2 patrons and merge them using the button above the list
- Verify the distance between the button and the table looks ok
3)
- Create a new MARC modification template
- Create a new action
- Verify the distances between the first and second line of form
  elements looks ok
4)
- Place a hold in staff
- Click on the Suspend link in the holds table (not below)
- Verify the modal looks ok

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2020-11-05 13:30:58 UTC
Created attachment 113111 [details] [review]
Bug 26727: (follow-up) More markup corrections

This patch makes more markup changes to the templates previously
modified in order to try to arrive at some agreeable solutions.

- In the value_builder EXAMPLE template, the <p> is unnecessary and is
  removed. Also removed is the script "type" attribute and obsolete
  CDATA markers.
- In the article request template an unnecessary <p> is removed.
- In the merge patrons template the submit button is now in a <fieldset
  class="action"> as is consistent with other forms.
- In the MARC modifications template I've added <div>s around each
  "line" in the form to add the line break, along with some CSS to add
  margins.
- The submit button is now inside a <fieldset class="action"> and the
  "Cancel" link now has the "cancel" class.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2020-11-05 13:31:57 UTC
Great teamwork here guys and gals, thanks very very much.

Passing QA

Note: This got me wondering whether there was a way we could run one of these validators as part of the test suit regularly...  food for thought
Comment 17 Jonathan Druart 2020-11-06 09:07:48 UTC
Added a QA check: https://gitlab.com/koha-community/qa-test-tools/-/issues/44
Comment 18 Jonathan Druart 2020-11-06 14:56:16 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 19 Lucas Gass 2020-11-16 22:15:54 UTC
backported to 20.05.x for 20.05.06
Comment 20 Aleisha Amohia 2020-11-17 05:37:37 UTC
backported to 19.11.x for 19.11.12
Comment 21 Victor Grousset/tuxayo 2020-11-17 13:24:57 UTC
Not backported to oldoldstable (19.05.x). Feel free to ask if it's needed.