CGI.pl: PutHeader and GetCommandMenu should output correct HTML

RESOLVED FIXED in Bugzilla 2.16

Status

()

Bugzilla
Bugzilla-General
RESOLVED FIXED
17 years ago
6 years ago

People

(Reporter: Christian Reis, Assigned: Christian Reis)

Tracking

2.10
Bugzilla 2.16
x86
Linux

Details

Attachments

(5 attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Updated

17 years ago
Blocks: 47251
(Assignee)

Comment 1

17 years ago
Created attachment 52457 [details] [diff] [review]
Fixes HTML compliance for CGI.pl, impacting proper HTML compliance of all pages - many are now HTML4 compliance apart from DTD.
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Keywords: patch, review
(Assignee)

Comment 2

17 years ago
-> me (the spammer)
Assignee: justdave → kiko
Status: ASSIGNED → NEW
(Assignee)

Comment 3

17 years ago
Created attachment 52459 [details] [diff] [review]
kiko_v2: fixes issue with headers being totally  ed
(Assignee)

Comment 4

17 years ago
Created attachment 52464 [details] [diff] [review]
kiko_v3: ignore the width in the header table, make rendering perfect and no warnings. yay.
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?
Attachment #52464 - Flags: review-
(Assignee)

Comment 6

17 years ago
Created attachment 52647 [details] [diff] [review]
kiko_v4: fixed as per caillon's recommendations
(Assignee)

Comment 7

17 years ago
> You need to escape the ampersands to &amp; 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=

Status: NEW → ASSIGNED
(Assignee)

Comment 9

17 years ago
Created attachment 52667 [details] [diff] [review]
kiko_v5: remove DOCTYPE URI for now to allow quirks
(Assignee)

Comment 10

17 years ago
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
Attachment #52667 - Flags: review+
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
Attachment #52667 - Flags: review+
(Assignee)

Comment 12

17 years ago
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.
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.
(Assignee)

Comment 14

17 years ago
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.
(Assignee)

Comment 15

17 years ago
(spam)

justdave, i've commented here.
ok, works for me.  checked in.

/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.108; previous revision: 1.107
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
no, really this time :)
Target Milestone: --- → Bugzilla 2.16
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)
<font> is deprecated in transitional, but still legal per the DTD.

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

Comment 21

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

Thanks Christopher.
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.