Opened 6 years ago

Closed 6 years ago

#5306 enhancement closed fixed (fixed)

HTML5 elements for twisted.web.template

Reported by: Corbin Simpson Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: web Keywords: patch
Cc: jknight, Tristan Seligmann Branch: branches/html5-5306
branch-diff, diff-cov, branch-cov, buildbot
Author: MostAwesomeDude

Description

While moving some HTML5-savvy code to Twisted Web, I discovered that a few fancy new tags were missing from the tag list, so I added them. Patch attached.

Attachments (2)

patch-5306.diff (2.1 KB) - added by Corbin Simpson 6 years ago.
Patch adding HTML5 tags to t.web.template
patch-5306.2.diff (4.7 KB) - added by Corbin Simpson 6 years ago.
Updated patch

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by DefaultCC Plugin

Cc: jknight added

Changed 6 years ago by Corbin Simpson

Attachment: patch-5306.diff added

Patch adding HTML5 tags to t.web.template

comment:2 Changed 6 years ago by Corbin Simpson

Keywords: patch review added

Opening for review. Given the original reviews for this swath of code, I'm relatively sure that no extra unit tests are needed, just a careful once-over.

comment:3 Changed 6 years ago by Tristan Seligmann

Cc: Tristan Seligmann added
Keywords: review removed
Owner: set to Corbin Simpson
Summary: HTML5 tags for twisted.web.templateHTML5 elements for twisted.web.template

I apologize in advance for the ridiculously long review given how trivial this patch is, but hopefully all of my suggestions are trivial to implement.

  1. I would suggest removing (or rather, not adding) the <xmp> element; the HTML 5 spec documents this under "non-conforming features", saying that "[it is] entirely obsolete, and must not be used by authors", and if you actually need the obsolete magic parsing semantics of <xmp>, you won't actually be able to output the string you want using t.w.t's element construction APIs.
  2. All changes to Twisted need a news fragment; I don't think it's necessary to list all of the added elements, but perhaps <audio> and <video> deserve an honorable mention.
  3. I assume you referred to a specific portion of the HTML 5 spec in order to compile the new list of elements; it would be helpful to future helionauts if you included a link to the relevant portion in a comment. Additionally, if you implement point 1 above, I'd suggest briefly mentioning that <xmp> has been excluded from the list deliberately.

Once the above points have been addressed, I believe this change is ready to be merged. I'm not sure if you're a committer or not; please reassign to me if you are not, and I will commit it to trunk.

comment:4 Changed 6 years ago by Tristan Seligmann

Since I already did the work of generating the list of added element names (no element names were removed), I thought I'd put it here in case it's useful to anyone else:

article, aside, audio, canvas, command, datalist, details, embed, figcaption, figure, footer, header, hgroup, keygen, mark, meter, nav, output, progress, rp, rt, ruby, section, source, summary, time, video, wbr, xmp

comment:5 Changed 6 years ago by Jean-Paul Calderone

I'm relatively sure that no extra unit tests are needed, just a careful once-over.

He he he. Good one.

It looks like there is little or no meaningful coverage of the attribute restriction logic on twisted.web.template.tags. The coverage tool may report that the AttributeError case is exercised, but that is only because trial accidentally triggers it during test suite collection.

Changed 6 years ago by Corbin Simpson

Attachment: patch-5306.2.diff added

Updated patch

comment:6 Changed 6 years ago by Corbin Simpson

Keywords: review added

Hi,

Attaching a new patch. Differences:

  1. <xmp> is removed.
  2. <bdi> is added.
  3. A new test case has been added, which exercises general tag lookup, failed tag lookup, HTML5 tag lookup, transparent tag lookup, and <xmp> tag lookup. This brings t.w.template coverage to 100% and adequately exercises every possible path through _TagFactory.getattr().
  4. A topfile has been added. I'm claiming this is a feature. <canvas> and <video> get mentioned.
  5. A comment explaining when the tags were last updated, and linking to the W3Schools page listing HTML5 tags, has been added. The comment also mentions <xmp>.

I went through the tag list, more exhaustively, and I found two things. One is that <bdi> was missing; I added that. The other is <isindex>. It's massively deprecated and never actually was standard. Since we also don't have <marquee>, should <isindex> be removed?

comment:7 Changed 6 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/html5-5306

(In [32848]) Branching to 'html5-5306'

comment:8 Changed 6 years ago by Jean-Paul Calderone

Author: exarkunMostAwesomeDude
Keywords: review removed
Owner: changed from Corbin Simpson to Jean-Paul Calderone

Thanks. This looks good to me. I'll merge it.

comment:9 Changed 6 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [32865]) Merge html5-5306

Author: MostAwesomeDude Reviewer: exarkun Fixes: #5306

Add audio and video tags to twisted.web.template.tags and generally try to be HTML5 compliant in the supported tag set.

Note: See TracTickets for help on using tickets.