Edgewall Software
Modify

Opened 5 years ago

Closed 5 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:
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.

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@… 5 years ago.
Modify some templates to converge to add_stylesheet behavior
pygments_styles_transfer.png (48.4 KB ) - added by Ryan J Ollos 5 years ago.

Download all attachments as: .zip

Change History (17)

Changed 5 years ago by dawuid@…

Attachment: layout.patch added

Modify some templates to converge to add_stylesheet behavior

comment:1 Changed 5 years ago by dawuid@…

Type: defectenhancement

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 Changed 5 years ago by Ryan J Ollos <ryan.j.ollos@…>

Cc: ryan.j.ollos@… added

comment:3 Changed 5 years ago by Ryan J Ollos

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

comment:4 Changed 5 years ago by Ryan J Ollos

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

comment:5 Changed 5 years ago by Ryan J Ollos

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.

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

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 Changed 5 years ago by Ryan J Ollos

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 5 years ago by Ryan J Ollos (previous) (diff)

comment:8 Changed 5 years ago by Jun Omae

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

Changed 5 years ago by Ryan J Ollos

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

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 Changed 5 years ago by Ryan J Ollos

Milestone: next-dev-1.1.x1.1.2

comment:11 Changed 5 years ago by Ryan J Ollos

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 Changed 5 years ago by Jun Omae

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">

comment:13 in reply to:  12 Changed 5 years ago by Ryan J Ollos

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 Changed 5 years ago by Ryan J Ollos

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 Changed 5 years ago by Ryan J Ollos

Resolution: fixed
Status: assignedclosed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain dawuid@….
The resolution will be deleted.
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 .
 
Note: See TracTickets for help on using tickets.