Edgewall Software
Modify ↓

Opened 14 years ago

Closed 14 years ago

#9461 closed defect (fixed)

layout.html omits text and comment nodes from templates' <header> tag

Reported by: thomas.moschny@… Owned by: osimons
Priority: normal Milestone: 0.12.1
Component: general Version: 0.11-stable
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:
Internal Changes:

Description

If one includes layout.html in another template, elements of the <head> tag are copied, see source:branches/0.11-stable/trac/templates/layout.html#L27​, however this does not copy text or comment nodes.

For example, if other template looks like this: 

<html xmlns="http://www.w3.org/1999/xhtml"
      xmlns:xi="http://www.w3.org/2001/XInclude"
      xmlns:py="http://genshi.edgewall.org/">
  <xi:include href="layout.html" />
  <xi:include href="macros.html" />

  <head>
    <title>some title</title>

    ${Markup('&lt;!--[if IE]&gt;')}
    <script type="text/javascript" src="only-for-ie.js"></script>
    ${Markup('&lt;![endif]--&gt;')}
  </head>
[...]

the comments to be found by the IE are removed. (Of course the javascript file itself could test for IE, but that's not the point here.)

The issue can be fixed like this:

  • trac/templates/layout.html

     
    2424    ${Markup('&lt;!--[if lt IE 7]&gt;')}
    2525    <script type="text/javascript" src="${chrome.htdocs_location}js/ie_pre7_hacks.js"></script>
    2626    ${Markup('&lt;![endif]--&gt;')}
    27     ${select("*[local-name() != 'title']")}
     27    ${select("*[local-name() != 'title']|text()|comment()")}
    2828  </head></py:match>
    2929
    3030  <py:match path="body" once="true" buffer="false"><body>

Similar problem in source:branches/0.11-stable/trac/templates/layout.html#L31​, and of course on trunk.

osimons asked me to open a ticket also to discuss how branches and backports are handled now.

Attachments (0)

Change History (8)

comment:1 by thomas.moschny@…, 14 years ago

N.B. The template uses Markup() instead of a "real" comment, because on my first try with a "real" comment, Genshi told me the template itself was invalid. (And layout.html itself also uses Markup() for the IE conditional so I thought that'd be the way to go.) However I'm not able to reproduce this anymore - but I'm on a different machine now, maybe a different Genshi version.

Anyhow, comments are not copied either.

So for Markup() one needs text() in the selector, for comments one needs comment(). Both are currently missing.

comment:2 by osimons, 14 years ago

Cc: osimons added

Thanks Thomas!

I think both text and comments should be included of course, and was tempted to just apply it to trunk. I resisted however, as perhaps it should be a 0.12-branch for this with forward porting to trunk? Also, I'm unsure about the policy for backporting to 0.11 - and certainly for a fix that then is not included in released the 0.12.0…

comment:3 by Christian Boos, 14 years ago

trunk is still 0.12.1dev for now, so feel free to commit directly there (patch looks good).

0.11-stable is mostly "closed", but if you're really convinced that the patch can't possibly break anything, you could also commit it there (source:branches/0.11-stable​) and port that to trunk the usual forward way (cd trunk; svn merge -c XXXX ^/branches/0.11-stable).

in reply to:  3 comment:4 by osimons, 14 years ago

Cc: osimons removed
Owner: set to osimons

Replying to cboos:

… but if you're really convinced that the patch can't possibly break anything…

Aren't we always?? If things break though, I'll make sure to find someone else to blame - starting with Thomas ;-)

comment:5 by osimons, 14 years ago

Actually, reviewing the other select() calls we make, for body we just do ${select('*|text()')} so that comments are ignored there too. Same with theme.html that for body also just inserts back elements and text into the <div id="main"> section.

The patch I'm currently testing locally for 0.11.x is then:

  • trac/templates/layout.html

    a b  
    2424    ${Markup('&lt;!--[if lt IE 7]&gt;')}
    2525    <script type="text/javascript" src="${chrome.htdocs_location}js/ie_pre7_hacks.js"></script>
    2626    ${Markup('&lt;![endif]--&gt;')}
    27     ${select("*[local-name() != 'title']")}
     27    ${select("*[local-name() != 'title']|text()|comment()")}
    2828  </head></py:match>
    2929
    3030  <py:match path="body" once="true" buffer="false"><body>
    31     ${select('*|text()')}
     31    ${select('*|text()|comment()')}
    3232
    3333    <script type="text/javascript" py:if="chrome.late_links">
    3434      <py:for each="link in chrome.late_links.get('stylesheet')">

  • trac/templates/theme.html

    a b  
    5555        </py:choose>
    5656      </div>
    5757
    58       ${select('*|text()')}
     58      ${select('*|text()|comment()')}
    5959    </div>
    6060
    6161    <div id="footer" xml:lang="en"><hr/>

Looks good so far, but I'll test it a bit more.

comment:6 by Christian Boos, 14 years ago

If you add comment() in the other selects, then please evaluate the performance impact of those changes.

comment:7 by osimons, 14 years ago

I've tested with Apache Bench on a number of pages - both Trac and plugins. I cannot find any measurable difference. I suspect that the templates are generally not over-documented with comments, and the special <!--! genshi internal comment --> is not included in the selection anyway.

I also tested comments in a #!html block, but that gets stripped away before it hits template output so this patch makes no difference with regards to #6508 either.

comment:8 by osimons, 14 years ago

Milestone: → 0.12.1
Resolution: → fixed
Status: new → closed

Committed in [10029] to 0.12-stable.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain osimons.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from osimons to the specified user.

Add Comment
E-mail address and name can be saved in the Preferences .
Attachments ↑ Description ↑
 
Note: See TracTickets for help on using tickets.