232397 - .bz_obsolete shouldn't specify "underline"

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[:glob ✱]

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:

Comment 1

[:glob ✱]

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

Comment 2

[Dave Miller [:justdave]]

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

Comment 3

[:glob ✱]

(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.

Comment 4

[Dave Miller [:justdave]]

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

Comment 5

[:glob ✱]

(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.

Comment 6

[Dave Miller [:justdave]]

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.

Comment 7

[:glob ✱]

(In reply to comment #6)
> because <strike> is deprecated

ah :)

replacing <strike> with <span ..> functions as expected wrt underline inheritance.

Comment 8

[:glob ✱]

Attachment: put bz_obsolete in <span> and remove <strike>

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 9

[Christian Reis]

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
:-)

Comment 10

[Christian Reis]

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

Comment 11

[:glob ✱]

(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.

Comment 12

[:glob ✱]

Attachment: put bz_obsolete in <span> and remove <strike> v2

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 13

[Christian Reis]

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.

Comment 14

[Gervase Markham [:gerv]]

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 15

[Christian Reis]

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?

Comment 16

[Gervase Markham [:gerv]]

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

Gerv

Comment 17

[:glob ✱]

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 %]

Comment 18

[Christian Reis]

Gerv?

Comment 19

[Gervase Markham [:gerv]]

Comment on attachment 141219 [details] [diff] [review]
 put bz_obsolete in <span> and remove <strike>  v2

I guess this is OK.

Gerv

Comment 20

[Dave Miller [:justdave]]

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 21

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 22

[:glob ✱]

Attachment: put bz_obsolete in <span> and remove <strike> v3

with myk's fixes.

Comment 23

[Myk Melez [:myk] [@mykmelez]]

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

Comment 24

[:glob ✱]

(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 25

[:glob ✱]

Attachment: put bz_obsolete in <span> and remove <strike> v4

Comment 26

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 27

[:glob ✱]

Attachment: put bz_obsolete in <span> and remove <strike> v5

addresses myk's latest comments.

thanks for your patience.

Comment 28

[Myk Melez [:myk] [@mykmelez]]

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

Comment 29

[:glob ✱]

(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 30

[Myk Melez [:myk] [@mykmelez]]

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

Comment 31

[Christian Reis]

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.

Comment 32

[Christian Reis]

Attachment: kiko_bustage

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