Edgewall Software
Modify

Opened 12 years ago

Last modified 3 years ago

#5997 new enhancement

Clean name-spaces for CSS for 0.11+?

Reported by: shap@… Owned by:
Priority: normal Milestone: next-major-releases
Component: general Version: devel
Severity: normal Keywords: css consider patch
Cc: leho@…, Ryan J Ollos Branch:
Release Notes:
API Changes:

Description

I suspect that it may be too late for this, but I think this is a low-risk change.

Trac uses CSS all over the place. This is good. The problem is that I need to pass through some content that *also* uses CSS, and there is a problem of name space collision in the choices of names for the class attribute. Since Trac is cutting over the template system, now is an opportune time to look at CSS issues if there is a desire to do so.

How difficult would it be for Trac to adopt the convention that all class attributes and all id attributes that are emitted from the trac templates would begin with "trac-" so that they are less likely to collide with CSS within content?

In our case, we cannot simply import the incoming content once and have done; it is a reference manual that we need to maintain externally via our version control system. Our goal is to simultaneously make it available within the Wiki for commenting and feedback. I considered re-writing the class attributes on our content using XSLT. It is doable, but it is probably beyond the skills of most XSLT authors.

Attachments (4)

T5997-css-trac-prefix.patch (157.7 KB ) - added by Peter Suter 7 years ago.
Rough mechanical adding of "trac-" to all id's
T5997-css-trac-prefix-r11382.patch (236.8 KB ) - added by Peter Suter 7 years ago.
T5997-css-prefix-script-fixups.patch (15.6 KB ) - added by Peter Suter 7 years ago.
T5997-css-prefix-script.patch (243.2 KB ) - added by Peter Suter 7 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 by Emmanuel Blot, 12 years ago

See also #4264.

comment:2 by Christian Boos, 12 years ago

Keywords: consider added
Milestone: 0.12

comment:3 by Remy Blank, 11 years ago

Milestone: 0.130.12
Owner: changed from Jonas Borgström to Remy Blank

#7932 reported a possibly frequent clash with Trac element ids (a heading like === attachments ===, which gets an attribute id="attachments", so the CSS for the attachments <div> matches), which would be solved by a clean namespace for element ids. It was closed as a duplicate.

Prefixing all element ids with e.g. trac- should not be too difficult, as ids are mostly referenced by the style sheets. However, IIRC, element classes are quite often referenced in the code, which would make the change much more painful.

I would like to at least start the process and fix the element ids. Would trac- be a good prefix?

in reply to:  3 ; comment:4 by Emmanuel Blot, 11 years ago

Replying to rblank:

I would like to at least start the process and fix the element ids. Would trac- be a good prefix?

The trouble is that this will break all the customized Trac installations. I guess there are many of them… So either this replacement has to wait for a major release (0.12) or better, provide a Python script to parse and update custom CSS file to match the new naming scheme.

in reply to:  4 comment:5 by Remy Blank, 11 years ago

Replying to eblot:

The trouble is that this will break all the customized Trac installations. I guess there are many of them… So either this replacement has to wait for a major release (0.12)

That was my intention. The CSS file updater sounds like a good idea, too, and it might even facilitate the conversion of Trac's CSS files.

comment:6 by Remy Blank, 10 years ago

#8514 was closed as a duplicate.

comment:7 by Remy Blank, 10 years ago

Milestone: 0.120.13

I guess we'll have enough incompatible changes in 0.12 without that. Let's do that later.

comment:8 by Remy Blank, 7 years ago

Owner: Remy Blank removed

Refocusing.

comment:9 by lkraav <leho@…>, 7 years ago

Cc: leho@… added

by Peter Suter, 7 years ago

Attachment: T5997-css-trac-prefix.patch added

Rough mechanical adding of "trac-" to all id's

comment:10 by Peter Suter, 7 years ago

The above patch is the result of mostly blindly grep & replacing in trac/:

  1. Replace id="([^t]|(t[^r])|(tr[^a])|(tra[^c])) by id="trac-\1 but revert changes in:
  1. Repeatedly (until no match found) replace in *.css files #(([^t]|(t[^r])|(tr[^a])|(tra[^c])).*\{) by #trac-\1.
  1. Manually fix trac/htdocs/js/wikitoolbar.js.

This would of course first need a very thorough review, testing and maybe reconsideration of exactly which id's need to be prefixed.

comment:11 by Peter Suter, 7 years ago

One id that was missed by the above was traceback (because it starts with trac). Maybe |(trac[^-]) should be added to the regular expressions above to also catch such cases.

comment:12 by Thijs Triemstra, 7 years ago

Summary: Clean name-spaces for CSS for 0.11?Clean name-spaces for CSS for 0.11+?

comment:13 by Ryan J Ollos <ryano@…>, 7 years ago

Cc: ryano@… added

comment:14 by Christian Boos, 7 years ago

Milestone: next-major-releasesnext-dev-1.1.x

r'id="(?!trac-)([^"]+)"' ;-)

The change can be done for 1.1.1.

We've not discussed yet what's going to happen for 1.1.x, but this will definitely be the place for disruptive changes like this one.

by Peter Suter, 7 years ago

in reply to:  10 comment:15 by Peter Suter, 7 years ago

Replying to psuter:

mostly blindly grep & replacing

Unsurprisingly, it's more complicated than that:

  • With the (?!trac-) guard, now tracpowered slips through instead of traceback.
  • There are some CSS id selectors not followed by a {:
    • 1x #tabs in admin.css.
    • 3x #overview in diff.css.
    • 2x #batchmod_form in report.css.
    • 3x #fieldhist, 4x #content and 2x #systeminfo in trac.css.
  • HTML label elements' for attributes contain id's.
    • for=" for="trac-
  • HTML td elements' headersattributes contain id's.
    • headers=" headers="trac-
  • Javascript / jQuery selectors contain id's.
    • (\$\("[^#"]*)#(?!trac-)\1#trac-
  • Functional tests use id's.
    • formvalue\('([^']+)'formvalue('trac-\1'
  • And more…

comment:16 by Christian Boos, 7 years ago

I found two issues:

  • $("#...")$"#trac-...")
  • in trac/ticket/templates/ticket_preview.html, id="trac-${'trac-change-%d-%d' ...

Also, you didn't modify the classes, is that saved for later or you think we shouldn't do it?

Finally, I think that the (shell/sed/Python) script you used for doing this should be part of the patch (in contrib), so that plugin authors or people having their own extensive CSS customizations could reuse it as well.

comment:17 by Peter Suter, 7 years ago

Yes, that sounds like a good idea.

by Peter Suter, 7 years ago

comment:18 by Peter Suter, 7 years ago

attachment:T5997-css-prefix-script.patch contains a Python script that does most of the automatable steps mentioned above. attachment:T5997-css-prefix-script-fixups.patch contains some fix-ups on top of that. These didn't really seem worth automating.

(The script still contains a commented out rule that used the cssutils Python module. I thought this might be cleaner and more stable. But I couldn't get it to work without also pretty-printing the CSS in a different style. So I replaced it with a custom hack again. For now I only left it in case anyone else has a good idea how to improve it.)

As for plugin and customization authors reusing the script: It might be possible to customize it for such purposes (with manual review and fixing), but getting a 100% automated solution seems unlikely to me. For small plugins and customizations it will probably be simpler to update everything entirely manually.

I still didn't modify any classes, just because getting the id's right first seems tricky enough. Actually, I'm not really sure the benefits are worth the risk. I would be surprised if there weren't still quite some id references I missed.

comment:19 by Christian Boos, 7 years ago

Great! Looks good, I'm going to integrate it in +testing.

About cssutils: if the pretty-printing it does is looking good (and keeps our comments), we could do a first pass of pretty-printing on our current css, then do the changes as a second step?

by Peter Suter, 7 years ago

comment:20 by Peter Suter, 7 years ago

(Updated patch to fix @media handling.)

The pretty-printing keeps the comments. It can't preserve some CSS hacks though.

comment:22 by Ryan J Ollos, 5 years ago

Cc: Ryan J Ollos added; ryano@… removed

comment:23 by Ryan J Ollos, 4 years ago

Milestone: next-dev-1.1.xnext-major-releases

Retargetting tickets to narrow focus for milestone:1.2. Please move the ticket back to milestone:next-dev-1.1.x if you intend to resolve it by milestone:1.2.

comment:24 by figaro, 3 years ago

Keywords: patch added

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned. Next status will be 'new'.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


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