User-Agent: Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040125 Firebird/0.8.0+ (stipe) since the upgrade to b.m.o, css/edit_bug.css now has: .bz_obsolete { text-decoration: line-through underline; } if you browse with underline links disabled, this looks weird, as no other links are underlined. is it possible to remove the underline attribute please? Reproducible: Always Steps to Reproduce:

actually it would be better if obsolete links used <strike> instead of the bz_obsolete class.

How about doing this? .bz_obsolete { text-decoration: line-through inherit; } which should in theory add line-through to whatever existing decoration it would have otherwise had instead of replacing the entire decoration set...

(In reply to comment #2) > How about doing this? > .bz_obsolete { text-decoration: line-through inherit; } nope, doesn't work. css text decoration isn't inherited.

how about if we apply bz_obsolete to a span which encloses the link instead of applying it to the link itself?

(In reply to comment #4) > how about if we apply bz_obsolete to a span which encloses the link instead of > applying it to the link itself? if you're going to do that, why not drop bz_obsolete and enclose the link in <strike>? this is how strikethrough is done elsewhere in bz.

because <strike> is deprecated and you're not supposed to use it anymore, which is the reason we've been converting them bit by bit, so our HTML will validate without warnings.

(In reply to comment #6) > because <strike> is deprecated ah :) replacing <strike> with <span ..> functions as expected wrt underline inheritance.

this patch puts bz_obsolute in a surrounding <span>, plus replaces all occurances of <strike> with css - bz_inactive, bz_notopen and bz_obsolete.

Comment on attachment 140457 [details] [diff] [review] put bz_obsolete in <span> and remove <strike> >+++ bugzilla-2.17.6-nostrict/Bugzilla/Template.pm Tue Feb 3 09:17:19 2004 >@@ -185,7 +185,9 @@ > # and t/004template.t. > FILTERS => { > # Render text in strike-through style. >- strike => sub { return "<strike>" . $_[0] . "</strike>" }, >+ inactive => sub { return '<span class="bz_inactive">' . $_[0] . "</span>" }, >+ notopen => sub { return '<span class="bz_notopen">' . $_[0] . "</span>" }, >+ obsolete => sub { return '<span class="bz_obsolete">' . $_[0] . "</span>" }, Are these extra filters actually useful? >+++ bugzilla-2.17.6-nostrict/checksetup.pl Tue Feb 3 09:07:40 2004 >@@ -1069,7 +1069,9 @@ > # These don't actually need to do anything here, just exist > FILTERS => > { >- strike => sub { return $_; } , >+ inactive => sub { return $_; } , >+ notopen => sub { return $_; } , >+ obsolete => sub { return $_; } , Any idea why this is needed? I don't get why we should be providing a special provider for precompiling templates.. but then again, I don't know much about TT. bugzilla-2.17.6-nostrict/template/en/default/attachment/show-multiple.html.tmpl Tue Feb 3 09:09:02 2004 >@@ -47,7 +47,7 @@ > <tr> > <td valign="top"> > [% IF a.isobsolete %] >- <strike>[% a.description FILTER html %]</strike> >+ <span class="bz_obsolete">[% a.description FILTER html %]</span> Can't you use more than one filter for the same item? It says so here: Chaining Filters Multiple FILTER directives can be chained together in sequence. They are called in the order defined, piping the output of one into the input of the next. [% PROCESS somefile FILTER truncate(100) FILTER html %] http://template-toolkit.org/docs/plain/Manual/Syntax.html The rest looks fine. I'll be happy to r+ a fixed patch with some explanations :-)

Gerv, see if the approach is acceptable to you, and perhaps explain why we need that fake provider in checksetup.pl..

(In reply to comment #9) > > # Render text in strike-through style. > >-strike => sub { return "<strike>" . $_[0] . "</strike>" }, > >+inactive => sub { return '<span class="bz_inactive">' . $_[0] . "</span>" }, > >+notopen => sub { return '<span class="bz_notopen">' . $_[0] . "</span>" }, > >+obsolete => sub { return '<span class="bz_obsolete">' . $_[0] . "</span>" }, > > Are these extra filters actually useful? right now only inactive is used. > Any idea why this is needed? nope :) > Can't you use more than one filter for the same item? i tried to match the original code, so where a <strike> tag was specified explicitly, i converted to a <span>. where a filter was used, i replace with the updated one. now that i know you can chain filters i'll update the patch to always use filters to apply the style.

all templates now use a filter to apply the style. i don't know if i'm doing it the correct way, this is my first bz patch.

Comment on attachment 141219 [details] [diff] [review] put bz_obsolete in <span> and remove <strike> v2 Interesting :-) I'm asking Gerv and Myk because I think they may have an opinion on the style used here, and perhaps a different suggestion. I do appreciate the fact that it does look cleaner than repeating the span and hardcoding the classes; I'm just thinking there may be a way to avoid even the IFs you're using to define the style.

The way this should work is as follows (IMO) <span class="[% "bz_obsolete" IF bug.obsolete %] [%+ "bz_foo" IF bug.foo %]">... etc. But IE doesn't support multiple classes, does it? Gerv

Comment on attachment 141219 [details] [diff] [review] put bz_obsolete in <span> and remove <strike> v2 >- [% "<strike>" IF NOT bug.opened %] >- <a href="show_bug.cgi?id=[% bug.id %]"> >- [% bug.id %]</a> >- [% "</strike>" IF NOT bug.opened %] >+ [% IF NOT bug.opened %] >+ [% FILTER style = notopen %][% END %] >+ [% ELSE %] >+ [% FILTER style = none %][% END %] >+ [% END %] >+ <a href="show_bug.cgi?id=[% bug.id %]">[% bug.id FILTER style %]</a> Erhm, I was asking more about this construct. Is it too un-HTML-ish, or is it somewhere we want to go with this?

Er... I don't even understand that construct. Gerv

i wanted to avoid duplicating identical code. so instead of if (is notopen) { "do something" filter notopen } else { "do something" (no filter) } i create a temporary filter called "style" set to obsolete/notopen/whatever if required, or set to "none" which is a noop filter. so the above pseudo code becomes if (is notopen) { style = notopen } else { style = none } "do something" filter style the FILTER assignment doesn't support conditional statements, and always requires an END tag, hence the assignment lines look like: [% FILTER style = notopen %][% END %]

Gerv?

Comment on attachment 141219 [details] [diff] [review] put bz_obsolete in <span> and remove <strike> v2 I guess this is OK. Gerv

I'm going to hold for Myk's review on this one, since he's the UI owner... myk: there's already a review pending with your name on it here.

Comment on attachment 141219 [details] [diff] [review] put bz_obsolete in <span> and remove <strike> v2 >+ notopen => sub { return '<span class="bz_notopen">' . $_[0] . "</span>" }, Nit: this should really be "closed" rather than "notopen". >+ [% IF attachment.isobsolete %] >+ [% FILTER style = obsolete %][% END %] >+ [% ELSE %] >+ [% FILTER style = none %][% END %] >+ [% END %] >+ <a href="attachment.cgi?id=[% attachment.attachid %]&amp;action=view">[% attachment.description FILTER html FILTER style %]</a> > </td> This is OK, although a simpler approach that makes the templates more readable and uses less code is to put the condition into the filter function itself, i.e.: >+ obsolete => sub { return $_[1] ? '<span class="bz_obsolete">' . $_[0] . "</span>" : $_[0] }, >+ <a href="attachment.cgi?id=[% attachment.attachid %]&amp;action=view">[% attachment.description FILTER html FILTER obsolete(attachment.isobsolete) %]</a> Filter arguments must be simple values, so you have to predefine them for calculated values, i.e.: [% isclosed = !dep.open %] [% FILTER notopen(isclosed) %] r=myk, but I'd prefer a patch with the two fixes suggested above.

with myk's fixes.

Comment on attachment 143687 [details] [diff] [review] put bz_obsolete in <span> and remove <strike> v3 A few issues: 1. the syntax i provided to you for the filters isn't exactly correct; filters that take arguments need a somewhat different syntax (they need to have filter factories created), something like: closed => [ sub { my ($context, $text, $isclosed) = @_; return sub { $isclosed ? '<span class="bz_closed">' . $text . "</span>" : $text; }; }, 1 ] See the docs for the exact syntax: http://www.template-toolkit.org/docs/blue/Manual/Config.html#Plugins_and_Filter s 2. you have at least one syntax error in your new patch ("obsoltete") 3. i'm not sure "[% isclosed = bug.resolution != "" %]" does what you expect. it should be tested; i know "[% isclosed = (bug.resolution != "" ? 1 : 0) %]" works

(In reply to comment #23) > 3. i'm not sure "[% isclosed = bug.resolution != "" %]" does what you expect. > it should be tested; i know "[% isclosed = (bug.resolution != "" ? 1 : 0) %]" > works [% isclosed = bug.resolution != "" %] is the same as [% isclosed = (bug.resolution != "" ? "1" : "") %] so it'll work

Comment on attachment 143703 [details] [diff] [review] put bz_obsolete in <span> and remove <strike> v4 >+ inactive => [ >+ sub { >+ my($content, $inactive) = @_; $content -> $context (The $context object is the part of the Template Toolkit that stores the current state of the toolkit, with references to the template being processed, the stash of variables, etc.) Nit: for consistency with code elsewhere in Bugzilla, prepend "is" to boolean variable names, i.e.: $inactive -> $isinactive >+ return sub { >+ $inactive ? '<span class="bz_inactive">'.$_[0].'</span>' : $_[0]; Nit: for clarity, explicitly return the value, i.e.: return sub { return $inactive ? '<span class="bz_inactive">'.$_[0].'</span>' : $_[0]; } Conflicting changes to list-for-user.html.tmpl got checked in yesterday that this patch needs to account for. Resolve those conflicts, change $content to $context, optionally fix the nits (optional but recommended), and submit a new patch which I'll grant review on and approve for checkin.

addresses myk's latest comments. thanks for your patience.

Comment on attachment 143925 [details] [diff] [review] put bz_obsolete in <span> and remove <strike> v5 In resolving the conflict in list-for-user.html.tmpl you accidentally reverted one of your changes: http://bugzilla.mozilla.org/attachment.cgi?oldid=143703&action=interdiff&newid= 143925&headers=1#bugzilla-2.17.6/template/en/default/bug/votes/list-for-user.ht ml.tmpl_sec1

(In reply to comment #28) > In resolving the conflict in list-for-user.html.tmpl you accidentally reverted > one of your changes: odd. the link you gave makes it look like i'm inserting <strike> code, but i'm not. will interdiff work given the patches are against different versions of list-for-user.html.tmpl ?

Comment on attachment 143925 [details] [diff] [review] put bz_obsolete in <span> and remove <strike> v5 Ah, indeed, interdiff is at fault here. Sorry about that. r=myk

Checked in. I took the liberty of removing the [resulting] dead <strike> CSS class in the dependency-tree. For reference, the change to that file caused closed bugs to use a white background (instead of the original #d9d9d9). Myk and I discussed and endorsed this change because it's consistent with our styling of closed bugs (and I think the legibility is better). Thanks to Byron for working with us towards a great solution.

Fixes redness I caused by not running tests before checking in :-(