Change <a name> anchors to use @id

RESOLVED FIXED in Bugzilla 3.6

Status

()

Bugzilla
Bugzilla-General
P1
enhancement
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: GPHemsley, Assigned: GPHemsley)

Tracking

unspecified
Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +
approval3.6 +

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
(This is the offshoot bug from bug 529201 comment 7 that neither I nor Max got around to filing until now.)

Using <a name> wrappers to specify anchors has long been deprecated behavior and, as noted in bug 529201 comment 6, support for using @id for this purpose goes all the way back to IE5 (but not NN4).

Thus, I think it is time that these obsolete tags are removed and replaced with the more modern usage of @id. For one thing, it would help prevent problems such as that of bug 546338.

Comment 1

7 years ago
Yes, agreed, if @id works then there's no reason to use <a name>. (This won't happen for 3.6, though, which is feature-frozen, and until somebody takes assignment of this bug, then we won't TM it.)
Severity: trivial → enhancement
Priority: -- → P1
Target Milestone: Bugzilla 3.6 → ---
(Assignee)

Updated

7 years ago
Blocks: 546353
(Assignee)

Comment 2

7 years ago
Created attachment 427028 [details] [diff] [review]
Change <a name> to @id (excluding release notes)

This patch touches 17 template files, changing instances of <a name> to use @id instead. I've modified the surrounding markup in some places to have things make a little more sense (e.g. using <h3> instead of <b> or <p><b>).

This patch does not touch any other elements that use @name, since they have a valid usecase on form elements. This patch also doesn't touch the release notes template, as that has 99 lines worth of <a name> instances. I'll have a separate patch for that later.
Assignee: general → gphemsley
Status: NEW → ASSIGNED
Attachment #427028 - Flags: review?

Comment 3

7 years ago
Comment on attachment 427028 [details] [diff] [review]
Change <a name> to @id (excluding release notes)

>=== modified file 'template/en/default/bug/edit.html.tmpl'
>-        <table class="status" cellspacing="0" cellpadding="0">
>+        <table id="bug_status_bottom"
>+          class="status" cellspacing="0" cellpadding="0">

  Nit: Our more normal spacing is like:

 <table id=
        class=

>=== modified file 'template/en/default/bug/votes/list-for-user.html.tmpl'

  This template is now extensions/Voting/template/en/default/pages/voting/user.html.tmpl.

>=== modified file 'template/en/default/pages/fields.html.tmpl'

  This file is awaiting an enormous patch in bug 529201 that's just about ready, so you might want to base your work on that patch instead.

>=== modified file 'template/en/default/pages/quicksearch.html.tmpl'
>=== modified file 'template/en/default/reports/components.html.tmpl'
>+  <tr id="[% comp.name FILTER html %]">

  You know, come to think of it, this doesn't have to be a valid id--it can start with a number or anything imaginable. Also, it can have just about *any* character in it.

>=== modified file 'template/en/default/reports/duplicates.html.tmpl'
>+<h3 id="explanation">
>+  What are "Most Frequently Reported [% terms.Bugs %]"?
> </b>

  Cool. But </b> -> </h3>.
Attachment #427028 - Flags: review? → review-
(Assignee)

Comment 4

7 years ago
Created attachment 427042 [details] [diff] [review]
Change <a name> to @id (excluding release notes), v2

Hopefully, this addresses your concerns. It was applied after attachment 425729 [details] [diff] [review]. Otherwise, same conditions apply as before.
Attachment #427028 - Attachment is obsolete: true
Attachment #427042 - Flags: review?

Updated

7 years ago
Depends on: 529201

Updated

7 years ago
Attachment #427042 - Flags: review? → review-

Comment 5

7 years ago
Comment on attachment 427042 [details] [diff] [review]
Change <a name> to @id (excluding release notes), v2

The only thing that's fixed in this patch is the spacing, and everything else seems to be the same.

Comment 6

7 years ago
  By the way, you'll want to request review from specific people, or there's a chance that your review request won't be gotten to. See our development process here for details:

  http://wiki.mozilla.org/Bugzilla:Developers
(Assignee)

Comment 7

7 years ago
Created attachment 427067 [details] [diff] [review]
Change <a name> to @id (excluding release notes), v3

Forgot to pull the latest revisions when I generated the last patch. This one should do it.
Attachment #427042 - Attachment is obsolete: true
Attachment #427067 - Flags: review?(mkanat)

Comment 8

7 years ago
Comment on attachment 427067 [details] [diff] [review]
Change <a name> to @id (excluding release notes), v3

Okay, this looks good! :-)
Attachment #427067 - Flags: review?(mkanat) → review+

Updated

7 years ago
Flags: approval+
Whiteboard: [waiting on checkin of blocker before being checked in]
Target Milestone: --- → Bugzilla 3.8
(Assignee)

Comment 9

7 years ago
Created attachment 427074 [details] [diff] [review]
Change <a name> to @id in release notes

Separate patch for release notes, since there were 99 lines of change. This was done mostly via a couple of regexps.
Attachment #427074 - Flags: review?(mkanat)
won't this generate invalid id's?

eg. css id's can't start with a number, however a filename can start with a number..

  <table id="[% file.filename FILTER html %]


see http://www.w3.org/TR/CSS2/syndata.html#tokenization
(Assignee)

Comment 11

7 years ago
(In reply to comment #10)
> won't this generate invalid id's?

Yes, it will. But the existing code already does, too. So we decided we'll address that issue in another bug, after this one has been applied.

Comment 12

7 years ago
Comment on attachment 427074 [details] [diff] [review]
Change <a name> to @id in release notes

Sweet! :-) Looks good. :-)
Attachment #427074 - Flags: review?(mkanat) → review+

Comment 13

7 years ago
Thanks for the patch, gphemsley! :-)

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified template/en/default/account/auth/login.html.tmpl
modified template/en/default/account/prefs/email.html.tmpl
modified template/en/default/admin/params/common.html.tmpl
modified template/en/default/attachment/diff-file.html.tmpl
modified template/en/default/attachment/list.html.tmpl
modified template/en/default/bug/comments.html.tmpl
modified template/en/default/bug/dependency-tree.html.tmpl
modified template/en/default/bug/edit.html.tmpl
modified template/en/default/bug/create/create-guided.html.tmpl
modified template/en/default/list/table.html.tmpl
modified template/en/default/pages/fields.html.tmpl
modified template/en/default/pages/quicksearch.html.tmpl                       
modified template/en/default/pages/release-notes.html.tmpl
modified template/en/default/reports/components.html.tmpl
modified template/en/default/reports/duplicates.html.tmpl
modified template/en/default/reports/keywords.html.tmpl
modified template/en/default/search/boolean-charts.html.tmpl
Committed revision 6998.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Whiteboard: [waiting on checkin of blocker before being checked in]

Updated

7 years ago
Blocks: 537789

Updated

7 years ago
Blocks: 500853

Comment 14

7 years ago
I backported just the release notes patch to 3.6, so that the trunk release notes and the branch release notes will be identical.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/3.6/                         
modified template/en/default/pages/release-notes.html.tmpl
Committed revision 7001.
Flags: approval3.6+
Target Milestone: Bugzilla 3.8 → Bugzilla 3.6

Updated

7 years ago
Blocks: 577407

Updated

5 years ago
Blocks: 536583
You need to log in before you can comment on or make changes to this bug.