Convert Bugzilla header to html4 and css (no functionality change)

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
User Interface
--
enhancement
RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: Svetlana Harisova, Assigned: Svetlana Harisova)

Tracking

2.19.1
Bugzilla 2.20
Bug Flags:
approval +

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

14 years ago
We continue to improve bugzilla html/css code (see bug 240486, bug 245924, bug 
248379, bug 251068).

This is corrected header code.

These changes are tested with:

Mozilla 0.9.4  (Windows NT 5.2)
Mozilla 1.7.0  (Windows NT 5.2)

Firefox 0.9.3  (Windows NT 5.2)

Opera 6.06 (Windows NT 5.2)
Opera 7.51 (Windows NT 5.2)

MSIE 5.0 (Windows NT 5.2)
MSIE 5.5 (Windows NT 5.2)
MSIE 6.0 (Windows NT 5.2)

Netscape 4.78 (Windows NT 5.2)

Links 0.98 (Windows NT 5.2)
Lynx 2.8.5rel.1 (Windows NT 5.2)
(Assignee)

Comment 1

14 years ago
Created attachment 171057 [details] [diff] [review]
Corrected header HTML code.
(Assignee)

Comment 2

14 years ago
Created attachment 171058 [details] [diff] [review]
Corrected header CSS code.
(Assignee)

Comment 3

14 years ago
Created attachment 171059 [details] [diff] [review]
Corrected header CSS code.
Attachment #171059 - Flags: review?(myk)
(Assignee)

Comment 4

14 years ago
Comment on attachment 171058 [details] [diff] [review]
Corrected header CSS code.

duplicate for patch 171059
Attachment #171058 - Attachment is obsolete: true
Comment on attachment 171057 [details] [diff] [review]
Corrected header HTML code.

>+    [% IF h1 %]
>+      <h1><div>[% h1 %]</div></h1>
>+    [% END %]

How come there's a div inside the h1?  The h1 is itself a block level element
that can be styled the same as the div can, so enclosing the content in this
additional block doesn't seem to add anything but complexity.
(Assignee)

Comment 6

14 years ago
Created attachment 172904 [details] [diff] [review]
div -> span

Oh, sorry, it's my fault. According to HTML 4.01 DTD <Hn> elements can not
contain block-level elements. So I replaced <div> to <span> in previous patch.

<span> elements may be used for applying addition styling.
Attachment #171057 - Attachment is obsolete: true
Attachment #172904 - Flags: review?(myk)

Comment 7

14 years ago
Why do we need al this bloat in our markup? CSS Zen Garden is fun, but Bugzilla
should not try to emulate such markup.
(In reply to comment #7)
> Why do we need al this bloat in our markup? CSS Zen Garden is fun, but 
> Bugzilla should not try to emulate such markup.

The goal of Bugzilla CSSification (bug 253449) is to separate style from
structure and make Bugzilla style customizable and skinnable.  In general, our
markup should be as clean and simple as possible, and these patches have been
achieving that, replacing archaic markup with modern equivalents and removing
most non-essential markup but including some optional additional markup that
adds useful customizability/skinnability.
> Oh, sorry, it's my fault. According to HTML 4.01 DTD <Hn> elements can not
> contain block-level elements. So I replaced <div> to <span> in previous patch.

It wasn't that I was concerned about so much, it's that the span seems
unnecessary.  After all, any style that can be applied to that span could be
applied to the enclosing header tag instead, and the result would be exactly the
same, so the span isn't necessary for styling of the content within the headers.
Anne brought up an additional point on IRC.  Regarding the intro and outro
elements, a simpler approach, and the more standard one, is to have stylers use
the :before and :after pseudo-elements to provide additional style before and
after the message.  This approach obviates the need for additional markup
without sacrificing functionality (f.e. it's still possible to add styled
content before and after the message).  We should adopt it.
Attachment #171059 - Flags: review?(myk)
Comment on attachment 172904 [details] [diff] [review]
div -> span

This looks good except for the issues raised in comment 9 and comment 10.  Fix
those, and I'll review and approve!
Attachment #172904 - Flags: review?(myk) → review-

Comment 12

14 years ago
(In reply to comment #9)
> > Oh, sorry, it's my fault. According to HTML 4.01 DTD <Hn> elements can not
> > contain block-level elements. So I replaced <div> to <span> in previous 
patch.
> 
> It wasn't that I was concerned about so much, it's that the span seems
> unnecessary.  After all, any style that can be applied to that span could be
> applied to the enclosing header tag instead, and the result would be exactly 
the
> same, so the span isn't necessary for styling of the content within the 
headers.

You may apply more styles to two elements whan one, e.g. you may underline 
header with two different borders. We may show examples of such styling.
Severity: enhancement → blocker

Comment 13

14 years ago
(In reply to comment #10)
> Anne brought up an additional point on IRC.  Regarding the intro and outro
> elements, a simpler approach, and the more standard one, is to have stylers 
use
> the :before and :after pseudo-elements to provide additional style before and
> after the message.  This approach obviates the need for additional markup
> without sacrificing functionality (f.e. it's still possible to add styled
> content before and after the message).  We should adopt it.

This is not implemented in MSIE and partially implemented in Gecko-based 
browsers. See bug 280459.

Comment 14

14 years ago
(In reply to comment #12)
> You may apply more styles to two elements whan one, e.g. you may underline 
> header with two different borders. We may show examples of such styling.

And you might want five. That is no reason to Add 4 nested SPAN elements inside
a DIV element by default, just to make sure everyone can at least apply styles 5
layers deep.

I really do not like the patches provided so for. (Both here and in related
bugs.) Bugzilla should use semantic markup, not nested DIV tag-soup. 

Comment 15

14 years ago
(In reply to comment #14)
> (In reply to comment #12)
> > You may apply more styles to two elements whan one, e.g. you may underline 
> > header with two different borders. We may show examples of such styling.
> 
> And you might want five. That is no reason to Add 4 nested SPAN elements
> inside a DIV element by default, just to make sure everyone can at least
> apply styles 5 layers deep.
> 
> I really do not like the patches provided so for. (Both here and in related
> bugs.) Bugzilla should use semantic markup, not nested DIV tag-soup. 

As you wish. We may remove additional markup and provide new patch-set with 
minimal clear markup.
(Assignee)

Comment 16

14 years ago
Created attachment 174399 [details] [diff] [review]
Markup is minimized and cleared up.

We removed unnecessary tags. Shall we provide new patch for removing
unnecessary tags in our patches (bug 240486, bug 245924, bug 248379 and bug
251068) that was before?
Attachment #172904 - Attachment is obsolete: true
Attachment #174399 - Flags: review?(myk)
Severity: blocker → enhancement
Comment on attachment 174399 [details] [diff] [review]
Markup is minimized and cleared up.

>> ... a simpler approach, and the more standard one, is to have stylers use
>> the :before and :after pseudo-elements to provide additional style before
>> and after the message.
>
>This is not implemented in MSIE and partially implemented in Gecko-based 
>browsers. See bug 280459.

That's certainly a problem of that approach, but in this case I think we're
better off pushing the standard than working around it.  And demonstrating this
capability in Bugzilla is a great way to get the Mozilla developers to fix
those lingering Gecko bugs, since they use Bugzilla day in and day out.


>Shall we provide new patch for removing unnecessary tags in our patches
>(bug 240486, bug 245924, bug 248379 and bug 251068) that was before?

Yes, that would be a good idea, since it makes those fixes consistent with this
one.


>+<div id="message">[% message %]</div>

Nit: indent the div two spaces as the table was indented before.

I had trouble applying this patch, but I was able to apply it by hand, and it
looks and works great along with its corresponding CSS fixes.  r=myk
Attachment #174399 - Flags: review?(myk) → review+
Vitaly and Svetlana, do you have CVS write access these days, or do you need me
to check this in for you?
Flags: approval+
Target Milestone: --- → Bugzilla 2.20

Comment 19

14 years ago
(In reply to comment #18)
> Vitaly and Svetlana, do you have CVS write access these days, or do you
> need me to check this in for you?

No, we have no check-in rights (can we get it?). Please, commit patch by 
yourself.

> No, we have no check-in rights (can we get it?). Please, commit patch by 
> yourself.

Ok, checked in:

Checking in skins/standard/global.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v  <--  global.css
new revision: 1.9; previous revision: 1.8
done
Checking in template/en/default/global/header.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/header.html.tmpl,v
 <--  header.html.tmpl
new revision: 1.36; previous revision: 1.35
done

You can certainly get CVS write access.  Just follow the instructions on the
following page:

http://www.mozilla.org/hacking/getting-cvs-write-access.html

I'll vouch for you, and you don't need super-reviewer approval since you're only
checking into Bugzilla code.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
Assignee: myk → light
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.