Edgewall Software
Modify ↓

Opened 11 years ago

Closed 10 years ago

#11126 closed enhancement (fixed)

Some templates could override layout (and theme) stylesheets with own stylesheets

Reported by: dawuid@… Owned by: dawuid@…
Priority: normal Milestone: 1.1.2
Component: rendering Version: 1.0.1
Severity: trivial Keywords: layout template stylesheet css
Cc: Branch:
Release Notes:

All stylesheets are added using add_stylesheet rather than using a link element in the template.

API Changes:

trac.web.chrome:add_stylesheet accepts arbitrary link attributes as trailing keyword arguments, and media is no longer a positional argument.

Internal Changes:

Description

Some templates put css stylesheets after layout stylesheets. That means that templates stylesheets can override styles defined in themes. Other templates add their own stylesheets through add_stylesheet. In this way layout stylesheets are written in head tag after them (so they override styles). I think that the last behavior is the valid one (and the first one is due to changes not yet committed).

Templates that I can see with ¿old behavior?:

I have attached a patch that change some (error.html and prefs_pygments.html are too complex for me) of these templates to add_stylesheet behavior.

Attachments (2)

layout.patch​ (3.7 KB ) - added by dawuid@… 11 years ago.
Modify some templates to converge to add_stylesheet behavior
pygments_styles_transfer.png​ (48.4 KB ) - added by Ryan J Ollos 10 years ago.

Download all attachments as: .zip

Change History (17)

by dawuid@…, 11 years ago

Attachment: layout.patch​ added

Modify some templates to converge to add_stylesheet behavior

comment:1 by dawuid@…, 11 years ago

Type: defect → enhancement

I'm deeply wrong. Add stylesheet in template doesn't conflict with ​official way to use themes. Only affects when using ​ThemeEnginePlugin. I've changed Type form bug to enhancement since is not a bug (but I hope that the change can be accepted). Sorry for that.

comment:2 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Cc: ryan.j.ollos@… added

comment:3 by Ryan J Ollos, 10 years ago

Cc: ryan.j.ollos@… removed
Milestone: → next-dev-1.1.x
Owner: set to Ryan J Ollos
Status: new → assigned

comment:4 by Ryan J Ollos, 10 years ago

The issue with diff_view was fixed on the trunk in #11406.

comment:5 by Ryan J Ollos, 10 years ago

As far as I can see, the advantages to using add_stylesheet rather than putting a link directly in the template are:

  • Stylesheets will always be loaded before scripts.
  • Using add_stylsheet offers the potential for manipulation in post_process_request.
  • Consistency in the codebase, and the majority of the codebase seems to use add_stylesheet.

I've tried out the suggested changes and made a few additional changes (log:rjollos.git:t11126). There is a problem with change to the Pygments preferences page in Chrome 31 - the page never renders in the browser. Everything seems normal in Firefox 26.

in reply to:  5 comment:6 by Jun Omae, 10 years ago

I think the changes in [1e14ab0f/rjollos.git] are API Changes for such a usage. 

    add_stylesheet(req, 'fooplugin/css/base.css', 'text/css', 'screen print')

comment:7 by Ryan J Ollos, 10 years ago

I hadn't worried about that since the changes are targeted to the trunk, but I can drop that changeset if you think it would be better.

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

comment:8 by Jun Omae, 10 years ago

Okay. The usage of add_stylesheet without keyword arguments is rare. I've searched and the same usage in trac-hacks doesn't exist.

by Ryan J Ollos, 10 years ago

Attachment: pygments_styles_transfer.png​ added

in reply to:  5 comment:9 by Ryan J Ollos, 10 years ago

Replying to rjollos:

There is a problem with change to the Pygments preferences page in Chrome 31 - the page never renders in the browser. Everything seems normal in Firefox 26.

The issued seemed to be that monokai.css and pastie.css weren't being transferred.

After reinstalling Pygments the issue seems to be resolved. I'm not sure if it was installed in development mode originally.

comment:10 by Ryan J Ollos, 10 years ago

Milestone: next-dev-1.1.x → 1.1.2

comment:11 by Ryan J Ollos, 10 years ago

As a side-note, the page source is much easier to read after the following change, which results in line breaks after each link element:

  • trac/templates/layout.html 

    diff --git a/trac/templates/layout.html b/trac/templates/layout.html
index 9d3316e..97d2ff4 100644
    a b  
    2121    </script><![endif]-->
    2222    <py:if test="chrome.links">
    2323      <py:for each="rel, links in chrome.links.items()">
    24         <link rel="${rel}" py:for="link in links" py:attrs="link" />
     24        <py:for each="link in links" py:with="type = link.pop('type', None)">
     25          <link rel="${rel}" type="${type}" py:attrs="link" />
     26        </py:for>
    2527      </py:for>
    2628    </py:if>
    2729    <py:if test="not defined('trac_error_rendering') and 'SEARCH_VIEW' in perm"

As an example, here is the relevant content from the Syntax Highlighting preferences page before and after the change: 

<link rel="stylesheet" type="text/css" href="/tracdev/chrome/common/css/trac.css" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/autumn.css" title="Autumn" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/borland.css" title="Borland" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/bw.css" title="Bw" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/colorful.css" title="Colorful" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/default.css" title="Default" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/emacs.css" title="Emacs" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/friendly.css" title="Friendly" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/fruity.css" title="Fruity" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/manni.css" title="Manni" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/monokai.css" title="Monokai" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/murphy.css" title="Murphy" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/native.css" title="Native" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/pastie.css" title="Pastie" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/perldoc.css" title="Perldoc" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/rrt.css" title="Rrt" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/tango.css" title="Tango" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/trac.css" title="Trac" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/vim.css" title="Vim" /><link rel="stylesheet" type="text/css" href="/tracdev/pygments/vs.css" title="Vs" /><link rel="stylesheet" type="text/css" href="/tracdev/chrome/common/css/prefs.css" />
        <link rel="shortcut icon" type="image/x-icon" href="/tracdev/chrome/common/trac.ico" />

<link rel="stylesheet" type="text/css" href="/tracdev/chrome/common/css/trac.css" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/autumn.css" title="Autumn" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/borland.css" title="Borland" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/bw.css" title="Bw" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/colorful.css" title="Colorful" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/default.css" title="Default" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/emacs.css" title="Emacs" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/friendly.css" title="Friendly" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/fruity.css" title="Fruity" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/manni.css" title="Manni" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/monokai.css" title="Monokai" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/murphy.css" title="Murphy" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/native.css" title="Native" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/pastie.css" title="Pastie" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/perldoc.css" title="Perldoc" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/rrt.css" title="Rrt" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/tango.css" title="Tango" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/trac.css" title="Trac" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/vim.css" title="Vim" />
<link rel="stylesheet" type="text/css" href="/tracdev/pygments/vs.css" title="Vs" />
<link rel="stylesheet" type="text/css" href="/tracdev/chrome/common/css/prefs.css" />

Should I make that change?

comment:12 by Jun Omae, 10 years ago

Why is it using type = link.pop('type', None)? That would change the items of req.chrome.links. I don't think template should change data of the template.

Either one of each link element with/without newline will be fine.

  • trac/templates/layout.html 

    diff --git a/trac/templates/layout.html b/trac/templates/layout.html
index 9d3316e..2f1d0f1 100644
    a b  
    2121    </script><![endif]-->
    2222    <py:if test="chrome.links">
    2323      <py:for each="rel, links in chrome.links.items()">
    24         <link rel="${rel}" py:for="link in links" py:attrs="link" />
     24        <py:for each="link in links">
     25          <link rel="${rel}" py:attrs="link" />
     26        </py:for>
    2527      </py:for>
    2628    </py:if>
    2729    <py:if test="not defined('trac_error_rendering') and 'SEARCH_VIEW' in perm" id="search">

in reply to:  12 comment:13 by Ryan J Ollos, 10 years ago

Replying to jomae:

Why is it using type = link.pop('type', None)? That would change the items of req.chrome.links. I don't think template should change data of the template.

Oh, sorry about that, I didn't intend to include the py:with in the patch. It was just due to some experimenting earlier when trying to figure out why the template wouldn't render (comment:9), so I had a dirty working copy when creating the patch in comment:11.

comment:14 by Ryan J Ollos, 10 years ago

API Changes: modified (diff)
Owner: changed from Ryan J Ollos to dawuid@…
Release Notes: modified (diff)

Changes committed to trunk in [12352:12353].

comment:15 by Ryan J Ollos, 10 years ago

Resolution: → fixed
Status: assigned → closed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain dawuid@….
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from dawuid@… 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.