Edgewall Software
Modify

Opened 5 years ago

Closed 3 years ago

Last modified 2 years ago

#12677 closed defect (fixed)

glitch in ITemplateStreamFilter backward compatibility support

Reported by: Christian Boos Owned by: Christian Boos
Priority: normal Milestone: 1.3.4
Component: rendering Version: 1.3dev
Severity: normal Keywords: jinja2 genshi ITemplateFilter
Cc: Branch: log:cboos.git@t12677-html5
Release Notes:
API Changes:
Internal Changes:

Description (last modified by Christian Boos)

On an instance running r15474 where the SpamFilter is active, the (render with Jinja2 / parse as HTML / apply filters / re-render with Genshi) render cycle is broken for CDATA sections, as can be seen on the /about page:

  <div id="systeminfo">
    <script type="text/javascript">//&lt;![CDATA[
      jQuery(document).ready(function ($) {
        $("#systeminfo table")
          .before("&lt;p&gt;User Agent: &lt;code&gt;" + navigator.userAgent + "&lt;/code&gt;&lt;/p&gt;")
          .append("&lt;tr&gt;&lt;th&gt;jQuery&lt;/th&gt;&lt;td&gt;" + $().jquery + "&lt;/td&gt;&lt;/tr&gt;" +
                  "&lt;tr&gt;&lt;th&gt;jQuery UI&lt;/th&gt;&lt;td&gt;" + $.ui.version + "&lt;/td&gt;&lt;/tr&gt;" +
                  "&lt;tr&gt;&lt;th&gt;jQuery Timepicker&lt;/th&gt;&lt;td&gt;" + $.timepicker.version +
                  "&lt;/td&gt;&lt;/tr&gt;");
      });
    //]]&gt;</script>

It should be:

  <div id="systeminfo">
    <script type="text/javascript">//<![CDATA[
      jQuery(document).ready(function ($) {
        $("#systeminfo table")
          .before("<p>User Agent: <code>" + navigator.userAgent + "</code></p>")
          .append("<tr><th>jQuery</th><td>" + $().jquery + "</td></tr>" +
                  "<tr><th>jQuery UI</th><td>" + $.ui.version + "</td></tr>" +
                  "<tr><th>jQuery Timepicker</th><td>" + $.timepicker.version +
                  "</td></tr>");
      });
    //]]></script>

(rendered with Trac trunk at r15474 as well, but with normal Jinja2 rendering)

Attachments (0)

Change History (21)

comment:1 by Christian Boos, 5 years ago

Description: modified (diff)

comment:2 by Christian Boos, 5 years ago

Confusion on my part, CDATA sections are an XML thing only, that's why the genshi.input.HTML parser (rightfully) ignored the CDATA.

Using genshi.input.XML instead should fix this (r15487). The page content we generate with Jinja2, should be also valid XML, still.

However, this raises the question of whether it's valid for us to continue to use CDATA sections in our Genshi templates, if we're not formally using the XHTML syntax for HTML5.

So with the current practice of using HTML5 (text/html), we should perhaps rather avoid having CDATA sections at all.

comment:3 by Christian Boos, 5 years ago

Using the Genshi XML parser to re-parse the content is more fragile than using the HTML one, it seems. It failed here on t.e.o for the /about page.

OTOH, this did catch a genuine error:

      </tbody>
    </table>
  </div>
</div>
    </div>
  </body>
      <!-- # block content (content inherited from layout.html) -->
    <!-- # endblock content (content inherited from layout.html) -->

      <!-- # endblock content (placeholder in theme.html) -->
    </div>

    <div id="footer"><hr/>
      <a id="tracpowered" href="http://trac.edgewall.org/"
         ><img src="/chrome/common/trac_logo_mini.png" height="30"
               width="107" alt="Trac Powered"/></a>
      <p class="left">
Powered by         <a href="/about"><strong>Trac 1.3.2.dev0</strong></a>
        <br />
 By         <a href="http://www.edgewall.org/">Edgewall Software</a>
.      </p>
      <p class="right">En savoir plus sur le projet Trac :<br /> <a href="http://trac.edgewall.org/">http://trac.edgewall.org/</a></p>
    </div>
    <!--   # include 'site_footer.html' (theme.html) -->
    <!--   end of site_footer.html -->
    <!-- # endblock body (content inherited from theme.html) -->
  </body>
</html>

The about.html template:

      # include 'environment_info.html'

    </div>
  </body>
  ${ super() }
  # endblock content
</html>

(I should really try to enhance the checker, see TracDev/JinjaChecker#KnownIssues)

in reply to:  3 comment:4 by Christian Boos, 5 years ago

Replying to Christian Boos:

OTOH, this did catch a genuine error: […] The about.html template:

was fixed in r15508.

comment:5 by Christian Boos, 5 years ago

I think that before release we need to go back to HTML parsing and therefore avoid using CDATA.

comment:6 by Ryan J Ollos, 4 years ago

Milestone: 1.3.21.3.3

comment:7 by Ryan J Ollos, 4 years ago

Owner: Christian Boos removed
Status: assignednew

comment:8 by Ryan J Ollos, 4 years ago

Milestone: 1.3.31.4

in reply to:  5 comment:9 by Christian Boos, 3 years ago

Owner: set to Christian Boos
Status: newassigned

Replying to Christian Boos:

I think that before release we need to go back to HTML parsing and therefore avoid using CDATA.

This is done at the beginning of this series: log:cboos.git@t12677-html5

While I was looking at the generated HTML through the nu validator, I noticed a couple of other issues. The rest of the series addresses them.

I've by far not run an exhaustive check on all the pages we generate, but the most common issues should be dealt with. We can always continue to improve upon this incrementally while working towards the 1.4 release.

comment:10 by Christian Boos, 3 years ago

Branch: log:cboos.git@t12677-html5

comment:11 by Christian Boos, 3 years ago

Well, now I realize that even without the CDATA, the generate/re-parse cycle used in presence of ITemplateStreamFilter still breaks the content of <script> tags, i.e. < and > are still escaped as &lt; and &gt;.

in reply to:  11 comment:12 by Christian Boos, 3 years ago

Replying to Christian Boos:

Well, now I realize that even without the CDATA, the generate/re-parse cycle used in presence of ITemplateStreamFilter still breaks the content of <script> tags, i.e. < and > are still escaped as &lt; and &gt;.

[6b2442e9/cboos.git] is a possible solution.

comment:13 by Christian Boos, 3 years ago

Another glitch with the generate/re-parse cycle: in this situation, we don't generate the "late" <script> elements (e.g. see source HTML for TracWorkflow for an example, search for <script> elements containing jQuery.loadStyleSheet and jQuery.loadScript near the end of the page).

in reply to:  13 comment:14 by Christian Boos, 3 years ago

… we don't generate the "late" <script> elements

Fixed in [00383b17/cboos.git].

comment:15 by Christian Boos, 3 years ago

For [f2d323202/cboos.git], some help in testing this would be appreciated (i.e. we need to test support for a plugin which would provide its own Genshi template and make use of the late links/scripts).

comment:16 by Christian Boos, 3 years ago

Milestone: 1.41.3.4
Resolution: fixed
Status: assignedclosed

The whole series committed in [16851:16865], I preferred to keep the individual changes for more focused "blame" info and easier bisecting if needed later on.

Note that the problem described initially in this ticket is only fixed late in the series, in r16863.

comment:17 by Ryan J Ollos, 3 years ago

Are prefix and suffix needed after ie_if is removed in 1.5.1? I'm working on rebasing changes in #12787.

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

in reply to:  17 ; comment:18 by Christian Boos, 3 years ago

Replying to Ryan J Ollos:

Are prefix and suffix needed after ie_if is removed in 1.5.1?

I don't see a reason for keeping them after that.

in reply to:  18 comment:19 by Ryan J Ollos, 3 years ago

Replying to Christian Boos:

I don't see a reason for keeping them after that.

Thanks, I've posted proposed changes in comment:18:ticket:12787.

comment:20 by Ryan J Ollos, 2 years ago

After r16852, translatable strings are not extracted inside script elements. See r17004.

Proposed fix:

  • trac/dist.py

    diff --git a/trac/dist.py b/trac/dist.py
    index dad6ba9ac..d3d2736ca 100644
    a b class ScriptExtractor(HTMLParser):  
    6666
    6767    def handle_starttag(self, tag, attrs):
    6868        if tag == 'script':
    69             for kv in attrs:
    70                 if kv == ('type', 'text/javascript'):
    71                     self.in_javascript = True
    72                     break
     69            self.in_javascript = True
    7370
    7471    def handle_startendtag(self, tag, attrs):
    7572        self.in_javascript = False
Last edited 2 years ago by Ryan J Ollos (previous) (diff)

comment:21 by Ryan J Ollos, 2 years ago

comment:20 change committed in r17011. New extraction in r17012.

Modify Ticket

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

Add Comment


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