Closed
Bug 103554
Opened 22 years ago
Closed 22 years ago
CGI.pl: PutHeader and GetCommandMenu should output correct HTML
Categories
(Bugzilla :: Bugzilla-General, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.16
People
(Reporter: kiko, Assigned: kiko)
References
Details
Attachments
(5 files)
6.77 KB,
patch
|
Details | Diff | Splinter Review | |
6.70 KB,
patch
|
Details | Diff | Splinter Review | |
6.68 KB,
patch
|
caillon
:
review-
|
Details | Diff | Splinter Review |
8.55 KB,
patch
|
Details | Diff | Splinter Review | |
8.51 KB,
patch
|
kiko
:
review+
gerv
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Assignee | ||
Comment 2•22 years ago
|
||
-> me (the spammer)
Assignee: justdave → kiko
Status: ASSIGNED → NEW
Assignee | ||
Comment 3•22 years ago
|
||
Assignee | ||
Comment 4•22 years ago
|
||
Comment 5•22 years ago
|
||
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 & 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•22 years ago
|
||
Assignee | ||
Comment 7•22 years ago
|
||
> 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=
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•22 years ago
|
||
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.
Assignee | ||
Comment 9•22 years ago
|
||
Assignee | ||
Comment 10•22 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 11•22 years ago
|
||
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•22 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.
Comment 13•22 years ago
|
||
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•22 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•22 years ago
|
||
(spam) justdave, i've commented here.
Comment 16•22 years ago
|
||
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
Closed: 22 years ago
Resolution: --- → FIXED
Comment 17•22 years ago
|
||
Fixed for 2.16
Comment 19•22 years ago
|
||
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•22 years ago
|
||
<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•22 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.
Updated•11 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•