264511 - make bugzilla easier to style with css

Status: RESOLVED FIXED

(Bugzilla :: User Interface, enhancement)

Description

[:glob ✱]

we should add css ids and classes to make bugzilla easier to style with css
until bugzilla uses html4 and css (bug 251068).

Comment 1

[:glob ✱]

Attachment: adds css ids and classes v1

this patch adds css ids and classes to make it easy to style bugzilla with css.


the bulk of the patch is to convert <td align="right><b>headers</b></td> into
<th align='right">headers</th>.

Comment 2

[:glob ✱]

bug 259723 contains a global.css and a few screenshots that show what can be
done with these changes.

Comment 3

[:glob ✱]

Attachment: adds css ids and classes v2

fixes a few stray <th>'s

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 162190 [details] [diff] [review]
adds css ids and classes v1

Mmm, yum.  This is not a complete review, but a few notes.

Please use dash-separated CSS id and class names, i.e. knob-options instead of
knobOptions.  

You have a typo here:

-function PutDescription() {
+function Puthescription() {

Otherwise this looks good and should be easy to review.

Comment 5

[:glob ✱]

Attachment: adds css ids and classes v3

fixes css naming convention

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 162197 [details] [diff] [review]
adds css ids and classes v3

Thanks for this useful patch with some great improvements to Bugzilla's HTML
code!  All the td -> th stuff is fine and sorely needed.  The id/class
additions are also mainly good, but there are a few issues, outlined below.  I
haven't installed and tested it yet, but once the issues below are resolved
I'll do so and let you know if I run across any other problems (which I don't
anticipate given the relatively low-risk nature of most of these changes).



>Index: template/en/default/attachment/list.html.tmpl

>-<table cellspacing="0" cellpadding="4" border="1">
>+<table cellspacing="0" cellpadding="4" border="1" id="attachment-list">

Nit: put id and class attributes first and second, respectively, in the list of
attributes on a tag, as they identify the tag and are the first attributes
stylers look for when perusing HTML code.  Also, later you enclose comments in
a div identified by the id "comments".	This should be consistent with that,
i.e. make this "attachments" rather than "attachment-list".


>-  <tr>
>+  <tr id="attachment-list-actions">

This only covers the bottom row of the table, while there are actions in the
last field of each row as well.  Perhaps this could be a class and the last
column could belong to that class as well, although this may require adding a
colgroup element.  Also, for simplicity this could just be
"attachment-actions", since the fact that they're in a list is orthogonal to
their identification for styling purposes.  Finally, what's the purpose of a
separate id here; couldn't necessary styling be done with the selector
"table#attachments > a"?



>Index: template/en/default/bug/comments.html.tmpl

>-    <div [% "class=\"bz_private\" " IF comment.isprivate %]
>+    <div class="comment"
>+         [% "class=\"bz_private\" " IF comment.isprivate %]
>          [% "class=\"bz_comment_hilite\" " IF marks.$count %]>
>       [% IF count > 0 %]
>-        <br>
>         <span class="bz_comment">

Calling the div "comment" when the span is already "bz_comment" suggests that:

1. we're switching from prefixed names to non-prefixed names;
2. the entire comment is better classified as the "comment" than the header;
and
3. that we're keeping bz_comment in place for backwards compatibility.

I'm OK with these changes, but the span should be given a new, more accurate,
additional class name so that we can deprecate bz_comment, f.e.
"comment-header".


>-          ------- <i>Additional Comment
>+          <span class="comment_rule">-------</span> <i>Additional Comment

comment_rule -> comment-rule.  Also, does the rule really need separate
styling?



>Index: template/en/default/bug/edit.html.tmpl

>-    document.write('[<a href="#add_comment" onclick="replyToComment(' + 
>-        id + ');">reply<' + '/a>]');
>+    document.write('<span class="reply-button">[<a href="#add_comment" ' +
>+        'onclick="replyToComment(' + id + ');">reply<' + '/a></span>]');

No need to add a span; the anchor tag itself should get the id here.  Also, is
this necessary given that it can be selected with ".bz_comment > a"?


>-  <table cellspacing="1" cellpadding="1" border="0">
>+  <table cellspacing="1" cellpadding="1" border="0" id="bug-form">

This id doesn't make sense given that the table in question doesn't contain the
whole form.



>Index: template/en/default/bug/knob.html.tmpl

>-  <br>
>+<br>
>+
>+<div id="knob">
>+  <div id="knobOptions">
...
>+  <div id="knob-buttons">

Since knob options are easily distinguishable from knob buttons (f.e. div.knob
> input[type="submit"]) it doesn't seem as if options and buttons need their
own divs.  Also, does anything depend on the knob being inline rather than
block context?



>Index: template/en/default/global/header.html.tmpl

>+  <div id="page-title-bar">
>     <table border="0" cellspacing="0" width="100%">

The id should go on the table rather than in a separate div, and the name could
be simplified (and the meaning made more flexible) by dropping the "-bar". 
Also, this would be better called a "header" since it's in "header.html.tmpl"
and displays "h1", "h2", and "h3" variables akin to the "header" tags and the
"head" section in HTML.


>-              <td valign="top" align="left" nowrap="nowrap">
>-                <font size="+1"><b>[% h1 %]</b></font>
>+              <td valign="top" align="left" nowrap="nowrap" id="page-title1">
>+                [% h1 %]

I suggest "page-header-h1" instead here.


>-        <td valign="middle" align="left">[% h2 %]</td>
>+        <td valign="middle" align="left" id="page-title1">[% h2 %]</td>
>         [% IF h3 %]
>-          <td valign="middle" align="right">[% h3 %]</td>
>+          <td valign="middle" align="right" id="page-title1">[% h3 %]</td>

These should be 2 and 3, no?


>+<div id="bugzilla-body">

If the header is called "page-header", then it would make sense to call this
"page-body".  "bugzilla-body" is problematic not only because of that
inconsistency but because installations can rename Bugzilla to something else
via global/variables.none.html.

Comment 7

[Marc Schumann [:Wurblzap]]

(In reply to comment #6)
> >+    <div class="comment"
> >+         [% "class=\"bz_private\" " IF comment.isprivate %]
> >          [% "class=\"bz_comment_hilite\" " IF marks.$count %]>

Attributes may appear only once...

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #7)
> (In reply to comment #6)
> > >+    <div class="comment"
> > >+         [% "class=\"bz_private\" " IF comment.isprivate %]
> > >          [% "class=\"bz_comment_hilite\" " IF marks.$count %]>
> 
> Attributes may appear only once...

I thought so too, but I can't find anything in the spec that says that, and
these attributes are already there, so that's not this patch's fault.

Comment 9

[Christian Reis]

(In reply to comment #8)
> > > >+    <div class="comment"
> > > >+         [% "class=\"bz_private\" " IF comment.isprivate %]
> > > >          [% "class=\"bz_comment_hilite\" " IF marks.$count %]>
> > 
> > Attributes may appear only once...
> 
> I thought so too, but I can't find anything in the spec that says that, and
> these attributes are already there, so that's not this patch's fault.

Yeah, but it would be nice to fix that as it goes. I know for a fact that that
will not work in fairly recent browser versions as I've made the same error.
It's quite easy to fix:

     <div class="comment
      [% "bz_private " IF comment.isprivate %]
      [% "bz_comment_hilite " IF marks.$count %]>">

Even though it is slightly illegible, we should do it Right :)

Comment 10

[:glob ✱]

thanks for the review :)

there's a few comments that i can address off the top of my head:

> comment_rule -> comment-rule.  Also, does the rule really need separate
> styling?

i want to be able to remove the comment-rules with css (they are ugly)

> >+  <div id="page-title-bar">
> >     <table border="0" cellspacing="0" width="100%">
> 
> The id should go on the table rather than in a separate div

that extra div is required as i had a lot of problems with IE rendering when i
applied the styles directly to the table (in relation to the width calculation).



should i drop backwards compatibility with repsect to bz_ prefixed styles, and
update them?  eg.  change bz_private to comment-private

Comment 11

[Myk Melez [:myk] [@mykmelez]]

> should i drop backwards compatibility with repsect to bz_ prefixed styles, and
> update them?  eg.  change bz_private to comment-private

It's not necessary to do so in this bug, but we should consider making these
changes in another (but keeping backwards compatibility where possible).

Comment 12

[Jeff Eng]

This bug hadn't been touched in a while... wasn't sure if it was still alive, or
if I was allowed to chime in;  this would be my first bugzilla comment ever.

>Index: template/en/default/bug/knob.html.tmpl
>-  <br>
>+<br>
>+
>+<div id="knob">
>+  <div id="knobOptions">
...
>+  <div id="knob-buttons">

It seems like the most sensical semantic mark up for the knobs is a <ul> list,
with each knob option being an <li>.   In the style sheet, we can remove the
usually displayed bullet points to simulate the original look.

>>Index: template/en/default/bug/edit.html.tmpl
-      <td align="right">
-        <b><u>P</u>roduct:</b>
-      </td>
+      <th align="right">
+        <u>P</u>roduct:
+      </th>

We might as well remove the align=right and add a css class from all the <th>'s.
 We can call it something like "field-name", altho I'm not sure what the
bugzilla css class naming scheme is.   We can use "text-align: right" to
recreate the right align.

Thanks for listening.

Comment 13

[PT]

Is anybody working on this? 
Is there an live example for these styles?
Do you plan to supply a patch for the upcoming 2.20 release?

Comment 14

[Marc Schumann [:Wurblzap]]

(In reply to comment #8)
> > Attributes may appear only once...
> 
> I thought so too, but I can't find anything in the spec that says that, and
> these attributes are already there, so that's not this patch's fault.

http://www.w3.org/TR/2004/REC-xml-20040204/#sec-starttags says:

   "Well-formedness constraint: Unique Att Spec
    An attribute name MUST NOT appear more than once in the same start-tag or
    empty-element tag."

Comment 15

[Marc Schumann [:Wurblzap]]

See also bug 321556.

Comment 16

[Frédéric Buclin]

*** Bug 335760 has been marked as a duplicate of this bug. ***

Comment 17

[Guy Pyrzak]

I am marking this as the meta bug for the polish of buzilla I'm doing for 3.2 as requested by LpSolit and various others. I will be removing all the hard coded css and adding classes and ids where appropriate. Also adding glob to the cc list of this bug since i'll be requesting review from him based on a referral from mkanat.

Comment 18

[Max Kanat-Alexander]

Negatory on the blocking there, Red Ryder. This is too much of a modification to go into 3.2.

Comment 19

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is too invasive or too risky to be accepted for 4.4 at this point. The target milestone is set to 5.0 which is our next major release.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else on time for 5.0.

Comment 20

[Frédéric Buclin]

All hardcoded CSS rules have been removed from templates.