Edgewall Software
Modify ↓

Ticket #9461 (closed defect: fixed)

Opened 20 months ago

Last modified 18 months ago

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:
Release Notes:
API 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

Change History

comment:1 Changed 20 months ago by thomas.moschny@…

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 Changed 20 months ago by osimons

  • 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 follow-up: Changed 20 months ago by cboos

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

comment:4 in reply to: ↑ 3 Changed 20 months ago by osimons

  • 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 Changed 20 months ago by osimons

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 Changed 20 months ago by cboos

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

comment:7 Changed 20 months ago by osimons

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 Changed 18 months ago by osimons

  • Milestone set to 0.12.1
  • Resolution set to fixed
  • Status changed from new to closed

Committed in [10029] to 0.12-stable.

View ↑

Add a comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
The resolution will be deleted. Next status will be 'reopened'
to The owner will be changed from osimons. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

Attachments ↑
 
Note: See TracTickets for help on using tickets.