Closed Bug 232397 Opened 16 years ago Closed 16 years ago

.bz_obsolete shouldn't specify "underline"

Categories

(Bugzilla :: User Interface, defect, trivial)

defect
Not set
trivial

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: glob, Assigned: glob)

References

()

Details

Attachments

(2 files, 4 obsolete files)

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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All
Attachment #140457 - Flags: review?(kiko)
Assignee: myk → bugzilla
Status: NEW → ASSIGNED
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
:-)
Attachment #140457 - Flags: review?(kiko) → review-
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.
Attachment #140457 - Attachment is obsolete: true
Attachment #141219 - Flags: review?
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.
Attachment #141219 - Flags: review?(myk)
Attachment #141219 - Flags: review?(gerv)
Attachment #141219 - Flags: review?
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
Attachment #141219 - Flags: review?(gerv) → review+
Flags: approval?
Target Milestone: --- → Bugzilla 2.18
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.
Flags: approval?
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.
Attachment #141219 - Flags: review?(myk)
with myk's fixes.
Attachment #141219 - Attachment is obsolete: true
Flags: approval?
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
Attachment #143687 - Flags: review-
(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
Attachment #143687 - Attachment is obsolete: true
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.
Attachment #143703 - Flags: review-
addresses myk's latest comments.

thanks for your patience.
Attachment #143703 - Attachment is obsolete: true
Attachment #143925 - Flags: review?(myk)
Flags: approval?
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
Attachment #143925 - Flags: review?(myk) → review-
(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
Attachment #143925 - Flags: review- → review+
Flags: approval+
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.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attached patch kiko_bustageSplinter Review
Fixes redness I caused by not running tests before checking in :-(
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.