Closed Bug 148488 Opened 23 years ago Closed 23 years ago

More HTML validation fixes

Categories

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

Product:
Bugzilla
Component:
Bugzilla-General
Version:
2.17
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Bugzilla 2.18

People

(Reporter: bbaetz, Assigned: bbaetz)

Details

(Keywords: polish)

Attachments

(1 file, 2 obsolete files)

v3
23.89 KB, patch
jouni
: review+
gerv
: review+
Details | Diff | Splinter Review 
The patch I'll attach fixes everything found by:

find . -iname '*.html.tmpl' | xargs gmuck --mode=HTML --nodeprecated --nomimetype

with one exception.

The exception is that the 'edit comment as text' <textarea> doesn't have a rows
attribute, which is required. We set the size, using a style attribute. Mozilla
seems to ignore the rows attribute, but I need to check if that is guarenteed.
That page only wokrs in mozilla ATM, so I'm not worried about other browsers.

The mimetype warnings are all on the fact that we use text/javascript instead of
application/x-javascript. Was this a deliberate decision made for old IE
versions, or something? If not, I'll fix those, too.

This is for 2.18, not 2.16, although the account/email/confirm.html.tmpl part
(<input type="input">) may need to go into 2.16, since I can imagine browsers
having issues with that.

Most of the fixes are obvious (& -> amp;, adding action attribute to <form>s,
and alt to <img> and so on). Those that aren't include:

global/useful-links.html.tmpl: gmuck warns about inconcistant quote style, so I
changed these to use " instead of ' (they were " because of how we were quoting
for TT)

search/form.html.tmpl: gmuck picked up on:

[%# Note: the <form> tag is unclosed at the end of this template %]

as not having an action attribute. Since the opening isn't in this template, and
only one template uses this particular template, I think its a fairly silly
comment, so I removed it. I care because I want gmuck to become part of the
tests - see bug 148487. I thought about reporting this as a gmuck bug, but its
not, since gmuck shouldn't be expected to know about TT's comment scheme.
Attached patch v1 (obsolete) — — Splinter Review 

    
  
Keywords: patch, 
          
            polish, 
          
            review
Priority: -- → P2
Target Milestone: --- → Bugzilla 2.18
Version: 2.15 → 2.17

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v2 (obsolete)
        — 
          — Splinter Review
      

    


  
This also fixes some of the html in the perl files. I've left out stuff which
is yet to be templatised or will be otherwise rewritten (reports.cgi,
*edit*.cgi, queryhelp.cgi) and would have been a pain to do, for not much gain.


gmuck isn't as accurate here, obviously - it picks up on <HTML> where HTML is a
file handle, for example.

Why are we using http-equiv redirects instead of 302 responses, btw?

        Attachment #85889 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 85891 [details] [diff] [review]
v2

>Index: template/en/default/admin/attachstatus/list.html.tmpl
>===================================================================
...
>+<script language="JavaScript" type="text/javascript">
>   function confirmDelete(attachcount, name, id)
>   {
>     if (attachcount > 0) {

Shouldn't there be <!-- --> around this javascript block as well? 
Could we add it while we're doing this?

Also note that the language attribute is deprecated markup.

>Index: template/en/default/bug/dependency-graph.html.tmpl
>===================================================================
>+  <img src="[% image_url %]" alt="Dependancy graph" usemap="#imagemap"> 
>+    <img src="[% image_url %]" alt="Dependancy graph" ismap="ismap"> 

Nit: Make these "dependEncy" graphs.

About the XHTMLish syntax 'ismap="ismap"' and 'nowrap="nowrap"': Why? 
We don't have selected="selected" either, and at least some legacy 
browsers have had problems with the expanded notation. If we claim 
HTML 4.01 compatibility, we can use the "nowrap" and "ismap" 
attributes without values, can't we? 

About the redirects: Yeah, HTTP status codes would be cooler. 
But shouldn't that be 301 instead of 302?

    
    

    
  
Yes, those scripts probably should be commented.

The extended syntax was supported for html4.01, and ISTR it being required for
html4.01 strict complience. Do browsers really have problems with it?

    
    

    
  
>The extended syntax was supported for html4.01, and ISTR it being 
>required for html4.01 strict complience. Do browsers really 
>have problems with it?

The "extended syntax" (meaning nowrap="nowrap") is actually the basic markup,
and attribute minimizations ("nowrap") are just SGML syntactic sugar. The
different HTML dialects are just DTD's; the SGML declaration is the same, and
thus the rules for SGML structural elements are identical between doctypes.
XHTML is based on XML which doesn't have the concept of attribute minimization,
and thus all X(HT)ML based markup must use fully written attributes. So in
short, in HTML you can have it done either way regardless of the DTD used. In
X(HT)ML you always have to write out the values. 

About the browser incompatibilities: I don't know what the current situation
practically is. At least some time ago I remember having a problem with some
older browser not implementing checked="checked" properly.

But my key point is: I don't see a reason to partially move towards a new markup
unless we have a clearly set goal of doing it all. Query forms etc. are filled
with minimized attributes, so we won't reach XHTML compatibility with these
changes anyway - and HTML 4.01 Strict can be done even without touching the
nowrap/checked/readonly/selected/whatever thingies.


    
    

    
  
The HTML2.0 spec says:

"Attributes such as ISMAP and COMPACT may be written using a minimized syntax
(see 7.9.1.2 "Omitted Attribute Name" in [SGML]). The markup:

<UL COMPACT="compact">

can be written using a minimized syntax:

<UL COMPACT>

(9)
"

Where footnote 9 is:

"Some historical implementations only understand the minimized syntax."

If HTML 2.0 refers to historical implementations, I'd be surprised if bugzilla
works on such versions. Note that even in html2.0, teh minimisation stuff was
only a shortcut.

I'll check with hixie, though.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v3
        — 
          — Splinter Review
      

    


  
Hixie says we don't have to worry about browsers which don't support the long
for for attributes - they're unlikely to be supporting forms anyway, and noone
uses them.

        Attachment #85891 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 87520 [details] [diff] [review]
v3

>-      <option value="[% resolution FILTER html %]" [% selected IF resolution == "FIXED" %]>
>+      <option value="[% resolution FILTER html %]" [% selected="selected" IF resolution  == "FIXED" %]>

Make that 'selected="selected"' IF resolution == "FIXED".
I know, the previous code didn't work either... but why not 
fix this now, since it's such a triviality.

r=jouni with the above.

        Attachment #87520 -
        Flags: review+

    
    

    
  
Comment on attachment 87520 [details] [diff] [review]
v3

r=gerv for 2.17 only. I can't be bothered to check the entire patch, but if you
break something, we are bound to spot it before 2.18.

Gerv

        Attachment #87520 -
        Flags: review+

    
    

    
  
Checked in to trunk
Status: NEW → RESOLVED
Closed: 23 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: