103554 - CGI.pl: PutHeader and GetCommandMenu should output correct HTML

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Christian Reis]

Every page bugzilla renders uses these two methods to output HTML, and at the
moment the HTML is broken in accord to W3C HTML4.01 Transitional, which is
AFAICT a worthy target. I agree this is a temporary fix, but since XHTML is
still a ways off (and this is a step towards it) I'd like to fix these two
functions and get the W3C validator parsing our pages. This bug, when fixed,
allows bug 83058 to output correct HTML.

Comment 1

[Christian Reis]

Attachment: Fixes HTML compliance for CGI.pl, impacting proper HTML compliance of all pages - many are now HTML4 compliance apart from DTD.

Comment 2

[Christian Reis]

-> me (the spammer)

Comment 3

[Christian Reis]

Attachment: kiko_v2: fixes issue with headers being totally  ed

Comment 4

[Christian Reis]

Attachment: kiko_v3: ignore the width in the header table, make rendering perfect and no warnings. yay.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 52464 [details] [diff] [review]
kiko_v3: ignore the width in the header table, make rendering perfect and no warnings. yay.

I already mentioned in IRC about +  $html .= "<A HREF=\"buglist.cgi?&cmdtype=runnamed&namedcmd=" .
You need to escape the ampersands to &amp; and of course change the ?& to just ?

Please escape the ampersand also in the links on lines 336 and 1272.

Line 558 of CGI.pl (after applying your patch):
$popup .= "<INPUT NAME=$groupname type=radio VALUE=\"$item\" check>$displaytext<br>";
"check" needs to be "checked" (or "CHECKED")

Also, there are many calls to PutHeader("blah") which have a print statement following it with no <p> tag before the text.

Line 1263:      $html .= "<table border cellpadding=4>\n";
border needs an attribute.


And why not include a Doctype Declaration?

Comment 6

[Christian Reis]

Attachment: kiko_v4: fixed as per caillon's recommendations

Comment 7

[Christian Reis]

> You need to escape the ampersands to & and of course change the ?&
> to just ?

Fixed. Looks wierd. :-)
  
> Please escape the ampersand also in the links on lines 336 and 1272.

Fixed, though they are not in the scope of this bug.

> Line 558 of CGI.pl (after applying your patch):
> $popup .= "<INPUT NAME=$groupname type=radio VALUE=\"$item\"
> check>$displaytext<br>"; "check" needs to be "checked" (or "CHECKED")

Fixed.

> Also, there are many calls to PutHeader("blah") which have a print
> statement following it with no <p> tag before the text.

The HTML validator doesn't seem to complain about a <TD> element
containing text unwrapped in <P> and IIRC it's allowed.

> Line 1263:      $html .= "<table border cellpadding=4>\n";
> border needs an attribute.

Fixed.

> And why not include a Doctype Declaration?

Added.

100% compliant as per:

http://validator.w3.org/check?uri=http%3A%2F%2Fbugs.async.com.br%2Fshowdependen$

Looking for r=/2r=

Comment 8

[Christian Reis]

Damn. Link was:

http://validator.w3.org/check?uri=http%3A%2F%2Fbugs.async.com.br%2Fshowdependencytree.cgi%3Fid%3D172&charset=%28detect+automatically%29&doctype=%28detect+automatically%29&ss=

Sincerely,
   K. T. Spammer.

Comment 9

[Christian Reis]

Attachment: kiko_v5: remove DOCTYPE URI for now to allow quirks

Comment 10

[Christian Reis]

Comment on attachment 52667 [details] [diff] [review]
kiko_v5: remove DOCTYPE URI for now to allow quirks

r=caillon on irc:
<caillon> kiko: add it for me.  i'm browserless.  i already gave it to you.
<kiko> caillon: I love you

Comment 11

[Gervase Markham [:gerv]]

Comment on attachment 52667 [details] [diff] [review]
kiko_v5: remove DOCTYPE URI for now to allow quirks

r=gerv. I don't quite see that the win is as big as the changes, but there you go... Has this been tested cross-browser?

Gerv

Comment 12

[Christian Reis]

Gerv, the advantage, simply put, is that we _become_ compliant with HTML4. This
means we can actually request compliance before a checkin and use the HTML
testing to validate the pages. This prerequisite is important in setting a
standard for bugzilla - no broken HTML allowed, and I think that's good.

Comment 13

[Dave Miller [:justdave]]

Was about to check this in, but decided to review it myself first.

Why did you remove the WIDTH="100%" from the header <TABLE> ?
I think it's going to look funny without it, and percentage widths are perfectly
valid for tables (just not for table cells) last I heard.

Comment 14

[Christian Reis]

Dave:

Actually, the WIDTH="100%" seems to be pretty much useless in the
context it is used. It wraps _only_ the heading table, and it actually
causes $h1 and $h2 to be rendered in a very strange (and perhaps broken)
fashion.

I removed width there because it created a problem: the TD for $h1 had
width="10%", which forced it to be the smaller of the two TDs (though it
is a hack and not really to be used AFAICT). This TD at the same time
contained a NOBR which is invalid HTML4. I believe the NOBR was stuck in
to counteract the line wrapping that the 10% width created. So, IOW, it
was a hack, and non-compliant at that. Removing both width and NOBR made
the rendering perfectly identical as the original version, so I think it
was reasonable to change.

Comment 15

[Christian Reis]

(spam)

justdave, i've commented here.

Comment 16

[Dave Miller [:justdave]]

ok, works for me.  checked in.

/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.108; previous revision: 1.107

Comment 17

[Dave Miller [:justdave]]

Fixed for 2.16

Comment 18

[Dave Miller [:justdave]]

no, really this time :)

Comment 19

[Dave Miller [:justdave]]

Is <FONT> legal in 4.01 transitional?  iCab's validator flags it as deprecated.
 If it's really not legal, we should reopen this...   (and fwiw, the copies of
the header and footer in the templates folder for the template versions have the
old bad HTML in them)

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

<font> is deprecated in transitional, but still legal per the DTD.

http://www.w3.org/TR/html401/present/graphics.html#edef-FONT

Comment 21

[Christian Reis]

Dave, we should file a bug for the templates' HTML then. I thought
the templates used these functions here though - no?

Thanks Christopher.