Opened 8 years ago

Closed 5 years ago

Last modified 5 years ago

#3695 closed (fixed)

duplicate html id's in doc pages

Reported by: Simon G. <dev@…> Owned by: simeon
Component: contrib.admin Version: master
Severity: Keywords: admin, docs, xhtml
Cc: dev@…, sf@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Whilst trying to fix #2411 (Validation errors in template filters/tags documentation) I came across this minor issue. The templates loop over each tag/filter, and title them with something like <h3 id="{{ tag.name|escape }}">{{ tag.name|escape }}</h3>.

When there are two template tags named the same, then there will be an id clash, causing an (x)html validation error (and the anchor tags won't work properly for the two duplicate id's):

# Error  Line 525 column 12: ID "filter" already defined.

    <h3 id="filter">filter</h3>

An "id" is a unique identifier. Each time this attribute is used in a document it must have a different value. If you are using this attribute as a hook for style sheets it may be more appropriate to use classes (which group elements) than id (which are used to identify exactly one element).

Currently, this is only an issue in template_tag_index.html, where we have a filter tag in the defaulttags section and in the admin_list section.

It can be fixed by prepending the library name to the id, so that each one will be unique. I'll add a patch once I've finished up #2411

Attachments (2)

3695.diff (1.3 KB) - added by simeon 7 years ago.
3695.2.diff (2.6 KB) - added by simeon 5 years ago.
Updated to apply against r13096

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 7 years ago by simeon

  • Owner changed from nobody to simeon
  • Status changed from new to assigned

comment:3 Changed 7 years ago by simeon

  • Has patch set

Ok, just to be clear on how to replicate the bug and verify that it is fixed:

Start a Django project with admin. Login to the Admin area and click on the "documentation" link and then on the "Tags" link to load the doc page for template tags.

On the left the index lists the tags for various modules with links that jump to the relevant tag based on ID. This works as long at there are no duplicate tag names - the admin_list module, however, defines a "filter" tag that then uses the same ID as the built in filter tag. Clicking on either link entitled filter in the index brings you to the built in filter tag (and as mentioned the page is now not valid HTML).

I applied the fix Simon G recommended by prefixing the ID and link target with the relevant module name (or built_in for the built in filters). See 3695.diff. Upon applying navigation ought to work properly and ID/link target elements should be inspected (yea firebug) to verify that they are constructed properly.

Changed 7 years ago by simeon

comment:4 Changed 7 years ago by simeon

  • Cc sf@… added

comment:5 Changed 7 years ago by mattmcc

  • milestone set to 1.0

comment:6 Changed 7 years ago by mtredinnick

  • milestone 1.0 deleted

As best I understand it, this doesn't create a display problem, it doesn't cause Django to crash, it doesn't prevent anything from actually working. It's a slight annoyance in some edge-cases creating invalid output at the markup level on the documentation page (only). If that's all correct, it can wait until after 1.0.

comment:7 Changed 7 years ago by mattmcc

I'll grant it's pretty minor, but there is one thing that doesn't work: linking to a particular tag/filter when more than one exists in the page. In most browsers, links will always go to the first occurrence of the id.

comment:8 Changed 5 years ago by adamnelson

  • Patch needs improvement set

All of admin_doc has been refactored - either this is no longer an issue or at the least the patch needs to be redone.

comment:9 Changed 5 years ago by adamnelson

Actually, I was looking at the wrong admin_doc directory (Which should be deleted). The patch still needs to be updated though to apply to the new location of admin_doc.

Changed 5 years ago by simeon

Updated to apply against r13096

comment:10 Changed 5 years ago by simeon

Ok - I updated the patch to apply to latest trunk - I also added the same fix to the filters template. However in testing it I noticed that in current trunk built-in filters and tags are being shown, only tags and filters that belong to modules. I'll open (or find an existing) ticket for this...

comment:11 Changed 5 years ago by adamnelson

  • Triage Stage changed from Accepted to Ready for checkin

comment:12 Changed 5 years ago by adamnelson

  • Patch needs improvement unset

comment:13 Changed 5 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [13728]) Fixed id attribute generation in the admin docs page. Patch from simeon.

Fixed #3695. Thanks.

comment:14 Changed 5 years ago by kmtracey

(In [13737]) Adjust AdminDocTests to run after r13728. Also match comments to tests and add test that was there in comment form only.Refs #3695.

comment:15 Changed 5 years ago by mtredinnick

(In [13747]) [1.2.X] Fixed id attribute generation in the admin docs page. Patch from simeon.

Fixed #3695. Thanks.

Backport of r13728 from trunk.

comment:16 Changed 5 years ago by mtredinnick

(In [13750]) [1.2.X] Adjust AdminDocTests to run after r13728. Also match comments to tests and add test that was there in comment form only.Refs #3695.

Backport of r13737 from trunk.

Note: See TracTickets for help on using tickets.
Back to Top