252810 - p1, critical not displayed in red when groups are used for bug.

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Jason Pyeron]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.0; .NET CLR 1.0.3705; .NET CLR 1.1.4322)
Build Identifier: 

wtf?

if I unclick all my groups for the bug, that is make it public, then it will 
show up as red in my list.



Reproducible: Always
Steps to Reproduce:

Comment 1

[Jason Pyeron]

it looks like it is a CSS issue


  <tr class="bz_[% bug.bug_severity FILTER css_class_quote %]
             bz_[% bug.priority FILTER css_class_quote %]
             bz_[% bug.bug_status FILTER css_class_quote %]
             bz_[% bug.resolution FILTER css_class_quote %]
             [%+ "bz_secure" IF bug.isingroups %]">

Comment 2

[Jason Pyeron]

Attachment: css patch

drop the color: black;

Comment 3

[Joel Peshkin]

Comment on attachment 154134 [details] [diff] [review]
css patch

r=joel, but remove the commented out line for checkin

Comment 4

[Joel Peshkin]

Comment on attachment 154134 [details] [diff] [review]
css patch

Second opinion wanted from a CSS expert since I recall	 some browsers had an
issue with combining classes like this.

Comment 5

[Christian Reis]

Comment on attachment 154134 [details] [diff] [review]
css patch

It's likely that the background is set to black to ensure readability with the
gray background. It's also the case that it's good accessibility <wink> to
provide a color when you specify a background color.

I'm curious as to how many secure bugs are common on Jason's installation to
understand if the inconsistency in colouring overrides the fact that private
bugs already stand out on their own.

Comment 6

[Jason Pyeron]

we have too many cooks and too many kitchens.

every product represents a different client, each client has a login or 2, 3 etc


it would not be wise to have cross access.


so yes there are a lot

Comment 7

[Joel Peshkin]

kiko, the same is true of my site.  I actually already applied this patch and I
did try it with mozilla, firefox, and ie5

Comment 8

[Dave Miller [:justdave]]

Comment on attachment 154134 [details] [diff] [review]
css patch

I agree with kiko, we want to apply the black foreground color to ensure
readability, since we're changing the background color, and the user's
foreground color might be unreadable on our background.

css cascading order is what is killing us.  The real solution here is to make
sure that if bz_secure is necessary, it is the first class in the list instead
of the last.  That way, the color:red from bz_critical will override the
color:black from bz_secure because it is applied last.

Comment 9

[Dave Miller [:justdave]]

(In reply to comment #8)
> css cascading order is what is killing us.  The real solution here is to make
> sure that if bz_secure is necessary, it is the first class in the list instead
> of the last.  That way, the color:red from bz_critical will override the
> color:black from bz_secure because it is applied last.

grrr...  but that gets us into other hot water because bz_enhancement sets the
background color to white and the foreground to gray, and we probably want the
secureness of a bug to override in that case...?

Comment 10

[Joel Peshkin]

Well, then,  we could either build the presentation into the templates so that
we get ...

<tr style="{ background-color: lightgrey; color: red; }"> without invoking any
external stylesheets

or

we could define styles for every possible combination of severity, priority, and
security.

Comment 11

[Dave Miller [:justdave]]

maybe we need two classes each for severity and secure...

applied in this order:
1) secure foreground
2) severity background
3) severity foreground
4) secure background

Although that would wind up with gray on gray for an enhancement, which wouldn't
be so hot.

Comment 12

[Marc Schumann [:Wurblzap]]

You could use something other than the background as a way to mark something. 
I'm thinking along the lines of

.bz_secure {
   background: url("littleLockImage.png") center left no-repeat;
   padding-left: 20px;
}

For alignment, the padding would be needed on other lines, too.

Comment 13

[Christian Reis]

Marc's idea is the best one. Who has an icon handy?

Comment 14

[Dave Miller [:justdave]]

Hmm, yeah, I like that idea.  We could probably get away with using the one
Firefox uses for secure web pages, since it's already in our cvs tree :)  Then
again, it's kinda ugly ;)

Comment 15

[Christian Reis]

There's one I could ask for permission here:

  http://old.no/icon/system/

(the small one). Is it likeable?

I've written to Gerv to see what he can do about padlocks in the Mozilla code.

Comment 16

[Christian Reis]

I received explicit permission from Tor Gjerde to use his padlock icon in
Bugzilla, so I'm archiving that email (it went to Joel and Dave as well) and
working on implementing it as the secure bug indicator.

Comment 17

[Christian Reis]

Bug 192898 is related. I have a fix in hand.

Comment 18

[Christian Reis]

Attachment: kiko_v1: add padlock, stripe buglist

This patch adds a padlock, adds some padding to ensure it fits into place, and
stripes the background of the buglist in alternating colours.

There's a good rationale for striping:

  - it makes reading the buglist a *LOT* easier
  - it makes it very easy to tell what bug the padlock is related to (which is
really a consequence of the point above)
  - practically *every* site and toolkit on earth uses it as a readability
enhancement -- it raises familiarity.

The second point is what really made me do it here. The patch is live and can
be seen by querying 
  
  http://www.async.com.br/~kiko/mybugzilla/query.cgi

(just click on search, no need to do anything to the form). You *must* have an
account created to test -- and your account must be in the Everybody group.
    
The image being added is available at http://old.no/icon/system/mini-lock.gif
-- I just converted it to a PNG and placed it in images/

Comment 19

[Christian Reis]

Comment on attachment 154615 [details] [diff] [review]
kiko_v1: add padlock, stripe buglist

I know this is a bit more than we asked for here, but IMO it's the best way to
solve this problem, and it's a problem we've had for a long time now.

Comment 20

[Christian Reis]

Note that back in bug 195415 we had positive feedback on this sort of
striping/alternating background.

We could consider using a light blue background to avoid clashing with
enhancement's gray background color (in reality, it ends up being the same
problem over and over again); it doesn't bother me right now though.

Comment 21

[Christian Reis]

Attachment: 155 bytes of padlock.png

Comment 22

[Joel Peshkin]

With 2.19 and firefox, the locks show op in every cell on even rows and never
onodd rows.

Comment 23

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

Comment on attachment 154615 [details] [diff] [review]
kiko_v1: add padlock, stripe buglist

As Joel mentioned, the lock icons seem to be showing up next to every cell for
even rows and not at all for odd rows.	They should be showing up only at the
beginning of rows (next to the bug ID fields) or as tiling background images
behind entire rows.

Also note the recommendation in bug 195415 comment 19 to use either #eff7ff or
#e6e6e6 as the background color.

Comment 24

[Christian Reis]

Attachment: kiko_v2: do it right

This should work correctly -- I had a couple of rules set wrong. 

Myk, can you tell #e9e9e9 and #e6e6e6 apart without special equipment <wink>?

Comment 25

[Christian Reis]

Attachment: kiko_v3: make it work with recent browsers too

This does things correctly. It's odd; my FF 0.8.0 only styled the actual TR
itself; more modern versions of Mozilla/FF style all the TDs inheriting from
it. Maybe it was a spec compliance fix. Anyway, this works in both and in IE as
well -- I had to refrain from using :first-child because, well, IE SUCKS.

Comment 26

[Christian Reis]

Attachment: kiko_v4: CTC

This actually works when change-multiple is in action. 

I removed this bizarre comment that lived there because AFAICT it's not true
any longer if it ever was.

Comment 27

[Joel Peshkin]

Comment on attachment 154734 [details] [diff] [review]
kiko_v3: make it work with recent browsers too

works with firefox 0.9.1
no locks in knoqueror 3.1-12
works in moz1.5
works in ie6
html validation got no worse
css validation ok

Comment 28

[Christian Reis]

Comment on attachment 154735 [details] [diff] [review]
kiko_v4: CTC

Got the guts for this one? :-)

Comment 29

[Marc Schumann [:Wurblzap]]

Fwiw, this works great for me with Opera 7.23.

Comment 30

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

Comment on attachment 154735 [details] [diff] [review]
kiko_v4: CTC

Looks good, r=myk.

A couple notes: in "change multiple" mode, it still makes sense to put the lock
icon next to the bug ID to make it clear that it's the bug which is "locked"
and not the editing of the bug.

Also, the icon is a bit unobtrusive in the lists I've tested it in.  It may
make sense to tile it across the background of the row to make it more obvious
(despite its drawbacks, this obviousness was a benefit of the old approach).

>can you tell #e9e9e9 and #e6e6e6 apart without special equipment

Not consciously, but it would make sense to use a commonly-used standard.

Comment 31

[Christian Reis]

(In reply to comment #30)
> A couple notes: in "change multiple" mode, it still makes sense to put the lock
> icon next to the bug ID to make it clear that it's the bug which is "locked"
> and not the editing of the bug.

Okay, that's already being done -- it's present in edit multiple mode.

> Also, the icon is a bit unobtrusive in the lists I've tested it in.  It may
> make sense to tile it across the background of the row to make it more obvious
> (despite its drawbacks, this obviousness was a benefit of the old approach).

I'd rather we not do that right now -- the end-result is quite tough on the eyes.

Approval request is now really standing -- Dave?

Comment 32

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

> > A couple notes: in "change multiple" mode, it still makes sense to put the 
> > lock icon next to the bug ID to make it clear that it's the bug which is 
> > "locked" and not the editing of the bug.
> 
> Okay, that's already being done -- it's present in edit multiple mode.

It's present, but it sits next to the checkbox rather than the bug ID, so the
connection between it and the bug isn't as strong, and there's a possibility of
users misconstruing the icon as connected to the checkbox itself, i.e. something
about the editing of the bug or the selection of the bug for editing is "locked."

Comment 33

[Marc Schumann [:Wurblzap]]

Um, as long as this is not checked in yet: how about styling not only odd lines
as bz_odd, but even lines as bz_even as well? Sites would then be able to create
not only on/off striping, but a/b striping, too.

Moreover, I personally think one-line results look better unstyled. What do you
think -- would it perhaps be better to apply the styling to the even lines
instead of the odd ones as a default?

Comment 34

[Christian Reis]

Attachment: kiko_v5: add bz_even and padlock next to bug id

Added a bz_even class and placed the padlock where it should be. This was the
patch checked in on trunk.

Comment 35

[Christian Reis]

Trunk:

/cvsroot/mozilla/webtools/bugzilla/css/buglist.css,v  <--  buglist.css
new revision: 1.3; previous revision: 1.2
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/table.html.tmpl,v 
<--  table.html.tmpl
new revision: 1.17; previous revision: 1.16

Branch:

/cvsroot/mozilla/webtools/bugzilla/css/buglist.css,v  <--  buglist.css
new revision: 1.2.4.1; previous revision: 1.2
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/table.html.tmpl,v 
<--  table.html.tmpl
new revision: 1.16.2.1; previous revision: 1.16

Thanks.

Comment 36

[Jussi Sirpoma]

Maybe this is a bit soon, but did the padlock.png get checked in?

Comment 37

[Christian Reis]

Oops. Thanks for reminding me of that slight detail <wink>.

/cvsroot/mozilla/webtools/bugzilla/images/padlock.png,v  <--  padlock.png
initial revision: 1.1

Fixed branch to use image in toplevel directory as well:

/cvsroot/mozilla/webtools/bugzilla/Attic/padlock.png,v  <--  padlock.png
new revision: 1.1.2.1; previous revision: 1.1
/cvsroot/mozilla/webtools/bugzilla/css/buglist.css,v  <--  buglist.css
new revision: 1.2.4.2; previous revision: 1.2.4.1

Thanks.

Comment 38

[Jason Pyeron]

padlock.png causes a bang head and repeat in IE see bug 260411