Closed Bug 129442 Opened 21 years ago Closed 21 years ago

Make HTML of a default installation HTML W3C compliant


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




Bugzilla 2.16


(Reporter: chema, Assigned: bbaetz)





(1 file, 3 obsolete files)

Make HTML of bugzilla HTML Transitional 4.01 compliant, Patch attached.

Before :


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 ***
Closed: 21 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 :-(

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
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!

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 ./ 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]

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 

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]


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

>-      <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. 

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]

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 %]

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]

>Index: quicksearchhack.html
>@@ -16,16 +17,16 @@
>       onsubmit="QuickSearch(; 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(;" />
>+                      onclick="LoadQuery(;">
>@@ -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="[% %]" size="40" value="
>-        [% default.${}.0 FILTER html %]" />
>+        [% default.${}.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>

>       <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; 
Instead of submitting nitpick changes or bug reports about HTML validity in Bugzilla, 
I'd like to point out gmuck, 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. 
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.

Assignee: justdave → bbaetz
Attached patch v3Splinter Review
Attachment #78102 - Attachment is obsolete: true
Comment on attachment 79116 [details] [diff] [review]

r=gerv. Can't see anything nasty in that lot.

Attachment #79116 - Flags: review+
Comment on attachment 79116 [details] [diff] [review]

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
Checking in bugwritinghelp.html;
/cvsroot/mozilla/webtools/bugzilla/bugwritinghelp.html,v  <--  bugwritinghelp.html
new revision: 1.3; previous revision: 1.2
Checking in confirmhelp.html;
/cvsroot/mozilla/webtools/bugzilla/confirmhelp.html,v  <--  confirmhelp.html
new revision: 1.3; previous revision: 1.2
Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision: 1.71; previous revision: 1.70
Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision: 1.155; previous revision: 1.154
Checking in help.html;
/cvsroot/mozilla/webtools/bugzilla/help.html,v  <--  help.html
new revision: 1.5; previous revision: 1.4
Checking in helpemailquery.html;
/cvsroot/mozilla/webtools/bugzilla/helpemailquery.html,v  <--  helpemailquery.html
new revision: 1.2; previous revision: 1.1
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
Checking in notargetmilestone.html;
/cvsroot/mozilla/webtools/bugzilla/notargetmilestone.html,v  <-- 
new revision: 1.3; previous revision: 1.2
Checking in quicksearch.html;
/cvsroot/mozilla/webtools/bugzilla/quicksearch.html,v  <--  quicksearch.html
new revision: 1.3; previous revision: 1.2
Checking in quicksearchhack.html;
/cvsroot/mozilla/webtools/bugzilla/quicksearchhack.html,v  <--  quicksearchhack.html
new revision: 1.4; previous revision: 1.3
Checking in reports.cgi;
/cvsroot/mozilla/webtools/bugzilla/reports.cgi,v  <--  reports.cgi
new revision: 1.53; previous revision: 1.52
Checking in votehelp.html;
/cvsroot/mozilla/webtools/bugzilla/votehelp.html,v  <--  votehelp.html
new revision: 1.10; previous revision: 1.9
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
Checking in template/default/admin/create_account.tmpl;
 <--  create_account.tmpl
new revision: 1.4; previous revision: 1.3
Checking in template/default/attachment/list.atml;
/cvsroot/mozilla/webtools/bugzilla/template/default/attachment/list.atml,v  <--
new revision: 1.7; previous revision: 1.6
Checking in template/default/buglist/table.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/buglist/table.tmpl,v  <-- 
new revision: 1.2; previous revision: 1.1
Checking in template/default/entry/enter_bug.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/entry/enter_bug.tmpl,v  <--
new revision: 1.5; previous revision: 1.4
Checking in template/default/info/describe-keywords.html.tmpl;
 <--  describe-keywords.html.tmpl
new revision: 1.3; previous revision: 1.2
Checking in template/default/prefs/email.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/prefs/email.tmpl,v  <-- 
new revision: 1.2; previous revision: 1.1
Checking in template/default/prefs/footer.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/prefs/footer.tmpl,v  <-- 
new revision: 1.2; previous revision: 1.1
Checking in template/default/query/query.atml;
/cvsroot/mozilla/webtools/bugzilla/template/default/query/query.atml,v  <-- 
new revision: 1.9; previous revision: 1.8
Checking in template/default/show/activity.html.tmpl;
<--  activity.html.tmpl
new revision: 1.3; previous revision: 1.2
Closed: 21 years ago21 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.