Closed Bug 129442 Opened 23 years ago Closed 23 years ago

Make HTML of a default installation HTML W3C compliant

Categories

(Bugzilla :: Bugzilla-General, defect, P1)

2.15
defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: chema, Assigned: bbaetz)

References

()

Details

Attachments

(1 file, 3 obsolete files)

Make HTML of bugzilla HTML Transitional 4.01 compliant, Patch attached. Before : http://www.gnome.org/~chema/html_verify_bugzilla_before.html After: http://www.gnome.org/~chema/html_verify_bugzilla_after.html regards, Chema
Attached patch Patch (obsolete) — Splinter Review
Forgot to add that it is a diff against the CVS.
Thanks for the patch, but this is already being handled in bug 47251.
*** This bug has been marked as a duplicate of 47251 ***
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → DUPLICATE
Chema - thanks for your effort, but it might have been better to check Bugzilla or ask in the newsgroup before doing all that work :-) We are already dealing with this issue as part of our templatisation effort, so (unless some of your patches are to templates) I'm afraid all your work is going to get obsoleted :-( Gerv
did anyone look at the patch? :-) It's against current CVS (within a day or two) and yes, he did patch templates. :-)
Blocks: 47251
Severity: major → blocker
Status: RESOLVED → REOPENED
Keywords: patch, review
Priority: -- → P1
Resolution: DUPLICATE → ---
Target Milestone: --- → Bugzilla 2.16
Chema just saved us a whole bunch of work guys. Can we get a few of the HTML gurus to check out his patch? He did patch against current CVS, and made changes to a number of template files.
Really? Cool :-) I'll try and look at this this evening, and we'll speed it in so it doesn't rot. Thanks, mate! Gerv
Yeah, it patches templates. It started out as a "can't sleep hack" to add automatic HTML verfication to the HTML generated by bugzilla something in the lines of ./checksetup.pl so that it could verify other templates too, as the night progressed i realize that the task was bigger than a 2-3 day hack for a number of reasons so i eneded up fixing the html and templates the verificator I was using would find. It still won't find all the issues because the HTML is fetched without cookies and thus wihout being logged in (complication #1) and also it couln't find pages where the wasn't a link to (since it was a spider). This hack will also require us to simulate POST methods (complication #2) for it to work, like adding or modifying a bug, or doing a query. And finally, I also was unable to find a HTML verficator that anybody could easily install as a perl module or something (complication #3), the perl module HTML:Verificator depends on a weird sgml utility which was a bitch to install.
Most of it seems to be removing the trailing slashes which is a retrograde step IMO. They are there for XHTML compatibility and AFAIK have no validation problems (and I'm pretty sure I've tested that).
Comment on attachment 72946 [details] [diff] [review] Patch I looked through the patch, and there are a lot of good changes in it. Just one question: why remove the trailing slashes? Don't we want these for future XHTML compatibility?
The meaning of an ending slash in empty element tags is differs greatly between XML and SGML. I'll leave the technical details out, but adding X(HT)ML-style slashes to HTML documents will kill validators. It will work in browsers, especially if you precede the slashes with spaces (<hr /> instead of <hr/>), but this just because the mainstream browsers do not really parse SGML by the rules. Just like XHTML is not truly backwards compatible - if the browsers did use a formally correct SGML parser for HTML, XHTML documents would never work. This time the lack of conformance just happened to bring us compatibility. So if the patch removes the ending slashes, it's correct. You can't use the <br /> syntax in a valid HTML document. There is no such thing as partial migration: if you want it in XHTML, you will have to do it all at once. Of course, it's wise to prepare by doing those changes you can (without breaking validation): use lowercase element and attribute names, quote all attribute values and so on.
We have pretty much done all the XHTML compatibility, we just need to change doctypes I think.
Practically speaking XHTML compatibility is more useful than SGML compatibility because folks are far more likely to be using XML parsers than SGML parsers with Bugzilla output. Thus if we have to choose between them we should implement XHTML instead of SGML compatibility.
Comment on attachment 72946 [details] [diff] [review] Patch Yes. IMO, Bugzilla's HTML should meet the following goals, in order: 1) Works well in as many browsers as possible 2) Is compatible with HTML 4.01 Transitional 3) Is XHTML-ready Passing through the W3C Validator, as an end in itself, is not on that list. The slashes should stay; if we remove them, we'll just have to put them back later. >- <td><input type="text" size="40" name="id"/></td> >- <td align=left><input type="submit" value="Search"/></td> >+ <td><input type="text" size="40" name="id"></td> >+ <td align=left><input type="submit" value="Search"></td> This is actually a correct change - if there's a </td> , there shouldn't be a slash on the <td>. >-<a name="features"/> >+<a href="#features"></a> This is definitely wrong, I think. Gerv
Attachment #72946 - Flags: review-
Well, we might want to try to avoid all the warnings generated by validators so that admins can run them on their own templates. And most of the templates are going to be based on the default ones, otherwise it is going to be hard to keep the HTML compliant.
Chema, what's the status of this? Can you attach an updated patch that takes Gerv's comments into account?
I've changed my mind after reading the discussion in bug 47251. Aiming for HTML 4.01 transitional compatibility now and implementing XHTML support later (including trailing slashes) is the most sensible approach until we can address the issues raised by Hixie in bug 47251 comment 70. Chema, can you refresh your patch and submit an updated version?
Attached patch v2 (obsolete) — Splinter Review
Same patch, fixing the trivial cvs conflicts, mostly from my FILTER patch. This patch doesn't fix the html in all the files, but since there is already this patch, we should get it in before we rename all the templates.
Attachment #72946 - Attachment is obsolete: true
Attached patch v2a (obsolete) — Splinter Review
Err, lets try diffing all the files...
Attachment #78101 - Attachment is obsolete: true
Comment on attachment 78102 [details] [diff] [review] v2a This doesn't fix every validation problem; thats ok, one step at a time. >-<script src="localconfig.js"></script> >-<script src="quicksearch.js"></script> >+<script src="localconfig.js" type="text/javascript"></script> >+<script src="quicksearch.js" type="text/javascript"></script> > Are we going with text/javascript? > >-<a name="features"/> >+<a href="#features"></a> > <h2>Features</h2> > > <ul> Why this change? >--- template/default/info/describe-keywords.html.tmpl 3 Apr 2002 20:01:26 -0000 1.2 >+++ template/default/info/describe-keywords.html.tmpl 7 Apr 2002 14:56:17 -0000 >@@ -62,8 +62,6 @@ > </tr> > [% END %] > >-</table> >- this is only valid if there are no keywords. Should be: [% "</table>" IF keywords.size > 0 %] However, the w3c validation service does not compain about the <br /> form. How did you get it to do you?
Attachment #78102 - Flags: review-
Comment on attachment 78102 [details] [diff] [review] v2a >Index: quicksearchhack.html >=================================================================== >@@ -16,16 +17,16 @@ > onsubmit="QuickSearch(f.id.value); return false;"> > <table> > <tr> >- <td><input type="text" size="40" name="id"/></td> >- <td align=left><input type="submit" name="run" value="Search"/></td> >+ <td><input type="text" size="40" name="id"></td> >+ <td align=left><input type="submit" name="run" value="Search"></td> I'm going to be picky and request that you quote every attribute value, too. Leaving values unquoted is one potential point of failure for HTML, as not everything can be legally left unquoted. Plus it will make the transition to XHTML easier for anyone who wishes to do so. > <td align=left><input type="button" name="load" value="Load Query" >- onclick="LoadQuery(f.id.value);" /> >+ onclick="LoadQuery(f.id.value);"> >@@ -70,7 +71,7 @@ > > <tr> > <td>&nbsp;</td> >- <td rowspan=2><tt><nobr>UNCO,NEW,...,CLOS,</nobr><br><nobr>FIX,DUP,...</nobr> <i><nobr>(as first word)</nobr></i></tt></td> >+ <td rowspan=2 nowrap><tt>UNCO,NEW,...,CLOS,<br>FIX,DUP,...<i>(as first word)</i></tt></td> Even though it is valid HTML to use nowrap as you did, please use nowrap="nowrap" which is equally valid, yet is somewhat XML ready also. Also please quote the attributes (I'm going to leave it up to you to find the attributes. I'd clutter the review if I pointed out everyone). > <td><tt>status</tt></td> > <td>&nbsp;</td> > <td><a href="bug_status.html">Status</a> >Index: quicksearch.html >=================================================================== >@@ -54,7 +55,7 @@ > </ul> > </ul> > >-<a name="features"/> >+<a href="#features"></a> What bbaetz said. You probably want: <a name="features" id="features"></a> > <h2>Features</h2> > > <ul> >@@ -70,7 +71,7 @@ > </ul> > <i>Example:</i> &nbsp;<tt>url,location bar,field -focus</tt>&nbsp; > means >- <nobr>(<tt>url</tt> OR <tt>location</tt>) AND (<tt>bar</tt> OR <tt>field</tt>) AND (NOT <tt>focus</tt>)</nobr> >+ (<tt>url</tt> OR <tt>location</tt>) AND (<tt>bar</tt> OR <tt>field</tt>) AND (NOT <tt>focus</tt>) > <p> > <li> Use &nbsp;<tt>+foo</tt>&nbsp; to search for bugs where the <b>summary</b> contains &nbsp;<tt>foo</tt>&nbsp; as a <b>substring</b>.<br> > Use &nbsp;<tt>#foo</tt>&nbsp; to search for bugs where the <b>summary</b> contains the <b>word</b> &nbsp;<tt>foo</tt>&nbsp; All the unordered lists on this page, including the one above, want to have end tags (Mozilla has a tendency to incorrectly guess where you want ending </li> tags in certain cases. Please add a </li> for every <li> where appropriate throughout the page. >Index: template/default/entry/enter_bug.tmpl >=================================================================== >@@ -171,7 +171,7 @@ > <td colspan="3"> > <textarea wrap="hard" name="comment" rows="10" cols="80"> > [% comment FILTER html %]</textarea> >- <br /> >+ <br> it's not your fault with this patch, but the wrap attribute of <textarea> is not valid HTML... > </td> > </tr> > >@@ -179,16 +179,16 @@ > <td></td> > <td colspan="3"> > [% IF group.size %] >- <br /> >+ <br> > <strong> > Only users in the selected groups can view this bug: > </strong> >- <br /> >+ <br> > <font size="-1"> > (Leave all boxes unchecked to make this a public bug.) > </font> >- <br /> >- <br /> >+ <br> >+ <br> Why the <br><br>? Would a <p> work here instead? > > <!-- Checkboxes --> > [% FOREACH g = group %] >@@ -196,7 +196,7 @@ > <input type="checkbox" name="bit-[% g.bit %]" value="1" > [% " checked=\"checked\"" IF g.checked %] />[% g.description %]<br /> You missed a trailing slash -------------------------^^^ > [% END %] >- <br /> >+ <br> > [% END %] > </td> > </tr> >Index: template/default/query/query.atml >=================================================================== >@@ -424,7 +424,7 @@ > </select> > </td> > <td><input name="[% field.name %]" size="40" value=" >- [% default.${field.name}.0 FILTER html %]" /> >+ [% default.${field.name}.0 FILTER html %]" > Nit: remove the space at the end of the element. <foo attr="value"> not <foo attr="value" > > </td> > <td></td> > </tr> >@@ -448,7 +448,7 @@ > </select> > </td> > <td> >- <input name="keywords" size="40" value="[% default.keywords.0 FILTER html %]" /> >+ <input name="keywords" size="40" value="[% default.keywords.0 FILTER html %]" > Same as above. You do this a few more times throughout this file. Please get all remaining incidents as well. > </td> > </tr> > [% END %] >@@ -509,7 +509,7 @@ > <td> > <input type="checkbox" name="emailassigned_to[% n %]" > id="emailassigned_to[% n %]" value="1" >- [% " checked" IF default.emailassigned_to.$n %] /> >+ [% " checked" IF default.emailassigned_to.$n %] > Please use [% " checked=\"checked\"" IF ... %] here and below as well. > <label for="emailassigned_to[% n %]"> > bug owner > </label> >@@ -519,7 +519,7 @@ > <td> > <input type="checkbox" name="emailreporter[% n %]" > id="emailreporter[% n %]" value="1" >- [% " checked" IF default.emailreporter.$n %] /> >+ [% " checked" IF default.emailreporter.$n %] > > <label for="emailreporter[% n %]"> > reporter > </label> >@@ -530,7 +530,7 @@ > <td> > <input type="checkbox" name="emailqa_contact[% n %]" > id="emailqa_contact[% n %]" value="1" >- [% " checked" IF default.emailqa_contact.$n %] /> >+ [% " checked" IF default.emailqa_contact.$n %] > > <label for="emailqa_contact[% n %]"> > QA contact > </label> >@@ -541,7 +541,7 @@ > <td> > <input type="checkbox" name="emailcc[% n %]" > id="emailcc[% n %]" value="1" >- [% " checked" IF default.emailcc.$n %] /> >+ [% " checked" IF default.emailcc.$n %] > > <label for="emailcc[% n %]"> > CC list member > </label> >@@ -551,7 +551,7 @@ > <td> > <input type="checkbox" name="emaillongdesc[% n %]" > id="emaillongdesc[% n %]" value="1" >- [% " checked" IF default.emaillongdesc.$n %] /> >+ [% " checked" IF default.emaillongdesc.$n %] > Nit: Indentation on the above input is off. Make it match with the <label> please. > <label for="emaillongdesc[% n %]"> > commenter > </label> >@@ -663,18 +663,18 @@ > <td colspan="2"> > > [% IF NOT userid %] >- <input type="hidden" name="cmdtype" value="doit" /> >+ <input type="hidden" name="cmdtype" value="doit" > > [% ELSE %] >- <br /> >- <input type="radio" name="cmdtype" value="doit" checked /> Run this query >- <br /> >+ <br> >+ <input type="radio" name="cmdtype" value="doit" checked> Run this query >+ <br> Again use checked="checked" >@@ -701,17 +701,17 @@ ... > Remember this query, and name it: > <input type="text" name="newqueryname"> >- <br />&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<input type="checkbox" name="tofooter" value="1" /> >+ <br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;<input type="checkbox" name="tofooter" value="1"> > and put it in my page footer Eww, all those &nbsp; are ugly :( But still valid HTML Transitional...
Bbaetz asks: >>+<script src="localconfig.js" type="text/javascript"></script> >>+<script src="quicksearch.js" type="text/javascript"></script> > >Are we going with text/javascript? We'd better if we want to be HTML 4.01 _AND_ usable in MSIE. > the w3c validation service does not compain about the <br /> form. > Howdid you get it to do you? I've never seen the validator barf at <foo/> in HTML. I'm not convinced it should because the HTML specs say that SGML items such as processing instructions and shorthand markup are allowed, though it doesn't explicitly give an example of <foo/> for empty elements. I don't see this explicitly forbidden and it can be thought of implicitly allowed depending on how you read the spec I guess, but Hixie would know better than I would.
so if the validator never barfs at <foo />, why are we changing it? I thought the reason was because it wouldn't validate, but that doesn't seem to be the case.
> so if the validator never barfs at <foo />, why are we changing it? Umm, I dunno :) I really didn't know we were until I saw this patch. They way I read the spec, it is allowed as one of those SGML things which are not widely supported but I would defer to Hixie in this case. It's quite possible that both: a) I'm interpreting the spec incorrectly and b) So did the validator's author and the validator is buggy as a result.
according to hixie, its valid sgml - <a/foo> apparently means <a>foo</a>. That syntax is invalid html, though, because that form is disallowed by the html spec. Chema: Are you still interested in working on this? Otherwise someone else will take it over.
Gerv, others; <ad> Instead of submitting nitpick changes or bug reports about HTML validity in Bugzilla, I'd like to point out gmuck, http://gmuck.sourceforge.net/ that I released a week ago. In short gmuck stands for Generated MarkUp ChecKer, and assists in generating valid HTML by examining the source code that generates it. Bugzilla was one of the projects I tested gmuck on before releasing, and I think you will find the output of gmuck --mode=HTML --nodeprecated `find -type f` pretty useful when ran against the Bugzilla templates, HTML and Perl scripts. </ad> All feedback on gmuck is very much appreciated!
W3C compliance is great, but it has such a low impact on Bugzilla installations and users that there's no way this is a release-blocking bug. Keeping this bug in 2.16, since anyone is welcome to fix it in this milestone (if they can navigate the stormy waters of XHTML compatibility), but dropping the severity.
Severity: blocker → normal
OK, I'm going to attach a new patch now. Notes: The *.html pages were made to validate against at least a transitional doctype. I ran the ones which didn't have tags in ALLCAPS against the html 4.01 strict dtd too, and fixed issues which it picked up. The dtd in the html files is strict if it validates as strict, else its transitional - we use stuff like <center> and so on, so to move to strict we need to start using css. I also ran some of the better html files against the xhtml doctype, and fixed those issues which don't conflict with html. Stuff like quoting attribute values is only picked up against strict, not transitional, so those html files which were only run against transitional checks didn't have that fixed - I only fixed the transitional errors, and added <head> and <body> tags so that I could add the charset in. The charset was only added for the static html files - we have other bugs discussing the issues with adding those to the template. I also went through the non-admin cgi files. I didn't fix the <foo /> style of stuff in any places where Chema's original patch didn't. It passes 4.01 transitional validation, and if someone really really wants to have this pass, then they can file a separate bug, and probably script the change. This patch does not cause us to be entirely complied with transitional html. The warp="hard" issue means that enter_bug fails, and it also uses <nobr> arround attachment statuses. myk, any idea why we do that? I've changed the default mybugs link, admins will need to reset it to have their installations pick up the change. I also didn't check admin (edit*) pages at all - we can tidy those up when they are templatised. In short, yes, this doesn't cover everything, but it mostly covers html4.01 transitional compliance, which is all we're aiming for at this point. I'd like to get this in, warts and all, into 2.16, since its definatly an improvment, and we can deal with the remaining issues later. Note that the show_activity change is because <tr> cannot nest - I had to play with the dom inspector before I worked out what the validator was complaining about. Taking.
Assignee: justdave → bbaetz
Status: REOPENED → NEW
Attached patch v3Splinter Review
Attachment #78102 - Attachment is obsolete: true
Comment on attachment 79116 [details] [diff] [review] v3 r=gerv. Can't see anything nasty in that lot. Gerv
Attachment #79116 - Flags: review+
Comment on attachment 79116 [details] [diff] [review] v3 r= justdave
Attachment #79116 - Flags: review+
Checked in. Please file other bugs on other issues, and either use bug 47251 or make them block that bug. Checking in bug_status.html; /cvsroot/mozilla/webtools/bugzilla/bug_status.html,v <-- bug_status.html new revision: 1.13; previous revision: 1.12 done Checking in bugwritinghelp.html; /cvsroot/mozilla/webtools/bugzilla/bugwritinghelp.html,v <-- bugwritinghelp.html new revision: 1.3; previous revision: 1.2 done Checking in confirmhelp.html; /cvsroot/mozilla/webtools/bugzilla/confirmhelp.html,v <-- confirmhelp.html new revision: 1.3; previous revision: 1.2 done Checking in defparams.pl; /cvsroot/mozilla/webtools/bugzilla/defparams.pl,v <-- defparams.pl new revision: 1.71; previous revision: 1.70 done Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.155; previous revision: 1.154 done Checking in help.html; /cvsroot/mozilla/webtools/bugzilla/help.html,v <-- help.html new revision: 1.5; previous revision: 1.4 done Checking in helpemailquery.html; /cvsroot/mozilla/webtools/bugzilla/helpemailquery.html,v <-- helpemailquery.html new revision: 1.2; previous revision: 1.1 done Checking in how_to_mail.html; /cvsroot/mozilla/webtools/bugzilla/how_to_mail.html,v <-- how_to_mail.html new revision: 1.3; previous revision: 1.2 done Checking in notargetmilestone.html; /cvsroot/mozilla/webtools/bugzilla/notargetmilestone.html,v <-- notargetmilestone.html new revision: 1.3; previous revision: 1.2 done Checking in quicksearch.html; /cvsroot/mozilla/webtools/bugzilla/quicksearch.html,v <-- quicksearch.html new revision: 1.3; previous revision: 1.2 done Checking in quicksearchhack.html; /cvsroot/mozilla/webtools/bugzilla/quicksearchhack.html,v <-- quicksearchhack.html new revision: 1.4; previous revision: 1.3 done Checking in reports.cgi; /cvsroot/mozilla/webtools/bugzilla/reports.cgi,v <-- reports.cgi new revision: 1.53; previous revision: 1.52 done Checking in votehelp.html; /cvsroot/mozilla/webtools/bugzilla/votehelp.html,v <-- votehelp.html new revision: 1.10; previous revision: 1.9 done Checking in template/default/index.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/default/index.tmpl,v <-- index.tmpl new revision: 1.5; previous revision: 1.4 done Checking in template/default/admin/create_account.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/default/admin/create_account.tmpl,v <-- create_account.tmpl new revision: 1.4; previous revision: 1.3 done Checking in template/default/attachment/list.atml; /cvsroot/mozilla/webtools/bugzilla/template/default/attachment/list.atml,v <-- list.atml new revision: 1.7; previous revision: 1.6 done Checking in template/default/buglist/table.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/default/buglist/table.tmpl,v <-- table.tmpl new revision: 1.2; previous revision: 1.1 done Checking in template/default/entry/enter_bug.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/default/entry/enter_bug.tmpl,v <-- enter_bug.tmpl new revision: 1.5; previous revision: 1.4 done Checking in template/default/info/describe-keywords.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/default/info/describe-keywords.html.tmpl,v <-- describe-keywords.html.tmpl new revision: 1.3; previous revision: 1.2 done Checking in template/default/prefs/email.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/default/prefs/email.tmpl,v <-- email.tmpl new revision: 1.2; previous revision: 1.1 done Checking in template/default/prefs/footer.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/default/prefs/footer.tmpl,v <-- footer.tmpl new revision: 1.2; previous revision: 1.1 done Checking in template/default/query/query.atml; /cvsroot/mozilla/webtools/bugzilla/template/default/query/query.atml,v <-- query.atml new revision: 1.9; previous revision: 1.8 done Checking in template/default/show/activity.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/default/show/activity.html.tmpl,v <-- activity.html.tmpl new revision: 1.3; previous revision: 1.2 done
Status: NEW → RESOLVED
Closed: 23 years ago23 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: