Last Comment Bug 262275 - Gmail conversation style comments folding (expand/collapse comments)
: Gmail conversation style comments folding (expand/collapse comments)
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: User Interface (show other bugs)
: unspecified
: All All
: -- enhancement with 1 vote (vote)
: Bugzilla 3.2
Assigned To: Frédéric Buclin
: default-qa
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-09-30 06:31 PDT by Roland
Modified: 2008-07-01 00:07 PDT (History)
5 users (show)
LpSolit: approval+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Doctored screenshot of a GMail conversation collapsing (30.87 KB, image/png)
2005-11-23 03:58 PST, Roland
no flags Details
patch, v1 (3.49 KB, patch)
2006-04-21 07:26 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v2 (3.90 KB, patch)
2006-04-21 10:24 PDT, Frédéric Buclin
myk: review-
Details | Diff | Splinter Review
patch, v3 (3.83 KB, patch)
2007-03-09 12:11 PST, Frédéric Buclin
no flags Details | Diff | Splinter Review
patch, v3.1 (3.85 KB, patch)
2007-03-09 12:55 PST, Frédéric Buclin
myk: review+
Details | Diff | Splinter Review

Description Roland 2004-09-30 06:31:02 PDT
User-Agent:       Mozilla/5.0 (Windows; U; WinNT4.0; rv:1.7.3) Gecko/20040913 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; WinNT4.0; rv:1.7.3) Gecko/20040913 Firefox/0.10

Specially for long bugs, it would be extremely useful to be able to collapse bug
conversations the same way google uses collapsing in gmail conversations.

Reproducible: Always
Steps to Reproduce:




The possible solutions could range from simple DHML manipulation of the bug
page's DOM to much more complex dynamic on-demand loading of comments as they
are uncollapsed.

Latter would propably yield better page loads on larger bugs but require much
more work on the backend side ... but then again I am not that familiar with the
bugzilla backend to comment on that.
Comment 1 Frédéric Buclin 2005-01-30 18:07:43 PST
I like that!
Comment 2 Roland 2005-11-23 00:42:51 PST
Any progress on this item?
Comment 3 Myk Melez [:myk] [@mykmelez] 2005-11-23 00:46:33 PST
How does gmail do it?
Comment 4 Frédéric Buclin 2005-11-23 01:16:32 PST
When collapsed, only the header of the comment is shown:

 ----- Comment #1 From Frédéric Buclin  2005-01-30 18:07 PST  [reply] -----[+]
 ----- Comment #2 From Roland  2005-11-23 00:42 PST  [reply] -----[+]
 ----- Comment #3 From Myk Melez  2005-11-23 00:46 PST  [reply] -----[+]


Clicking on [+] will show the comment:

 ----- Comment #1 From Frédéric Buclin  2005-01-30 18:07 PST  [reply] -----[-]
I like that!

 ----- Comment #2 From Roland  2005-11-23 00:42 PST  [reply] -----[+]
 ----- Comment #3 From Myk Melez  2005-11-23 00:46 PST  [reply] -----[+]

Clicking on [-] will hide the comment again.
Comment 5 Roland 2005-11-23 03:58:15 PST
Created attachment 204030 [details]
Doctored screenshot of a GMail conversation collapsing

The screenshot I attached has been taken from a conversation with 10 messages in it and has been doctored to put it into a context of bugzilla.

The main features I'd like to point out are following:
[*] Headers show a snippet of the comment content when collapsed. Clicking anywhere on the header will either collapse or expand it. The last comment is never collapsed. (I think same should apply to highlighted comments)

[*] If there are more than 8 comments, the collapsed headers are additionally collapsed into a single pile conserve space (these are the empty lines on the screenshot). Clicking on the collapsed pile expands this pile into a list of collapsed headers. There is no way to collapse group of headers back to a "pile".

At the end of each comment (when expanded) there is a rectangle that looks like a text area for typing in the reply to this comment. Setting focus to this textarea creates a quoted text and positions text entry cursor before the quoted text. A buttons "Submit" and "Discard" appear above the text area allowing to submit new comment or discard it in which case the text inside the text area gets thrown away. There might be other controls that could be created or shown when composing a reply to the bug, that could allow more common workflow and bug editing actions to be performed along with writing a comment (accept a bug, report a duplicate, add a CC, etc...).
Clicking the "Reply" link above the text area performs same actions as focusing the textarea.

As a sidenote - this "screenshot" also demonstrates, how it *could* be possible to remove the often minboggingly difficult to understand and frightening to a novice user bug editing form monstrosity... 

PS: This is by no means a final design suggestion - more like a demonstration (or a fantasy, if you will) of what the bugzilla would look like if it were designed and implemented by google folks... Feel free to interpret and improve the design and the interaction scheme.

I believe that Bugzilla is a mighty powerful tool for managing your bugs, issues and such - it would be nice to turn that raw power into well-designed and pleasant to use interface that would help the user to harness that power...
Comment 6 Frédéric Buclin 2006-04-21 07:26:55 PDT
Created attachment 219301 [details] [diff] [review]
patch, v1

Tested successfully on Firefox 1.5.0.2. Each comment can be expanded/collapsed individually. You can also click on "Expand All Comments" and "Collapse All Comments".
Comment 7 Frédéric Buclin 2006-04-21 10:24:50 PDT
Created attachment 219321 [details] [diff] [review]
patch, v2

Much better patch, tested on Firefox, Konqueror, Internet Explorer and Opera. It fixes two major problems which were present in my first patch:
- Now IE is able to expand individual comments too;
- Expanding/collapsing all comments doesn't stop at the first private comment.
Comment 8 Roland 2006-05-18 01:07:26 PDT
This seems like a nice patch.

Although AJAX style lazy loading of comment bodies would be even nicer :)

What's the status of this patch - still waiting for review?
Comment 9 Frédéric Buclin 2006-05-18 10:00:29 PDT
(In reply to comment #8)
> What's the status of this patch - still waiting for review?

Yeah, unfortunately. Both wicked and myk are busy these days. I hope they will have some time very soon now.
Comment 10 Myk Melez [:myk] [@mykmelez] 2006-05-22 19:15:38 PDT
Comment on attachment 219321 [details] [diff] [review]
patch, v2


>+<style type="text/css">
>+  .hide_comment {display: none;}
>+  .show_comment {display: block;}
>+</style>

These should be in a Bugzilla CSS file.


>+  function coll_exp(link, comment_id) {

Functions are generally best named using the pattern <verb>_<object>, which for this function would probably be something like toggle_comment_display.


>+    var comment = document.getElementById('comment_text_' + comment_id);
>+    if (comment.className == "hide_comment") {
>+        expand_comment(link, comment);
>+    }
>+    else {
>+        collapse_comment(link, comment);
>+    }
>+    return false;
>+  }

Nit: JS doesn't treat single quotes differently from double quotes, so be consistent in use of quote marks (i.e. use either single or double, not both) unless it's necessary to distinguish between two kinds of strings.

Nit: don't use brackets around single-line blocks, i.e.:

    if (comment.className == "hide_comment")
        expand_comment(link, comment);
    else
        collapse_comment(link, comment);

Also, the "class" attribute to DOM nodes can contain multiple space-separated values, so this code should check to see if comment.className *contains* the "hide_comment" class, either via a regular expression or by splitting the value of that attribute into an array and checking each member.

Also, class names are usually best as adjectives, since they describe the nodes they are attached to, and this is no exception, so it should be "hidden" rather than "hide_comment".  The other advantage of not including the word "comment" in the class name is that we can then reuse the class for other UI elements we might hide in the future.

Better yet, use "collapsed" and "expanded" instead of "hidden" and "shown", since these comments are actually collapsed, not hidden.

Also, ideally the HTML code should look something like this:

<div class="comment">
  <span class="comment-header">...</span>
  <pre class="comment-text">...</pre>
</div>

And then we would actually attach the class to the <div> node, since that represents the comment as a whole, i.e.:

<div class="comment collapsed">...</div>

Your CSS rule could then be:

div.comment.collapsed > span.comment-header {
  display: none;
}

Also, since we're toggling what is essentially a boolean value, it would make sense to have only a single class name, "collapsed", and add/remove it as necessary.


>+  function collapse_all() {
>+    var nb_comments = [% comments.size FILTER html %];

Nit: number is usually abbreviated num, i.e. num_comments.


>+    /* If for some given ID the comment doesn't exist, this doesn't mean
>+       there are no more comments, but that the comment is private and
>+       the user is not allowed to view it. */

Nit: multi-line comments should be rendered in one of the following forms:

// line one
// line two
// line three

(This is the preferred form for comments inside a function.)

/* line one
 * line two
 * line three
 */

(This is the preferred form for comments which document a function.)


>+
>+    for (id = 0; id < nb_comments; id++) {

Loop iterators should be declared via a var statement; otherwise they pollute the global namespace:

    for (var id = 0; id < nb_comments; id++) {


>+        comment = document.getElementById('comment_text_' + id);

Variables should be declared via a var statement; otherwise they pollute the global namespace:

        var comment = document.getElementById('comment_text_' + id);

Note that the scope of variables declared via "var" is the function, so "comment" will be declared for the entire function block, even if you declare it within this for loop.  This is different from Perl, where variables are always block-scoped.  JS2's (or possibly JS1.7's) "let" statement is going to have block scope.


>+  function collapse_comment(link, comment) {
>+    link.innerHTML = "(+)";
>+    link.title = "Expand the comment";

Nit: period at the end of this complete sentence, i.e. "Expand the comment."

Nit: it would be better to use DOM-standard methods instead of innerHTML to add "(+)" to the link.


>+  function addCollapseLink(count) {
>+    document.write(' <a href="#" id="comment_link_' + count +
>+                   '" onclick="return coll_exp(this, ' +  count +
>+                   ');" title="Collapse the comment">(-)</a> ');
>+  }

Nit: it would be better to use DOM-standard methods instead of document.write.


>+  <a href="#" onclick="return collapse_all();">Collapse All Comments</a> -
>+  <a href="#" onclick="return expand_all();">Expand All Comments</a>

Instead of having the functions return false this code might be more transparent if the functions returned nothing and the click handlers returned false, i.e.:

  <a href="#" onclick="collapse_all(); return false;">Collapse All Comments</a> -
  <a href="#" onclick="expand_all(); return false;">Expand All Comments</a>
Comment 11 Myk Melez [:myk] [@mykmelez] 2006-05-22 19:18:12 PDT
> Your CSS rule could then be:
> 
> div.comment.collapsed > span.comment-header {
>   display: none;
> }

Err, I actually meant:

div.comment.collapsed > span.comment-text {
  display: none;
}
Comment 12 jey.michael 2006-11-14 13:02:45 PST
(In reply to comment #9)
> (In reply to comment #8)
> Yeah, unfortunately. Both wicked and myk are busy these days. I hope they will
> have some time very soon now.

Is this bug Fixed now?  Just anxious to start using this cool enhancement.
Comment 13 Frédéric Buclin 2006-11-14 13:36:35 PST
(In reply to comment #12)
> Is this bug Fixed now?  Just anxious to start using this cool enhancement.

No, not fixed yet. I had more critical bugs to fix. This will be implemented in Bugzilla 3.2.
Comment 14 Frédéric Buclin 2007-03-09 12:11:44 PST
Created attachment 258020 [details] [diff] [review]
patch, v3

I addressed most comments myk made in his review. I still use innerHTML and document.write(), which are both pretty common.
Comment 15 Frédéric Buclin 2007-03-09 12:55:01 PST
Created attachment 258026 [details] [diff] [review]
patch, v3.1

My previous patch was only working with Firefox and Opera. This one also works with IE and Konqueror.
Comment 16 Myk Melez [:myk] [@mykmelez] 2007-04-06 12:51:40 PDT
Comment on attachment 258026 [details] [diff] [review]
patch, v3.1

>+  /* The functions below permit to expand and collapse comments  */

Nit: "The functions below let us expand and collapse comments" or simply "The functions below expand and collapse comments."


>+    for (var id = 0; id < num_comments; id++) {
>+        var comment = document.getElementById('comment_text_' + id);
>+        if (comment) {
>+            var link = document.getElementById('comment_link_' + id);
>+            if (action == 'collapse')
>+                collapse_comment(link, comment);
>+            else
>+                expand_comment(link, comment);
>+        }
>+    }

Nit: the structure would be a bit simpler like this:

    for (var id = 0; id < num_comments; id++) {
        var comment = document.getElementById('comment_text_' + id);
        if (!comment)
            continue;

        var link = document.getElementById('comment_link_' + id);

        if (action == 'collapse')
            collapse_comment(link, comment);
        else
            expand_comment(link, comment);
    }


>+.collapsed {
>+    display: none;
>+}

XUL has a global "hidden" attribute that, when set on an element, applies the CSS "display: none" rule to it, hiding the object.  I wonder if it would make sense for us to establish the same thing in Bugzilla so that we can set hidden="true" on nodes we want to hide instead of always parsing and modifying the class attribute value.

F.e., we could add this rule to the global CSS file:

*[hidden="true"] {
  display: none;
}

And then do this to hide and show a node:

// Hide node.
node.setAttribute("hidden", true);

// Show node (either one of these two statements will do the job).
node.setAttribute("hidden", false);
node.removeAttribute("hidden");
Comment 17 Frédéric Buclin 2007-04-06 19:34:43 PDT
Checking in skins/standard/global.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v  <--  global.css
new revision: 1.35; previous revision: 1.34
done
Checking in template/en/default/bug/comments.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/comments.html.tmpl,v  <--  comments.html.tmpl
new revision: 1.30; previous revision: 1.29
done
Comment 18 Max Kanat-Alexander 2008-07-01 00:07:23 PDT
Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.

Note You need to log in before you can comment on or make changes to this bug.