Remove the "align" attribute from <th>

RESOLVED FIXED in Bugzilla 5.0

Status

()

enhancement
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: GPHemsley, Assigned: chtrom)

Tracking

({html5, student-project})

Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(2 attachments, 6 obsolete attachments)

Posted patch Remove <th align="left"> (obsolete) — Splinter Review
Remove the align attribute from Bugzilla templates towards the goal of removing presentational markup for HTML5 compliance.

(This attachment is identical to attachment 427734 [details] [diff] [review] from bug 547171 comment 2.)
<th> defaults to centered alignment, not left, I'm pretty sure.
(In reply to comment #1)
> <th> defaults to centered alignment, not left, I'm pretty sure.

Oh, darn it, you're right. Alright, back to the drawing board.
Attachment #427833 - Attachment is obsolete: true
Attachment #427833 - Flags: review-
you might want to look at YUI's reset css. It has a lot of stuff to help make things uniform across browsers
This changes all instances of <th align="right"> to <th class="form-label">.

This class should be used whenever there is a form in a two-column layout with a <th> column on the left (the labeler) and a <td> column on the right (the labelee).
Assignee: ui → gphemsley
Status: NEW → ASSIGNED
This is a companion to attachment 427909 [details] [diff] [review]. It replaces certain instances of <th align="left"> with <th class="form-label">.
Blocks: 547389
Keywords: student-project
Assignee: gphemsley → ui
Status: ASSIGNED → NEW
Target Milestone: Bugzilla 4.0 → Bugzilla 5.0
Hum, nobody noticed these patches because they have no requests for review.
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
If the patches provided are not satisfactory, I would like to work on this bug.
I cannot find the class (form-label) that was referenced in the previous patches. It looks to me like all that is needed is classes that apply the "text-align:left;" style instead of align="left" and "text-align:right;" instead of align="right". What naming convention should I use for these classes?
(In reply to Christopher from comment #9)
> I cannot find the class (form-label) that was referenced in the previous
> patches. It looks to me like all that is needed is classes that apply the
> "text-align:left;" style instead of align="left" and "text-align:right;"
> instead of align="right". What naming convention should I use for these
> classes?

Simply define .form-label into global.css which sets align="right". I don't think we need a class for align="left". This last one is used in showdependencygraph.cgi only, and we could as well use .form-label (i.e. right-aligned) there too.
Assignee: ui → chtrom
This patch changes every instance of align="right" to class="form-label".
Attachment #682907 - Flags: review?(LpSolit)
Comment on attachment 682907 [details] [diff] [review]
Replace align="right" with class="form-label"

You forgot to add this class in global.css. This should be part of this patch.
Attachment #682907 - Flags: review?(LpSolit) → review-
This patch removes align="right", align=right, and align="left" from every <th> tag in the template directory and adds "form th { text-align: right; }" to global.css.
Attachment #682907 - Attachment is obsolete: true
Attachment #682939 - Flags: review?(LpSolit)
Attachment #427909 - Attachment is obsolete: true
Attachment #427910 - Attachment is obsolete: true
Comment on attachment 682939 [details] [diff] [review]
Remove align=left and align=right from every <th>

Looks good. Thanks! :) r=LpSolit
Attachment #682939 - Flags: review?(LpSolit) → review+
Subsequent bugs must be filed to remove remaining align="..." attributes from other HTML elements. This bug is specific to <th>.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified skins/standard/global.css
modified template/en/default/account/auth/login.html.tmpl
modified template/en/default/account/email/confirm-new.html.tmpl
modified template/en/default/account/email/confirm.html.tmpl
modified template/en/default/account/password/set-forgotten-password.html.tmpl
modified template/en/default/account/prefs/account.html.tmpl
modified template/en/default/admin/classifications/edit-common.html.tmpl
modified template/en/default/admin/classifications/edit.html.tmpl
modified template/en/default/admin/classifications/reclassify.html.tmpl
modified template/en/default/admin/classifications/select.html.tmpl
modified template/en/default/admin/custom_fields/edit-common.html.tmpl
modified template/en/default/admin/fieldvalues/create.html.tmpl
modified template/en/default/admin/fieldvalues/edit.html.tmpl
modified template/en/default/admin/keywords/create.html.tmpl
modified template/en/default/admin/keywords/edit.html.tmpl
modified template/en/default/admin/milestones/create.html.tmpl
modified template/en/default/admin/products/confirm-delete.html.tmpl
modified template/en/default/admin/products/create.html.tmpl
modified template/en/default/admin/products/edit-common.html.tmpl
modified template/en/default/admin/products/edit.html.tmpl
modified template/en/default/admin/versions/create.html.tmpl
modified template/en/default/admin/workflow/comment.html.tmpl
modified template/en/default/admin/workflow/edit.html.tmpl
modified template/en/default/bug/dependency-graph.html.tmpl
modified template/en/default/global/choose-classification.html.tmpl
modified template/en/default/global/choose-product.html.tmpl
modified template/en/default/reports/keywords.html.tmpl
modified template/en/default/whine/mail.html.tmpl
modified template/en/default/whine/schedule.html.tmpl
Committed revision 8503.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Summary: Remove @align from Bugzilla templates → Remove the "align" attribute from <th>
Blocks: 821717
Several instances of the align attribute on <th> elements were missed in the previous patch. This patch removes those which were missed and adds class "left" to fix those which require left text alignment.
Attachment #754600 - Flags: review?(LpSolit)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 754600 [details] [diff] [review]
Remove remaining align attributes from <th>'s

>=== modified file 'skins/standard/admin.css'

>+th.left {
>+    text-align: left;
>+}

>=== modified file 'skins/standard/attachment.css'

>+th.left {
>+    text-align: left;
>+}

Why not putting th.left in global.css directly? That's where we put "form th" in the previous patch.
(In reply to Frédéric Buclin from comment #17)
> 
> Why not putting th.left in global.css directly? That's where we put "form
> th" in the previous patch.

Because putting it in these two files covered all of the known cases. If it were put in global.css, many pages that didn't need it would still load it.
(In reply to Christopher Trom from comment #18)
> Because putting it in these two files covered all of the known cases. If it
> were put in global.css, many pages that didn't need it would still load it.

But we wouldn't have to add this CSS rule elsewhere the next time we add a new table in a template which doesn't load these 2 CSS files. :) But that's not that important.
(In reply to Frédéric Buclin from comment #19)
> But we wouldn't have to add this CSS rule elsewhere the next time we add a
> new table in a template which doesn't load these 2 CSS files. :) But that's
> not that important.

Do you want me to move the rule into global.css?
Attachment #754600 - Flags: review?(LpSolit) → review?(sgreen)
(In reply to Christopher Trom from comment #20)
> Do you want me to move the rule into global.css?

Yes. I think the rule should only be defined once in CSS. If it is used in many places, then global.css is the correct place.
Attachment #754600 - Flags: review?(sgreen) → review-
Posted patch patch.diff (obsolete) — Splinter Review
Moved CSS rule into global.css.
Attachment #754600 - Attachment is obsolete: true
Attachment #791591 - Flags: review?(sgreen)
Comment on attachment 791591 [details] [diff] [review]
patch.diff

>=== modified file 'template/en/default/global/choose-product.html.tmpl'
>--- template/en/default/global/choose-product.html.tmpl	2013-08-15 23:08:20 +0000
>+++ template/en/default/global/choose-product.html.tmpl	2013-08-17 01:25:53 +0000
>@@ -37,7 +37,7 @@
> [% FOREACH c = classifications %]
>   [% IF c.object %]
>     <tr>
>-      <th colspan="2" align="left">[% c.object.name FILTER html %]:
>+      <th colspan="2">[% c.object.name FILTER html %]:
>       [%+ c.object.description FILTER html_light %]</th>
>     </tr>
>   [% END %]
>

This change needs class="left" otherwise the select new bug product page looks not so nice.
Attachment #791591 - Flags: review?(sgreen) → review-
Posted patch patch.diffSplinter Review
Oops, sorry I missed that.
Attachment #791591 - Attachment is obsolete: true
Attachment #794929 - Flags: review?(sgreen)
Attachment #794929 - Flags: review?(sgreen) → review+
Flags: approval?
Flags: approval? → approval+
Thanks for the patch.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified skins/standard/global.css
modified template/en/default/admin/table.html.tmpl
modified template/en/default/admin/classifications/del.html.tmpl
modified template/en/default/admin/components/confirm-delete.html.tmpl
modified template/en/default/admin/custom_fields/confirm-delete.html.tmpl
modified template/en/default/admin/fieldvalues/confirm-delete.html.tmpl
modified template/en/default/admin/fieldvalues/edit.html.tmpl
modified template/en/default/admin/milestones/confirm-delete.html.tmpl
modified template/en/default/admin/products/confirm-delete.html.tmpl
modified template/en/default/admin/versions/confirm-delete.html.tmpl
modified template/en/default/attachment/confirm-delete.html.tmpl
modified template/en/default/attachment/diff-file.html.tmpl
modified template/en/default/attachment/list.html.tmpl
modified template/en/default/global/choose-product.html.tmpl
Committed revision 8720.
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.