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
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
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 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 ?
(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.
(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.
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.
(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.
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.
Thanks a lot, Owen! Wouldn't mind if you want my patch out for simplicity.
(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.
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>
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>
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/:
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>
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>
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
Added a QA check: https://gitlab.com/koha-community/qa-test-tools/-/issues/44
Pushed to master for 20.11, thanks to everybody involved!
backported to 20.05.x for 20.05.06
backported to 19.11.x for 19.11.12
Not backported to oldoldstable (19.05.x). Feel free to ask if it's needed.