375382 - When viewing a bug, obsolete attachments should be hidden by default

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement, P1)

Description

[Frédéric Buclin]

Now that we can show/hide obsolete attachments, it would be fine if obsolete attachments were hidden by default. Probably a trivial hack to do in template/en/default/attachment/list.html.tmpl.

Comment 1

[Max Kanat-Alexander]

Even better would be a user preference for it--I may have filed that already, somewhere, but I'm not sure.

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

Comment 3

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

Comment on attachment 265853 [details] [diff] [review]
patch, v1

>Index: skins/standard/global.css

>+.bz_attach_obsolete_hide {
>+    display: none;
>+}
>+
>+.bz_attach_obsolete_show {
>+    display: table-row;
>+}

You don't need both of these, you just need to add bz_attach_obsolete_hide to the element being hidden and then remove that class when you want it to appear again.  Also, I think it'd be better to have a more generic bz_hidden class that you can use in other situations as well.


>Index: template/en/default/attachment/list.html.tmpl

>     for (var i = 0; i < rows.length; i++) {
>-      if (rows[i].className.match('bz_tr_obsolete'))
>-        rows[i].style.display = toggle;
>+      if (rows[i].className.match('bz_attach_obsolete_show'))
>+        rows[i].className = 'bz_attach_obsolete_hide';
>+      else if (rows[i].className.match('bz_attach_obsolete_hide'))
>+        rows[i].className = 'bz_attach_obsolete_show';

The class attribute holds a space-separated list of values, and Bugzilla code sometimes sets the bz_private class.  But this code wipes out the bz_private class, if present, when the user clicks the Show or Hide Obsolete button.

Instead of setting the value of className to a specific string, split it into an array, add or remove the appropriate class, and then join the array back into a string, i.e.:

  for (var i = 0; i < rows.length; i++) {
    var classes = rows[i].className.split(/\s+/);
    if (classes.indexOf("bz_hidden") != -1)
      classes.splice(classes.indexOf("bz_hidden"), 1);
    else
      classes.push("bz_hidden");
    rows[i].className = classes.join(" ");
  }

Comment 5

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set "blocking3.2" tp "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment 6

[Max Kanat-Alexander]

I would really like to see this happen.

Comment 7

[timeless]

Attachment: dynamic style

this doesn't actually fix the bug, but it stops setting a classthat shouldn't exist.

it'd be fairly trivial to add a preference which would then merely fix up a few other lines....

Comment 8

[Max Kanat-Alexander]

I've decided against the user preference, by the way. I just want them hidden by default, I think that's sensible for everybody.

Comment 9

[timeless]

Attachment: dynamic style

this should do the right thing, i need a bugzilla to test

Comment 10

[Max Kanat-Alexander]

Attachment: v3

Okay, here's a very simple patch that does this.

Comment 11

[Guy Pyrzak]

Comment on attachment 359534 [details] [diff] [review]
v3

tested on IE and firefox and it works. Code looks fine to me as well

Comment 12

[Frédéric Buclin]

Hunk #3 FAILED at 132.
1 out of 3 hunks FAILED -- saving rejects to file template/en/default/attachment/list.html.tmpl.rej

Could I have an updated patch to test?

Comment 13

[Frédéric Buclin]

Note also that the JS code is already in bug 376673. You're going to conflict.

Comment 14

[Max Kanat-Alexander]

Attachment: v4

Okay, here you go.

Comment 15

[Frédéric Buclin]

Comment on attachment 361805 [details] [diff] [review]
v4

When I click "show obsolete", hidden attachments are displayed but the link remains the same. If I click it again, obsolete attachments are hidden and the link changes to "hide obsolete" despite they are already hidden (and vice versa)

Comment 16

[Max Kanat-Alexander]

Attachment: v5

Okay, here's a fix for it that's much better than checking the innerHTML (the problem was that there was now a newline and lots of spaces inside the <a>).

Comment 17

[Frédéric Buclin]

Comment on attachment 361813 [details] [diff] [review]
v5

>           [% IF Param("allow_attachment_display") %]
>-            <a href="attachment.cgi?bugid=[% bugid %]&amp;action=viewall">View All</a>
>+            | <a href="attachment.cgi?bugid=[% bugid %]&amp;action=viewall">View All</a>
>           [% END %]

Please revert this change. I removed "|" on purpose while fixing one of the security bugs. If a bug has no obsolete attachment, this bar is displayed despite there is no text on its left, which is not nice. And the UI is fine without it in all cases.

Tested successfully with Fx3.1b2, Opera 9.63 and IE 6 SP1. r=LpSolit (I wonder how much time will elapse before someone requests the show/hide status to be stored in a cookie.)

Comment 18

[Max Kanat-Alexander]

Okay, I did the checkin fix.

Checking in template/en/default/attachment/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.41; previous revision: 1.40
done

Comment 19

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.4 in bug 494037.