Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Remove the "align" attribute from <th>

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
User Interface
--
enhancement
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: GPHemsley, Assigned: Christopher Trom)

Tracking

({html5, student-project})

unspecified
Bugzilla 5.0
html5, student-project
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 427833 [details] [diff] [review]
Remove <th align="left">

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.)

Comment 1

8 years ago
<th> defaults to centered alignment, not left, I'm pretty sure.
(Reporter)

Comment 2

8 years ago
(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.
(Reporter)

Updated

8 years ago
Attachment #427833 - Attachment is obsolete: true
Attachment #427833 - Flags: review-

Comment 3

8 years ago
you might want to look at YUI's reset css. It has a lot of stuff to help make things uniform across browsers
(Reporter)

Comment 4

8 years ago
Created attachment 427909 [details] [diff] [review]
Change <th align="right"> to <th class="form-label">

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
(Reporter)

Comment 5

8 years ago
Created attachment 427910 [details] [diff] [review]
Change <th align="left"> to <th class="form-label">

This is a companion to attachment 427909 [details] [diff] [review]. It replaces certain instances of <th align="left"> with <th class="form-label">.
(Reporter)

Updated

8 years ago
Blocks: 547389
(Reporter)

Updated

8 years ago
Keywords: student-project
(Reporter)

Updated

7 years ago
Assignee: gphemsley → ui
Status: ASSIGNED → NEW

Updated

7 years ago
Target Milestone: Bugzilla 4.0 → Bugzilla 5.0

Comment 6

5 years ago
Hum, nobody noticed these patches because they have no requests for review.

Comment 7

5 years ago
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
(Assignee)

Comment 8

5 years ago
If the patches provided are not satisfactory, I would like to work on this bug.
(Assignee)

Comment 9

5 years ago
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?

Comment 10

5 years ago
(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
(Assignee)

Comment 11

5 years ago
Created attachment 682907 [details] [diff] [review]
Replace align="right" with class="form-label"

This patch changes every instance of align="right" to class="form-label".
Attachment #682907 - Flags: review?(LpSolit)

Comment 12

5 years ago
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-
(Assignee)

Comment 13

5 years ago
Created attachment 682939 [details] [diff] [review]
Remove align=left and align=right from every <th>

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)

Updated

5 years ago
Attachment #427909 - Attachment is obsolete: true

Updated

5 years ago
Attachment #427910 - Attachment is obsolete: true

Comment 14

5 years ago
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+

Comment 15

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Summary: Remove @align from Bugzilla templates → Remove the "align" attribute from <th>

Updated

5 years ago
Blocks: 821717
(Assignee)

Comment 16

4 years ago
Created attachment 754600 [details] [diff] [review]
Remove remaining align attributes from <th>'s

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)
(Assignee)

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 17

4 years ago
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.
(Assignee)

Comment 18

4 years ago
(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.

Comment 19

4 years ago
(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.
(Assignee)

Comment 20

4 years ago
(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?

Updated

4 years ago
Attachment #754600 - Flags: review?(LpSolit) → review?(sgreen)

Comment 21

4 years ago
(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.

Updated

4 years ago
Attachment #754600 - Flags: review?(sgreen) → review-
(Assignee)

Comment 22

4 years ago
Created attachment 791591 [details] [diff] [review]
patch.diff

Moved CSS rule into global.css.
Attachment #754600 - Attachment is obsolete: true
Attachment #791591 - Flags: review?(sgreen)

Comment 23

4 years ago
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-
(Assignee)

Comment 24

4 years ago
Created attachment 794929 [details] [diff] [review]
patch.diff

Oops, sorry I missed that.
Attachment #791591 - Attachment is obsolete: true
Attachment #794929 - Flags: review?(sgreen)

Updated

4 years ago
Attachment #794929 - Flags: review?(sgreen) → review+

Updated

4 years ago
Flags: approval?

Updated

4 years ago
Flags: approval? → approval+

Comment 25

4 years ago
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
Last Resolved: 5 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.