296034 - missing explanation of significance of padlock icon shown in bug list

Status: NEW

(Bugzilla :: Query/Bug List, defect)

Description

[Nino]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b) Gecko/20050217
Build Identifier: any

What does this little icon mean? 
After some Googling I found the explanation (see bug#252810) but...
I'd prefere to see just a little explaining comment 
PREFERRED on the "bug list" page (top or bottom) or
OR        on the "change columns" page 
AND       in the docs (chapter 6.6)

did I miss it somewhere? ;-)

Reproducible: Always

Steps to Reproduce:
do any search which produces a bug list containing padlock icons
Actual Results:  
unknown meaning of padlock icons 

Expected Results:  
explain the icon :-)

Comment 1

[victory <[e-mail]>]

Attachment: add description under bug list, for tip v1

Comment 2

[Teemu Mannermaa (:wicked)]

Comment on attachment 220504 [details] [diff] [review]
add description under bug list, for tip v1

>Index: template/en/default/list/table.html.tmpl
>===================================================================
>+[%- IF securedbug > 0 %]

This line errors with 

buglist.cgi: Argument "" isn't numeric in numeric gt (>) at data/template/template/en/default/list/table.html.tmpl line 580., referer: xxx/query.cgi

when no bug in the list is in a group.

>+  <p>padlock icon (<img src="images/padlock.png">) means that
>+     the bug is restricted to one or more group(s)</p>
>+[% END -%]
>\ No newline at end of file

Nit: I don't think I like this much space taken for this (p tag adds two blanks in addition to a text line). Could you try to combine this with the bug count line? Or maybe even both of them. 

I'd like to see this text as a real sentence that starts with capital letter and ends with a dot.

This makes test suite fail with

not ok 261 - template/en/default/list/table.html.tmpl contains invalid bare words (e.g. 'bug') --WARNING
#     Failed test (t/009bugwords.t at line 90)

Comment 3

[victory <[e-mail]>]

Attachment: v2 for tip, fix wicked mentioned

Comment 4

[Olav Vitters]

Comment on attachment 221489 [details] [diff] [review]
v2 for tip, fix wicked mentioned

>Index: template/en/default/list/list.html.tmpl
[..]
>@@ -117,8 +119,11 @@
>-<br>
>+[%- IF securedbug > 0 %]

Nit: Why not just 'IF securedbug'? Meaning: remove the '> 0'. In that case the securedbug = 0 should not be needed.

>+  Note that padlock icon (<img src="images/padlock.png">) means that
>+    each of the [% terms.bug %] is restricted to one or more group(s).

'each of the bug' isn't English + I think there should be a 'the' before padlock.

Suggest 'Note that the padlock icon (..) means that the bug is restricted ...'. Maybe even s/means/indicates/. With terms.bug added in there of course.

>+[% END -%]
>+</p>
> 
>Index: template/en/default/list/table.html.tmpl
[..]
>+  [%- securedbug = 1 IF bug.secure_mode -%]

For some reason this change isn't passed on to list.html.tmpl. Perhaps a bug in perl-Template? I'm using v2.14. From what I read in man Template::Manual::Variables, this should have worked.

Comment 5

[Frédéric Buclin]

Not a documentation bug. Personally, I would prefer to have this information in a tooltip with the padlock. No need to mess the UI.

Comment 6

[Eric Olson]

Attachment: v3 padlock description

This adds a tooltip to the padlock icon. That required changing the padlock from a CSS background image to an explicit IMG tag.

Comment 7

[Teemu Mannermaa (:wicked)]

Comment on attachment 388139 [details] [diff] [review]
v3 padlock description

>Index: skins/standard/buglist.css
>===================================================================
>@@ -44,10 +44,8 @@
>+td.first-child table td.bz_secure_icon { 

Hmm, is that a correct selector? Why not simply styling "td.bz_secure_icon"?

>Index: template/en/default/list/table.html.tmpl
>===================================================================
>@@ -170,7 +170,6 @@
>-             [%+ "bz_secure" IF bug.secure_mode -%]

Please, don't remove bz_secure class even though we don't use it anymore. Somebody else might want to use it to style secure bugs differently.

>@@ -180,10 +179,19 @@

This last hunk is bit-rotted and fails to apply due to all columns now having an id defined.

>+    <table cellpadding=0 cellspacing=0><tr><td class="bz_secure_icon">
>+        [% IF bug.secure_mode %]
>+            <img alt="[SEC]" height=16px src="../../images/padlock.png" 
>+                 title="Access to this [% terms.bug %] is restricted to one or more group(s)" 
>+                 width=16px >
>+        [% END %]
>+    </td><td>

Image file has incorrect path so it's not found anymore (CSS and CGI files are on different directory level).

This causes multiple validation warnings:
  Warning: <img> attribute "height" has invalid value "16px"
  Warning: <img> attribute "width" has invalid value "16px"

Previously the ID-header and bug # were aligned together and padlock icon was shown "outside" of this column. Now the icon and and ID-header are aligned, which I don't like. Please, align these as they were before.

Please intend table-tag correctly and use 2-spaces indentation since this is a template file.

Nit: I'm not a big fan of tables anymore so if this can be done without introducing yet another table then I'm all for it. :)

>       <span style="display: none">[%+ '[SEC]' IF bug.secure_mode %]</span>

Is there a need for this now that even non-CSS enabled output has the padlock icon shown? Removing this line would also fix the validation warning about empty span-tag as a bonus.

Comment 8

[Eric Olson]

Attachment: v4 padlock description

(In reply to comment #7)
> Hmm, is that a correct selector? Why not simply styling "td.bz_secure_icon"?

It was correct in the sense that it validated. I've changed it per your suggestion. In fact, I'm thinking this class isn't necessary anymore and could be removed, but I'm not 100% sure.

> Please, don't remove bz_secure class even though we don't use it anymore.
> Somebody else might want to use it to style secure bugs differently.

Done.

> Image file has incorrect path so it's not found anymore (CSS and CGI files are
> on different directory level).

Fixed. 

> This causes multiple validation warnings:
>   Warning: <img> attribute "height" has invalid value "16px"
>   Warning: <img> attribute "width" has invalid value "16px"

It didn't cause validation warnings for me on the W3C validator, but I've changed the attribute to be unitless. 

> Previously the ID-header and bug # were aligned together and padlock icon was
> shown "outside" of this column. Now the icon and and ID-header are aligned,
> which I don't like. Please, align these as they were before.

I've changed this patch to make the padlock icon get its own column. The default spacing seems a bit wide if I blow the buglist up to full width on my widescreen monitor, but I've left it at the default, mostly because I can't figure out what's driving the 60 pixel default width.

> Please intend table-tag correctly and use 2-spaces indentation since this is a
> template file.

Fixed.

> Nit: I'm not a big fan of tables anymore so if this can be done without
> introducing yet another table then I'm all for it. :)

Agreed, and this may be a better way.

> Is there a need for this now that even non-CSS enabled output has the padlock
> icon shown? Removing this line would also fix the validation warning about
> empty span-tag as a bonus.

I wasn't sure what that line was for, so I had left it alone in the previous patch. Here, I've removed it, per your suggestion.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 432461 [details] [diff] [review]
v4 padlock description

  This is a good idea--people ask about this a lot.

  However, I'm pretty sure this patch won't apply anymore, because we removed all the <col> tags from the table.

  Also, no need to remove tr.bz_secure--that might be important to people styling Bugzilla.

  Finally, width="" and height="" are, I believe, presentational and should be removed.

  If you attach a new patch, I will endeavor to review it quickly.

Comment 10

[Eric Olson]

(In reply to comment #9)
>   Finally, width="" and height="" are, I believe, presentational and should be
> removed.

I kind of disagree. The HTML spec points out that, "The height and width attributes give user agents an idea of the size of an image or object so that they may reserve space for it and continue rendering the document while waiting for the image data." I'll admit it's pretty minor for an image of this size, which will probably download fairly quickly, but it seems to be to be a good idea in general.

This just occurred to me: would CSS styling of the image do the same?

If you feel strongly about it, I'll leave it out.

Comment 11

[Max Kanat-Alexander]

  Well, I just checked the HTML5 spec, and it still has width="" and height="" in there. I don't like them, though, because if a customizer wants to change the image on the disk, then he also has to change the HTML, when really he shouldn't have to.

  As far as I know, all modern UAs will continue to render the page anyway without height="" and width="" being specified, and will just reflow when the image downloads.

Comment 12

[Eric Olson]

Attachment: v5 padlock description

This is slightly modified to remove <col> references. I've also taken out the width and height attributes for the <img> tag. For an image this small, it's really not important.

This gives the padlock icon its own column, which on my widescreen monitor is a bit wider than needed. The checkbox when changing multiple bugs is also displayed that way, so I made them the same.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 439727 [details] [diff] [review]
v5 padlock description

This looks good. However, I'm wondering--should we instead put in a <div> with the image as a background, so that it's easier for people to change it as a pure-CSS customization?

Comment 14

[Eric Olson]

You can't have a tooltip (i.e. a title attribute) on a background. It could go on the div itself, I think, but that seems awkward.

Comment 15

[Max Kanat-Alexander]

(In reply to comment #14)
> You can't have a tooltip (i.e. a title attribute) on a background. It could go
> on the div itself, I think, but that seems awkward.

  Yeah, I was thinking it would go on the div itself. I don't think it's so awkward, because even if the image changes in CSS, the significance is still the same.

  We would lose out on the alt text, though.

Comment 16

[Eric Olson]

>   We would lose out on the alt text, though.

Exactly. Which means you lose accessibility. CSS is for cosmetic styling, and the presence/absence of this image is content, not style. Respectfully, losing accessibility like this and putting the tooltip on an empty container seems like a really contrived way to allow someone to do a CSS-only customization.

Comment 17

[Max Kanat-Alexander]

Comment on attachment 439727 [details] [diff] [review]
v5 padlock description

Okay, yeah, fair enough. The "width" and "height" are still in this patch, by the way.

Comment 18

[Eric Olson]

Attachment: v6 padlock description

OK, I was sure I'd taken that out. Sorry about that. This is the same without the height and width.

Comment 19

[Max Kanat-Alexander]

Comment on attachment 442954 [details] [diff] [review]
v6 padlock description

Sweet. Looks great. Thanks! :-)

Comment 20

[Max Kanat-Alexander]

Comment on attachment 442954 [details] [diff] [review]
v6 padlock description

Hey Eric. I just went to commit this, but it doesn't apply on trunk. Could you update your patch for trunk?

Comment 21

[Max Kanat-Alexander]

Hmm, any hope of getting an updated patch?

Comment 22

[Eric Olson]

Attachment: v7 padlock description

OK, let's try this again. :) Made this against the trunk. I'm not sure why the last patch didn't work; none of the files in question seemed to have changed.

Comment 23

[Max Kanat-Alexander]

Comment on attachment 483775 [details] [diff] [review]
v7 padlock description

Okay, I'm sorry that this took SO LONG for me to get to! I just haven't been doing reviews at all, but this is the first one I'm doing now that I'm getting back to things.

When you have a lot of bug ids that are of varying lengths, this starts to look odd. I think that at least, it needs to be right-aligned inside of its column.

Also, I've CC'ed pyrzak to see if he has any ideas about how to make this look slightly better.

Comment 24

[Frédéric Buclin]

Bugzilla 4.0 is restricted to security fixes only.