Closed Bug 547171 Opened 14 years ago Closed 10 years ago

Remove all presentational markup from Bugzilla for HTML5 compliance

Categories

(Bugzilla :: User Interface, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: GPHemsley, Assigned: LpSolit)

References

()

Details

(Keywords: html5, meta, student-project)

Attachments

(3 obsolete files)

The link details all of the obsolete markup considered presentational. Bugzilla has a ton of such markup. This bug will get multiple patches to eliminate these atrocities.

I'll try to keep them short, sweet, and to the point.
Attached patch Starting point (obsolete) — Splinter Review
All patches in this bug should be applied after attachment 427539 [details] [diff] [review].

This starting point patch adds some basic default styling to take care of some of the more obvious and repetitive presentational markup. (I anticipate having to modify this some more as development goes on.)

This patch also brings the header (and footer, which needed no changes) up to code, which is always a good starting point.
Attached patch Remove <th align="left"> (obsolete) — Splinter Review
This removes all instances of <th align="left">, since that should be the default alignment anyway.
  Okay, each of these should go on a separate bug, actually.

  Also, you're going to want to request review for them, or they'll just sit here.

(In reply to comment #1)
> All patches in this bug should be applied after attachment 427539 [details] [diff] [review].

  Actually, I think we're going to do that last. If this all works, browsers will render the html5 identically as they do the html4.
Depends on: 547311
(In reply to comment #3)
>   Okay, each of these should go on a separate bug, actually.
> 
>   Also, you're going to want to request review for them, or they'll just sit
> here.

Alright, I'll break them up by attribute and then request reviews when all the patches for that attribute are ready.

> (In reply to comment #1)
> > All patches in this bug should be applied after attachment 427539 [details] [diff] [review] [details].
> 
>   Actually, I think we're going to do that last. If this all works, browsers
> will render the html5 identically as they do the html4.

Per the logic presented in IRC, that sounds fair.
Keywords: meta
Comment on attachment 427734 [details] [diff] [review]
Remove <th align="left">

This patch is now attached to bug 547311 as attachment 427833 [details] [diff] [review].
Attachment #427734 - Attachment is obsolete: true
Attachment #427728 - Flags: review?(mkanat)
Comment on attachment 427728 [details] [diff] [review]
Starting point

>=== modified file 'skins/contrib/Dusk/global.css'
>+/* Bad web designers. Bad. */
>+table.for-layout {
>+    border-spacing: 0em;
>+    border-width: 0px;
>+}

  Hmm. Why not just make that the default table CSS, and then fix any tables that it messes up?

  Also, the comment is amusing, but it will be in Bugzilla forever, so it should probably go. :-)

>+form tr th:first-child {

  first-child doesn't work in older browsers, for sure. I'm pretty sure it doesn't even work in IE7.
Attachment #427728 - Flags: review?(mkanat) → review-
(In reply to comment #6)
> (From update of attachment 427728 [details] [diff] [review])
> >=== modified file 'skins/contrib/Dusk/global.css'
> >+/* Bad web designers. Bad. */
> >+table.for-layout {
> >+    border-spacing: 0em;
> >+    border-width: 0px;
> >+}
> 
>   Hmm. Why not just make that the default table CSS, and then fix any tables
> that it messes up?

Because tables aren't supposed to be for layout. Tables actually used for tabular data (i.e. used properly) should have their borders and padding show as normal.

>   Also, the comment is amusing, but it will be in Bugzilla forever, so it
> should probably go. :-)

It shouldn't be in there forever, because the tables-for-layout shouldn't be in there forever. ;)

> >+form tr th:first-child {
> 
>   first-child doesn't work in older browsers, for sure. I'm pretty sure it
> doesn't even work in IE7.

True. However, my thinking was that the browsers that don't support it would just get left-aligned header cells, which is certainly not the end of the world.
(In reply to comment #7)
> Because tables aren't supposed to be for layout. Tables actually used for
> tabular data (i.e. used properly) should have their borders and padding show as
> normal.

  Even so, when I have a real table, I want to control the layout totally, I don't want to have the browser's default formatting on it.

  Also, in reality, there are some types of layout that are difficult-to-impossible to do without tables. (I am *really* good with CSS, and this is true even for me.)

> It shouldn't be in there forever, because the tables-for-layout shouldn't be in
> there forever. ;)

  They may be, though. In any case, let's remove the comment.

> True. However, my thinking was that the browsers that don't support it would
> just get left-aligned header cells, which is certainly not the end of the
> world.

  Well, yes, but in this case, "browsers that don't support it" represent 50% of the Internet. Also, why only the first-child, for this?
(In reply to comment #8)
> (In reply to comment #7)
> > Because tables aren't supposed to be for layout. Tables actually used for
> > tabular data (i.e. used properly) should have their borders and padding show as
> > normal.
> 
>   Even so, when I have a real table, I want to control the layout totally, I
> don't want to have the browser's default formatting on it.

Alright, but I can tell you that that's not the case with current real tables in Bugzilla.

>   Also, in reality, there are some types of layout that are
> difficult-to-impossible to do without tables. (I am *really* good with CSS, and
> this is true even for me.)

Yeah, I really wish they would do something about that....

> > It shouldn't be in there forever, because the tables-for-layout shouldn't be in
> > there forever. ;)
> 
>   They may be, though. In any case, let's remove the comment.

Should I replace it with something else, or just remove it entirely?

> > True. However, my thinking was that the browsers that don't support it would
> > just get left-aligned header cells, which is certainly not the end of the
> > world.
> 
>   Well, yes, but in this case, "browsers that don't support it" represent 50%
> of the Internet. Also, why only the first-child, for this?

Hmm... well, it's for the cases where you have a form and a two-column table, with the left column (<th>) for the title of the input and the right column (<td>) for the input itself.
(In reply to comment #9)
> >   Even so, when I have a real table, I want to control the layout totally, I
> > don't want to have the browser's default formatting on it.
> 
> Alright, but I can tell you that that's not the case with current real tables
> in Bugzilla.

  Okay, so maybe we should have a patch to fix that first. I really want tables to default to having 0 cell spacing/padding.

> Should I replace it with something else, or just remove it entirely?

  Hmm, let's just remove it entirely for now.

> > of the Internet. Also, why only the first-child, for this?
> 
> Hmm... well, it's for the cases where you have a form and a two-column table,
> with the left column (<th>) for the title of the input and the right column
> (<td>) for the input itself.

  Oh, do we have a fair number of those?
(In reply to comment #10)
> (In reply to comment #9)
> > >   Even so, when I have a real table, I want to control the layout totally, I
> > > don't want to have the browser's default formatting on it.
> > 
> > Alright, but I can tell you that that's not the case with current real tables
> > in Bugzilla.
> 
>   Okay, so maybe we should have a patch to fix that first. I really want tables
> to default to having 0 cell spacing/padding.

OK, if that's what you want, it's not a problem to change it to all tables.
 
> > > of the Internet. Also, why only the first-child, for this?
> > 
> > Hmm... well, it's for the cases where you have a form and a two-column table,
> > with the left column (<th>) for the title of the input and the right column
> > (<td>) for the input itself.
> 
>   Oh, do we have a fair number of those?

Approximately 75.
Attached patch Starting point, v2 (obsolete) — Splinter Review
This should address the issues with the previous patch.
Attachment #427728 - Attachment is obsolete: true
Attachment #427907 - Flags: review?(mkanat)
Keywords: student-project
Comment on attachment 427907 [details] [diff] [review]
Starting point, v2

  This patch by itself can't be accepted, because it adjusts the CSS of global elements without fixing the rest of Bugzilla.

>+table th, table td {
>+    padding: 0em;
>+}

  Also, that should just be "th" and "td"--they only appear in tables, and setting them as just a simple element makes it easier to override the style.

>+form tr th.form-label {
>+	text-align: right;
>+}

  Similarly, that should just be form th.form-label, although I don't think that form-label is a good class name, because there are lots of form labels that align different ways.
Attachment #427907 - Flags: review?(mkanat) → review-
Target Milestone: Bugzilla 4.0 → Bugzilla 5.0
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.
Target Milestone: Bugzilla 4.4 → Bugzilla 5.0
Depends on: 840407
Depends on: 920681
Depends on: 322402
Depends on: bz-css
Depends on: 952793
Depends on: 952796
Attachment #427907 - Attachment is obsolete: true
All done. All templates are (should be) valid HTML5 pages now.
Assignee: ui → LpSolit
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.