251656 - Redesign dependency tree, part 1: fix the tree itself

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Christian Reis]

I've worked for a while on a redesigned dependency tree. It has graphical
expanders, ctrl-click deep expansion/contraction, and a cleaner, nicer layout.
Patch coming up.

Comment 1

[Christian Reis]

Attachment: kiko_v1: first cut

This is live at 

http://www.async.com.br/~kiko/mybugzilla/showdependencytree.cgi?id=3#b11

Comments appreciated!

Comment 2

[Joel Peshkin]

Comment on attachment 153354 [details] [diff] [review]
kiko_v1: first cut

1) With no javascript, make sure that everything is expanded.  (also, make sure
case where there is javascript but no css is workable)
2) fix indentation in template
3) put some hint about the expand-all and collapse-all mouseclicks

aside from that, I like it...

Comment 3

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

Comment on attachment 153354 [details] [diff] [review]
kiko_v1: first cut

(UI review only)

I like this design.  It's much cleaner and easier to read than the old one. 
The tooltip in particular is an innovative way to hide bug details while
continuing to make them available.

A few notes:

1. The "bug" in "bug nnn" should be all lowercase except in page and section
titles.

2. Closed bugs should have both bug number and summary struck through.

3. I don't see the need for the asterisk noting the duplicate entry for the
bug.  If a bug appears multiple times on a page, and a user cares to know where
else it appears, they probably care about all occurrences, not just the topmost
one, and those can be found by searching the page for the bug number.

Having said that, it might be useful to mark duplicate bugs somehow and provide
some mechanism for users to more identify their dupes.	Perhaps there could be
an icon next to dupes that when clicked would highlight the duplicate bugs on
the page via a background color.

4. Icons next to bug numbers would be useful to provide strong visual guidance
to the hierarchy (i.e. what bullets are used for in lists) and differentiate
between bugs with and without dependencies (ala file explorers with "folder"
and "file" icons).

5. Tooltips would read better as f.e.:

RESOLVED INVALID; assigned to [e-mail]; target ---

6. Although I like the way bug details are hidden in the tooltip, I wonder if
some users rely on their constant visibility for triaging and other purposes. 
Consider some way of showing them on demand, f.e. via a CSS rule that hides
them by default but that can be changed to show them should the user request it
by clicking some button.

Cancelling (not denying) review since I haven't looked at the code and expect
you'll do more work on the UI before getting to code review.

Comment 4

[Christian Reis]

What do you think of styling bugs with dependencies as bold by default?

(In reply to comment #3)
> 1. The "bug" in "bug nnn" should be all lowercase except in page and section
> titles.

Fixed.

> 2. Closed bugs should have both bug number and summary struck through.

I tried this before, and the legibility is just too decreased, I really don't
think it's a good idea when viewing a tree with a lot of resolved bugs. I have
left in a CSS hook ready to customize this if you like:

/* define this if you would like summary text of closed classes to be
 * striked through */
.bz_closed + .summ_text {
}


> 3. I don't see the need for the asterisk noting the duplicate entry for the
> bug.  If a bug appears multiple times on a page, and a user cares to know where
> else it appears, they probably care about all occurrences, not just the topmost
> one, and those can be found by searching the page for the bug number.

So you reckon we should omit it? The original page had some pretty long text
indicating there was a duplicate bug, and our users have requested we keep it
here. /me shrugs -- if you feel strongly enough about this I'll omit it and
customize it locally (but I hate template forks).

> Having said that, it might be useful to mark duplicate bugs somehow and provide
> some mechanism for users to more identify their dupes.	Perhaps there could be
> an icon next to dupes that when clicked would highlight the duplicate bugs on
> the page via a background color.

Ah, are you referring here to actual RESOLVED DUPLICATE? That's an interesting
idea, really. My plan for dependency tree after this is to add some  styling
based on a set of criteria, and then write a few stylesheets that color based on
 this set. That way you could visually group bugs by color with relation to
priority, severity, product, etc. But that's not going to be in this bug :)

> 4. Icons next to bug numbers would be useful to provide strong visual guidance
> to the hierarchy (i.e. what bullets are used for in lists) and differentiate
> between bugs with and without dependencies (ala file explorers with "folder"
> and "file" icons).

This is very easily added with a custom CSS rule:

ul.tree li a.b {
  background: url("bug_item.png") center no-repeat;
}

It's hard to find something that works straight away. Originally I had little
bullets for bugs but user testing showed that it was confusing people into
thinking they could click on them too. I don't know, do you have a suggestion?

> 5. Tooltips would read better as f.e.:
> 
> RESOLVED INVALID; assigned to [e-mail]; target ---

Done.

> 6. Although I like the way bug details are hidden in the tooltip, I wonder if
> some users rely on their constant visibility for triaging and other purposes. 
> Consider some way of showing them on demand, f.e. via a CSS rule that hides
> them by default but that can be changed to show them should the user request it
> by clicking some button.

Originally we had the assignee and target milestone in-line. Jouni asked me for
this too. Problem is I can't seem to find a way to put them in nicely (things
get verbose quick :). I guess I could replicate the information in the link
title after the bug, in square brackets, invisible by default. I don't have an
idea of how to expose UI to actually change this, so we could leave it as a CSS
customization for now and when I redo the rest of the page layout (I'm going to
clean up the headings at least) I'll consider exposing it. Even a bookmarklet
would suffice for advanced users, wouldn't it?

Comment 5

[Christian Reis]

Attachment: kiko_v2: merge comments in

Took into account (hopefully) the comments made. Joel, I haven't found a good
way to put in a legend for the ctrl-click business, but I'm thinking about it,
and it's a minor impact that shouldn't bar review at this point (since I think
you'll be nitpickity with things as they are :-)

Comment 6

[Christian Reis]

Comment on attachment 153414 [details] [diff] [review]
kiko_v2: merge comments in

Note that I merged both tree sections (that had a lot of duplicated code) into
one here too. Smaller files rock (but htmlsize of the generated thing went up
:-/).

Comment 7

[Joel Peshkin]

Comment on attachment 153414 [details] [diff] [review]
kiko_v2: merge comments in

3 things

1) the png files are missing.  Please attach them seperately.  Let's settle the
debate on where NEW images should go.  I dont think we want to clutter the main
directory with them.

2) This still needs a hint that control-click does expand-all or collapse-all.

3) If the person viewing the tree has no access to a blocking bug, don't ignore
it.  Put its number in the tree, but don't look up any further information or
traverse the dependency tree through it.  (same information as when you look at
show_bug)

Comment 8

[Joel Peshkin]

one more thing...  fix the bugwords test exception

Comment 9

[Christian Reis]

1) I updated bug 251727 with a simpler idea and requested approval. No patch
necessary.

2) Any ideas on how to suggest that?

3) It's not easy, but okay.

Comment 10

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

I didn't notice this earlier, but you've unlinked the summaries.  The current
dependency tree code links summaries in addition to bug numbers, and summaries
should be linked in the new code as well.  In addition to providing a larger
click target, making it easier to click an entry, summaries are critical
identifiers of bugs to users, and users are more likely to be looking at a
summary than an ID when they decide to view an entry's bug report, making bug
reports easier to obtain when the summaries are linked.

If you unlinked summaries for readability, use styling instead to make summaries
more readable (even if less obviously links).

Also, after taking a second look I realized that the word "bug" before each ID
takes up valuable horizontal space unnecessarily.  Horizontal space is at a
premium because of the length of many summaries, and since every entry is a bug
that begins with the bug's ID, users don't need them labeled as such.

>> 2. Closed bugs should have both bug number and summary struck through.
>
>I tried this before, and the legibility is just too decreased, I really don't
>think it's a good idea when viewing a tree with a lot of resolved bugs. I have
>left in a CSS hook ready to customize this if you like

This should be the other way around, with the entire entry struck through by
default and a CSS hook for overriding that.  Strikethrough is designed to be
legible even for large bodies of text (a common use outside Bugzilla is to show
obsolete portions of legal documents under heavy revision), and whole dependency
entries have been struck through in Bugzilla for a long time without complaint.

Entries in the dependency tree are identified by both ID and summary, and users
are more apt to be browsing down through summaries than ID, which makes
identifying closed bugs without summary strikethrough more difficult, since they
have to look left then back right again, breaking the previously single
direction motion of their eyes.

Note that because you've bolded bug IDs in the new version, the strike through
becomes two-pixels thick on my screen instead of one pixel, which makes the text
less legible than if the numbers weren't bolded (since bolding doesn't make the
text twice as high, but it does make the strikethrough twice as thick).  It
would be better to remove the bold style, especially since bug numbers aren't
the most important data in the entries.

>So you reckon we should omit it? The original page had some pretty long text
>indicating there was a duplicate bug, and our users have requested we keep it
>here. /me shrugs -- if you feel strongly enough about this I'll omit it and
>customize it locally (but I hate template forks).

We shouldn't use an asterisk and link only to the first occurrence of the dupe,
but that doesn't mean we should omit it.  A better approach would be to add an
icon to duplicate entries that highlights all occurrences of the dupe when
clicked.  This should be doable with JS code that sets a style on all li with
class bug-nnn, where "nnn" is the bug ID.

The icon should be a toggle, so clicking it again (next to any of the duplicate
entries) would turn it off again.  You could use background colors to highlight,
although that might conflict with our current use of a background color for
secure bugs.  If background colors and background images can be used
concurrently, however, then the other option is to use a semi-transparent
background image for one of these sets and a color for the other.

>Ah, are you referring here to actual RESOLVED DUPLICATE?

No, I was referring to multiple occurrences of the same bug in the list.

>It's hard to find something that works straight away. Originally I had little
>bullets for bugs but user testing showed that it was confusing people into
>thinking they could click on them too. I don't know, do you have a suggestion?

If you can't link the bullets in a list (which I don't think you can), make them
part of the content of the page and stick them inside the <a> tag that surrounds
the bug IDs.  They should be part of the link, as your user testing has shown.

>Originally we had the assignee and target milestone in-line. Jouni asked me for
>this too. Problem is I can't seem to find a way to put them in nicely (things
>get verbose quick

A button next to "hide resolved" that says something like "show details" and
sets the necessary CSS style (or regenerates the page if that makes more sense
given the amount of content) should work.

>1) the png files are missing.  Please attach them seperately.  Let's settle
>the debate on where NEW images should go.  I dont think we want to clutter
>the main directory with them.

An images/ directory makes sense to me.  I've said so in bug 251727, but I held
off on approval because I'd like Dave to chime in as well.

Note that the image names should be tree-collapse.png and tree-expand.png
instead of tree_minus.png and tree_plus.png.  The hypens are consistent with
current Bugzilla file naming conventions, and the words collapse and expand are
more amenable to future changes to other idioms to reflect the two tree states
(for example a triangle icon that changes direction).

>2) This still needs a hint that control-click does expand-all or collapse-all.

This isn't a blocker to review for me, but some possibilities for this are to
change the cursor when the user holds down the control key while hovering over
the twisty or to pop up a tooltip with usage hints (for both types of clicks) if
the user hovers over the twisty for a certain amount of time.

>3) If the person viewing the tree has no access to a blocking bug, don't
>ignore it.

Good point, especially given that the numbers show up in show_bug.cgi, but if we
currently ignore it, then this is a separate bug.

Comment 11

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

Comment on attachment 153414 [details] [diff] [review]
kiko_v2: merge comments in

Removing review flag since I've now reviewed UI but again not code.

Comment 12

[Christian Reis]

Wrt readability of striked text: just for the record, back in bug 40286 it was
already argued that striking out the whole summary decreases readability.

Comment 13

[Christian Reis]

Attachment: tree_open.png

Comment 14

[Christian Reis]

Attachment: tree_closed.png

Comment 15

[Christian Reis]

(In reply to comment #10)
> If you unlinked summaries for readability, use styling instead to make summaries
> more readable (even if less obviously links).

Okay.

> but that doesn't mean we should omit it.  A better approach would be to add an
> icon to duplicate entries that highlights all occurrences of the dupe when
> clicked.  This should be doable with JS code that sets a style on all li with
> class bug-nnn, where "nnn" is the bug ID.

I consider this a separate bug, or this one will become too unwieldy. You're
already getting a major improvement, help me finish this one so we can get more :-)

> Note that the image names should be tree-collapse.png and tree-expand.png
> instead of tree_minus.png and tree_plus.png.  The hypens are consistent with

I picked tree_open.png and tree_closed.png to reflect the actual status (and not
verbs). Attached above.

> >2) This still needs a hint that control-click does expand-all or collapse-all.
> 
> This isn't a blocker to review for me, but some possibilities for this are to
> change the cursor when the user holds down the control key while hovering over
> the twisty or to pop up a tooltip with usage hints (for both types of clicks) if
> the user hovers over the twisty for a certain amount of time.

A tooltip takes up a lot of html and isn't really discoverable; I'll add a
legend as a short-term compromise. I have more extensive plans for this page (if
I ever manage to get this reviewed).

> Good point, especially given that the numbers show up in show_bug.cgi, but if we
> currently ignore it, then this is a separate bug.

We currently ignore them.

I have an updated patch to attach; it addresses most comments here.

Comment 16

[Christian Reis]

(In reply to comment #15)
> (In reply to comment #10)
> > If you unlinked summaries for readability, use styling instead to make summaries
> > more readable (even if less obviously links).
> 
> Okay.

I tried, but I don't know exactly how to. I used something like:

a my_style {
  text-decoration: none;
}

and it didn't work, presumably because the style is being set in the a. Does
this mean I need to split this into two links? I *so* don't think this is worth
it if so.

Comment 17

[Christian Reis]

Attachment: kiko_v3: separate js and css, add hint text, runtests ok and html validates

Comment 18

[Christian Reis]

Attachment: kiko_v4: make style work

I was missing a style_urls statement and the images were referred to
incorrectly. Fixed.

Comment 19

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

Comment on attachment 157462 [details] [diff] [review]
kiko_v4: make style work

This is looking better from a UI perspective, but it still needs some work...

>Index: js/expandingtree.js
>Index: css/dependency_tree.css
>tree_open.png
>tree_closed.png

Use hyphens between multiple words in filenames, i.e. expanding-tree.js,
dependency-tree.css, tree-open.png, tree-closed.png.


>>If you unlinked summaries for readability, use styling instead to make summaries
>>more readable (even if less obviously links).
>
>I tried, but I don't know exactly how to.

Here's one option:

li a {
  text-decoration: none;
}


>I consider this a separate bug, or this one will become too unwieldy. You're
>already getting a major improvement, help me finish this one so we can get more :-)

Agreed.


>A tooltip takes up a lot of html

Since twisties are links, tooltips for them are just title attributes on the a
tags, which is very little extra HTML.

>and isn't really discoverable; I'll add a legend as a short-term compromise.

Tooltips are designed for just-in-time discoverability, since they pop up when
users hesitate on a UI element; and while the legend shows up for all users,
telling them what they already know most of the time, tooltips show up only
when needed.  They're also more contextual, being tied to the UI element they
describe, and they're just as discoverable as the tooltips into which we're
placing bug details.  We should be using tooltips rather than a legend.

Changes from the way it currently works that should be reverted:

* prefixing "bug" to bug IDs (takes up precious horizontal screen space, and
unnecessary label for what will always be a tree of bug items);
* reducing strike-through to just bug IDs (makes it harder to identify closed
bugs when browsing summaries);
* bolding bug IDs (makes strike-through too big);
* making the bug whose tree is being displayed the topmost item in the tree
(redundant with section headings, and takes up precious horizontal screen
space).

Comment 20

[Jouni Heikniemi]

Comment on attachment 157462 [details] [diff] [review]
kiko_v4: make style work

I agree with Myk's comments with perhaps the exception of the last one. I think
a root node for those visual trees is good for clarity. You could reduce the
indentation if you need more horizontal space, though.

>Index: js/expandingtree.js
>===================================================================
>+    /* XXX we should in fact check our *computed* style, not the display
>+     * attribute of the current node, which may be inherited and not set */

Make a bug out of this instead of a vague XXX comment.

JS reviewed only superficially, but it works on both IE and Firefox.


>Index: css/dependency_tree.css
>===================================================================

>+ul.tree ul {
>+    /* lists inside the tree are by default invisible */
>+    padding-top: 3px;
>+    display: block;
>+}

How does that comment relate to "display: block" below?

>+.summ_info {
>+    display: none; /* change to inline if you miss the pollution */
>+    font-size: 75%;
>+}

What's this? I think we shouldn't bloat HTML by dumping information we're going
to hide in the CSS anyway. Anyway, that comment is confusing. Don't make jokes,
explain what you're talking about. 


>Index: template/en/default/bug/dependency-tree.html.tmpl
>===================================================================

>+<script type="text/javascript" language="JavaScript">
>+<!-- 
>+// change style to default display to none
>+// and update the default icon to the plus
>+// (except for top-level bug)
>+//-->
>+</script>

Explain. 


I'll say this right before I go into the template details: Your code
readability blows. There are several sections where I think the template code
could be much more readable. Like we discussed elsewhere, 80 chars isn't god
and some wrapping issues can just be ignored, but some of this is trivial to
fix and will affect readability _very_ significantly. So here's some nitpicking
augmented by the rage that I have to rewrite these comments since I
accidentally reloaded the edit attachment page once ;-)

>+<div class="hint">
>+<h3>Usage Hints</h3>
>+<ul>
>+<li>Click on expander icons to expand or contract a portion of the tree.
>+<li>Control-clicking on expander icons performs a "deep" change:<br>
>+    all children [% terms.bugs %] are also expanded or contracted accordingly.
>+</ul>
>+</div>

Indent h3's and ul's contents; for me it's fine to leave the div as is.

This block is way too left on IE; the bullets get clipped and it looks pretty
lame :-(

> [%###########################################################################%]
>-[%# Block to display a tree                                                 #%]
>+[%# Blocks to display a tree                                                 #%]
> [%###########################################################################%]

Fix the right end of that comment line. Also, "Tree drawing blocks" would
probably be more understandable...

You might want to consider documenting the interfaces with small IF comments
just for clarity. You're explaining some of the interface anyway, so why not do
it properly. Hmm... It's almost like payback from the time when I rewrote
dependency graphs and got a needs-work from you because you wanted better
comments. Figures ;-)

>+  [%# To simplify the interface, I chose to supply tree as a string and
>+    # use it both as a tree id and as the variable name of the tree  %]

Please don't say "I" (who I?) in the code, "To simplify the interface, the tree
is supplied as a string". But by all means, explain what it means to pass a
tree as a string.

>+  [%# Display the tree of bugs that this bug depends on. %]
>+    <h3>
>+    [% IF hide_resolved %]

Wrong indentation. Pull the <h3> and </h3> back by two spaces.

>+  [% IF ids.size > 0 %]
>+    ([% IF maxdepth -%]Up to [% maxdepth %] level[% "s" IF maxdepth > 1 %] deep | [% END -%]<a href="buglist.cgi?bug_id=[% ids.join(",") %]">view as [% terms.bug %] list</a>

You have no good reason to exceed 80 chars here, so don't do it. For example,
you could trivially wrap before the word deep. Remember to indent accordingly
with the IF block.

>+    [% IF canedit && ids.size > 1 %]
>+    | <a href="buglist.cgi?bug_id=[% ids.join(",") %]&amp;tweak=1">change several</a>
>+    [% END %])

Indentation & wrap - yeah yeah, it not much over 80, but there's no good reason
for exceeding it...

>+    <ul class="tree">
>+    [% INCLUDE display_tree tree=$tree bugid=bugid %]
>+    </ul>

Indentation...

>+    <b>Total [% terms.bugs %] displayed: [% dependson_ids.size %]</b>

It escapes me why you're doing pretty excessive CSS styling and then using <b>
instead of a <strong> - or even just a properly classed div - here. 

Repeat this comment for several <b>s around the template.

>+  [%# This code was meant to keep a *count* of how many times a certain
>+    # bug had been displayed; it turns out that fetching keys to increment
>+    # is *very* slow in TT and causes very large trees to never render.
>+    # I ended up using it as a marker only %]

"I" syndrome.

If you need to explain the past, explain it in the bug. Code readers need
concise information about the current code, not the development history. :-)

>+      <b>(<a title="Already displayed above; click to locate" href="#b[% bugid %]">*</a>)</b>

Wrap (before the href is a trivial place).

>+[% BLOCK bullet %]
>+<a name="b[% bugid %]" class="b [% IF bug.dependencies.size > 0 && !  global.seen.$bugid %]b_open " href="#" onclick="return doToggle(this, event) [% END %]">&nbsp;</a>
>+[% END %]

Get these monsters wrapped and indented properly, it's really unnecessary to do
this. Repeat for next couple of blocks.

Could you make that tag construction a bit less hacky? Reusing the quotes like
that is... well, hacky. Unreadable. Whatever.

How about using ids instead of names?

>+[% BLOCK buginfo %]
>+[% bug.status FILTER html %] [%+ bug.resolution FILTER html %]; assigned to [% bug.assignee_email FILTER html %]; [% "Target:" _ bug.milestone IF bug.milestone %]
> [% END %]

Space after 'Target:'

Comment 21

[Christian Reis]

I love the positive tone of the reviews on this bug, it reminds me of how fun it
is to work with Open Source Software.

Comment 22

[Christian Reis]

(In reply to comment #19)
> Here's one option:
> 
> li a {
>   text-decoration: none;
> }

This makes the whole text unlinked instead of the bug number. At this point I
believe it's best to just link the bug number.

> Tooltips are designed for just-in-time discoverability, since they pop up when
> users hesitate on a UI element; and while the legend shows up for all users,
> telling them what they already know most of the time, tooltips show up only
> when needed.  They're also more contextual, being tied to the UI element they
> describe, and they're just as discoverable as the tooltips into which we're
> placing bug details.  We should be using tooltips rather than a legend.

Tooltips require the user to scrub the interface to understand how it works.
I'll file a bug for this if it proves to be a problem later, I am certain the
legend is the right way to go here.

I have chosen not to revert the changes below. I've been avoiding this
discussion, and I hate to sound so assertive, but I have performed *real* user
testing with this page, and I am certain of my conclusions at this point. I am
certain that:

> * prefixing "bug" to bug IDs (takes up precious horizontal screen space, and
> unnecessary label for what will always be a tree of bug items);

Is a good idea, because the bug numbers alone in sites with few bugs offer very
little clickable area, and because they provide a visual cue that allows users
to easily find the point. Removing them made the two test users slower in
locating a certain bug.

> * reducing strike-through to just bug IDs (makes it harder to identify closed
> bugs when browsing summaries);

Is a good idea. Full strike-through made the list *much* harder to read and both
the test users scored worse in locating a bug in the dependency list. Full
strike-through *with* summary linking is almost torture and times double for
selected buglists. 

> * bolding bug IDs (makes strike-through too big);

It doesn't change strike-through significantly in any browser that I have tested
 -- it's a single pixel high in FF 0.9.

> * making the bug whose tree is being displayed the topmost item in the tree
> (redundant with section headings, and takes up precious horizontal screen
> space).

I've reduced the indentation slightly, and we've now regained more than the
space allocated to this initial bug. It's a good way to fold the whole
dependency tree, and makes it clear that the bugs are indeed dependent/blocking
the top bug. It also allows for custom styling (for instance, on very large
trees we have custom JS written which allows only the parent bug to be unfolded
and let the rest be dynamically unfolded by the user). 

I ask you to please bear with these changes and discuss reverting them after a
while of getting used to them, I am very hopeful you will find them agreeable. I
have good experience with UI design and testing (In my MSc I studied one full
year of UI design which included paper prototyping, user testing, interaction
modeling, ) and I am aware of the tradeoffs I chose for this design. It is a
*large* improvement over what we have today. We haven't been nitpicky over this
sort of change on this page before -- certainly not when Shaver added text-based
widgets, for one. 

Please concede to the effort I've put in here to unclutter the page and make it
*much* better to use. 

I'll have your other comments, and Jouni's comments addressed and fixed shortly.

Comment 23

[Christian Reis]

(In reply to comment #20)
> >+.summ_info {
> >+    display: none; /* change to inline if you miss the pollution */
> >+    font-size: 75%;
> >+}
> 
> What's this? I think we shouldn't bloat HTML by dumping information we're going
> to hide in the CSS anyway. Anyway, that comment is confusing. Don't make jokes,
> explain what you're talking about. 

The idea is to, in a future bug, allow a JS control that will display this
selectively. I am happy to do this later.

> I'll say this right before I go into the template details: Your code
> readability blows. 

Thanks. I would appreciate more constructive comments, but I'll try my best to
make it easier to read.

> >+    <ul class="tree">
> >+    [% INCLUDE display_tree tree=$tree bugid=bugid %]
> >+    </ul>
> 
> Indentation...

No idea what I should be indenting here.

Comment 24

[Jouni Heikniemi]

(In reply to comment #23)
> Thanks. I would appreciate more constructive comments, but I'll try my best to
> make it easier to read.

Well, I already apologized on IRC. The comments turned out pretty harsh, but I
already rewrote them once (because of the accidental reload) so I didn't want to
waste time by going through them yet again since I was going to bed. Sorry
again, anyway. :-) 

Even though I didn't elaborate on each of the points I marked as "wrap" or
"indent", I believe most of that stuff can be cleaned up simply by following the
indentation and wrapping rules used everywhere in Bugzilla.

> > >+    <ul class="tree">
> > >+    [% INCLUDE display_tree tree=$tree bugid=bugid %]
> > >+    </ul>
> > Indentation...
> No idea what I should be indenting here.

The content of the <ul> - just like you'd say 

<ul>
  <li>foo</li>
</ul>

instead of

<ul>
<li>foo</li>
</ul>

Or did I misunderstand something?

Comment 25

[Christian Reis]

(In reply to comment #24)
> Even though I didn't elaborate on each of the points I marked as "wrap" or
> "indent", I believe most of that stuff can be cleaned up simply by following the
> indentation and wrapping rules used everywhere in Bugzilla.

Well, wrapping and indentation:

  - is to me a great mystery when it comes to HTML. I am a strict code-wrapper.
Dave seems to concur with me here. 
  - is something that honestly can't be *that* important given the above. I
agree that wrapping and indenting correctly when possible is a good thing, but
TT code is by definition harder read because it fundamentally mixes two
different code syntaxes in one block. And they both have differing indentation! 

So please relax on this sort of complaint, it really should not be critical. If
you want to point them out as nits that don't block review, I'm happy.

> > > >+    <ul class="tree">
> > > >+    [% INCLUDE display_tree tree=$tree bugid=bugid %]
> > > >+    </ul>
> > > Indentation...
> > No idea what I should be indenting here.
> 
> The content of the <ul> - just like you'd say 
[...]
> Or did I misunderstand something?

Perhaps. The *real* child of the UL there is defined inside the display_tree
block, and *there* it would need to be indented if we want the HTML *generated*
to come out indented correctly. From what I've seen of our TT code most of the
time the TT code is indented relative to itself, and the HTML code relative to
itself (so the HTML generated comes out nicely indented). These goals work
against each other, unfortunately. 

Do you expect TT blocks to be indented relative to the HTML around them?

Comment 26

[Jouni Heikniemi]

(In reply to comment #25)
>   - is to me a great mystery when it comes to HTML. I am a strict code-wrapper.

I hadn't read your developers@ mail when I last replied to the bug; now that I
have, I think we can go through HTML mechanics there. I'm very much ready to
give whatever I can to the process instead of just r-ing patches. If we reach a
new consensus, I'm naturally ready to go with it. 

>   - is something that honestly can't be *that* important given the above. I
> agree that wrapping and indenting correctly when possible is a good thing, but
> TT code is by definition harder read because it fundamentally mixes two
> different code syntaxes in one block. And they both have differing 
> indentation! 

Indentation and wrapping are nothing by themselves. Code clarity is the stuff
that matters, and I see absolutely no reason to exclude either TT directives OR
HTML. I do agree that the mixing of two languages and the tag structures in
particular make templates harder. Then again, we have the same issues in a
different form with Perl and embedded SQL. People are just so used to it that
they've stopped caring.

FWIW, I've been working with ASP/PHP style languages for 8 years now and managed
to stick to the 80 char rule fairly well. Even though the mixture is a challenge
at times, it is not impossible. If it is a mystery to you or some other
developers, it's very good that you posted to developers. If we can hash out
some ideas and develop better techniques (and learn in the process), all the better.

> So please relax on this sort of complaint, it really should not be 
> critical. If you want to point them out as nits that don't block 
> review, I'm happy.

80+ char lines by themselves block no review. However, if reading the code
requires extra time just because it's formatted badly, that is just as a good a
reason for a r- as is any other hard-to-read code. We must pay attention to this
sort of stuff as well, otherwise our codebase will deteriorate. The code should
set an good example for new possible contributors around. <cut babble>

Of course, we're now talking about what should that example look like in
practice. I'm still happy you took the issue to the mailing list. O:-)

> Perhaps. The *real* child of the UL there is defined inside the display_tree
> block, and *there* it would need to be indented if we want the HTML 
> *generated* to come out indented correctly. 

I don't think we care about generated output. I'm almost certain we did discuss
this through when starting templatization (and ended up thinking that pre_chomp
et al will waste output readability anyway, so why bother), but can't find it...
Could be in reviewers@ archive which I can't access easily. 

Gerv's indentation example in
<http://groups.google.com/groups?selm=3BDD9D2C.20701%40mozilla.org> is the
closest to a complete example of "correct" template indentation. There the rule
 is something like "TT control structures don't get indented, but output
statements do". That example is also in the Bugzilla FAQ. Practically we've also
been indenting control structures as well, and I think it's a good idea to do so.

> From what I've seen of our TT code most of the
> time the TT code is indented relative to itself, and the HTML code relative to
> itself (so the HTML generated comes out nicely indented). These goals work
> against each other, unfortunately. 

We've had all sorts of styles along the time. There are some really nasty Perl
files too. Nowhere have I seen a systematic separation of indentation for TT
directives and HTML elements, although the idea of keeping TT directives at
column 0 and indenting HTML along the way is visible in some very rare templates...

If you want an example of a template well written IMO, I find bug/edit.html.tmpl
 to be very readable even though it's insanely complex. It's not completely
uniform especially re the control structure rule above, but it's fairly good.

> Do you expect TT blocks to be indented relative to the HTML around them?

I probably already answered this above, but mostly yes. When it causes total
chaos, then no. O:-)

Comment 27

[Christian Reis]

Attachment: kiko_v5: hyphenize names, indent nicely (hopefully)

There are a couple of long lines in the HTML, and I couldn't find a way around
them in the time I dedicated to that. The file names are hyphenized now, and I
added interface comments for two of the main blocks. It's not an easy patch to
read, but it works out nicely.

Comment 28

[Christian Reis]

Comment on attachment 157834 [details] [diff] [review]
kiko_v5: hyphenize names, indent nicely (hopefully)

The updated summary indicates that yes, there is more to come after this. I
don't plan on leaving the legend where it is, and the page needs to look nice
and consistent. We need to start somewhere though.

Comment 29

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

Comment on attachment 157834 [details] [diff] [review]
kiko_v5: hyphenize names, indent nicely (hopefully)

> This makes the whole text unlinked instead of the bug number. At this point I
> believe it's best to just link the bug number.

That's correct, it removes the underline from the whole item, which removes the
distraction of the underline while enabling much larger and more contextual
clickable area at no extra cost.


> Tooltips require the user to scrub the interface to understand how it works.
> I'll file a bug for this if it proves to be a problem later, I am certain the
> legend is the right way to go here.

Tooltips do require the user to navigate the interface, but they do so in a
standard, contextual, and well-understood way.	We use them elsewhere to good
effect.  In fact, this patch itself implements them to solve another problem. 
I think they're the right solution here, too.


> bug numbers alone in sites with few bugs offer very
> little clickable area, and because they provide a visual cue that allows users
> to easily find the point. Removing them made the two test users slower in
> locating a certain bug.

The solution to the clickable area problems is to linkify the entire item.


> Full strike-through made the list *much* harder to read and both
> the test users scored worse in locating a bug in the dependency list. Full
> strike-through *with* summary linking is almost torture and times double for
> selected buglists. 

Locating a bug isn't the only (or even necessarily the most common) use of the
dependency list, and clearly distinguishing between open and closed bugs in the
list, even at the cost of making the struck-through bugs somewhat harder to
read, is an important design objective.  As for the problem of strike-through
with summary linking, there is no issue once the underlining of items is
removed.


> > * bolding bug IDs (makes strike-through too big);
> 
> It doesn't change strike-through significantly in any browser that I have 
> tested -- it's a single pixel high in FF 0.9.

In Firefox 0.9.x on my laptop the strike-through is two pixels thick for bolded
text.


> I've reduced the indentation slightly, and we've now regained more than the
> space allocated to this initial bug. It's a good way to fold the whole
> dependency tree, and makes it clear that the bugs are indeed 
> dependent/blocking the top bug. It also allows for custom styling (for 
> instance, on very large trees we have custom JS written which allows only
> the parent bug to be unfolded and let the rest be dynamically unfolded by
> the user). 

This makes sense, and reducing the indentation helps mitigate this problem
somewhat.  I'm OK with this change now.


> I ask you to please bear with these changes and discuss reverting them after a
> while of getting used to them, I am very hopeful you will find them agreeable.

It's not that I'm unsure about these changes or find them questionable.  I
think they're the wrong way to go, and the revertability safety net is not a
good reason to go the wrong way.


> I have good experience with UI design and testing (In my MSc I studied one
> full year of UI design which included paper prototyping, user testing, 
> interaction modeling, ) and I am aware of the tradeoffs I chose for this 
> design.

Your training is not in question, but neither does it confer additional
authority.  The point is not who is able to create UI but what UI is best to
create.  It's the implementation we're judging here, not the implementer.


> It is a *large* improvement over what we have today. We haven't been nitpicky
> over this sort of change on this page before -- certainly not when Shaver 
> added text-based widgets, for one.

My issues aren't nits--they address important functional aspects of the page. 
And any past lapses in attention to UI changes don't mean we should continue to
suffer similar lapses.	Also, unlike previous efforts, this attempt is a major
redesign with numerous changes and deserves additional scrutiny in proportion
to its significance.


> Please concede to the effort I've put in here to unclutter the page and make
> it *much* better to use.

I understand and appreciate the effort you've put into this patch, but no
amount of effort should make a change for the worse more acceptable, and I
think some of the changes in this patch are for the worse.

I'm happy to accept the beneficial changes in this patch into the tree,
decoupled from the changes with which I disagree.  As for the rest, if you
still want to discuss them, please file separate bugs for each change and
continue the discussion there.

Comment 30

[Christian Reis]

Committing to UI branch for further refinements there. Some commenting:

(In reply to comment #29)
> That's correct, it removes the underline from the whole item, which removes the
> distraction of the underline while enabling much larger and more contextual
> clickable area at no extra cost.

Normal page-content links *should* be underlined; removing the underline reduces
usability. Hundreds of references, easiest one is
http://www.useit.com/alertbox/20040510.html

> > > * bolding bug IDs (makes strike-through too big);
> > 
> > It doesn't change strike-through significantly in any browser that I have 
> > tested -- it's a single pixel high in FF 0.9.
> 
> In Firefox 0.9.x on my laptop the strike-through is two pixels thick for bolded
> text.

I see this now on a xft/gtk2 build. I'll look into this; I realize it is indeed
a serious issue that needs fixing.

The rest of the issues I'll tackle incrementally in the branch. When it's done,
we can decide whether they turned out for the better or not.

Comment 31

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

> Normal page-content links *should* be underlined; removing the underline 
> reduces usability.

These aren't "normal page-content links," they're items in a tree, every one of
which links to the object it represents, like the folder and message header
panes in Thunderbird, and completely unlike a page of web content.

> The rest of the issues I'll tackle incrementally in the branch. When it's 
> done, we can decide whether they turned out for the better or not.

I strongly suggest you land the approved changes on the trunk and revert the
rejected changes on the branch.  The rejected changes are very unlikely to ever
make it to the trunk, and landing both approved and rejected changes on the
branch without landing the approved ones on the trunk makes future integration
of the approved changes harder and thus less likely, which is bad for Bugzilla.

The branch should be a place to work out changes, not a place to land rejected
ones.  You've done some great work in this bug, and it deserves to be landed for
the betterment of Bugzilla.  I encourage you to do so.

Comment 32

[Christian Reis]

Attachment: screenshots comparing approaches

Just to illustrate the differences we're discussing.

Comment 33

[André Batosti]

Attachment: tree-open.png

Comment 34

[André Batosti]

Attachment: tree-closed.png

Comment 35

[André Batosti]

Attachment: bug-item.png

Comment 36

[André Batosti]

Attachment: batosti_v6

Comment 37

[Frédéric Buclin]

*** Bug 316871 has been marked as a duplicate of this bug. ***

Comment 38

[Frédéric Buclin]

Comment on attachment 203382 [details] [diff] [review]
batosti_v6

not ok 142 - (en/default) template/en/default/bug/dependency-tree.html.tmpl has unfiltered directives:
#   131:+ extra_class
#   131: extra_args
# --ERROR
#     Failed test (t/008filter.t at line 131)

Before asking myk to review a patch, make sure it passes runtests.pl successfully.

Comment 39

[André Batosti]

Attachment: batosti_v6: no runtests error

sorry by the filter error.

Comment 40

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

Comment on attachment 203440 [details] [diff] [review]
batosti_v6: no runtests error

This looks very good.  The only issue I see is that the "Total bugs displayed:" messages take up a significant amount of real estate.  These should be folded into the list headers so that instead of reading "Bugs that bug xxx depends on|blocks" they read "xxx Bugs that Bug yyy Depends On|Blocks".


>Index: css/dependency-tree.css

This should move to skins/standard/dependency-tree.css, and the images should move to skins/standard/dependency-tree/.


>+ul.tree li { 
>+    /* see http://www.kryogenix.org/code/browser/aqlists/ for idea */
>+    padding-top: 3px; 
>+    text-indent: -1.2em; 
>+    padding-left: 0.5em; 
>+    list-style-type: none;
>+    padding-bottom: 3px;

Nit: it'd be nice for all the padding rules to appear consecutively.


>+/* uncomment this if you would like summary text of closed classes to be
>+ * striked through */
>+/*
>+.bz_closed + .summ_text {
>+    text-decoration: line-through;
>+}
>+*/

Nit: striked through -> struck through

Also, what are "closed classes"?  Do you perhaps mean "closed bugs"?  And isn't the summary text of closed bugs already struck through?


>+/* for styling summaries that are leaves or not */

I'm afraid I don't understand this comment.  Can you elaborate?


>RCS file: js/expanding-tree.js

>+if (!Node) {
>+    /* MSIE doesn't define Node, so provide a compatibility object */

Nit: one-line and otherwise short comments here and elsewhere should use // instead of /* */.


>+function doToggle(node, event) 
>+{

Placement of the opening bracket in function declarations should be consistent, and it should follow Bugzilla Perl style, which is to place it on the same line as the function keyword, name, and argument list, i.e.:

function doToggle(node, event) {


>+    if (toggle) {
>+        if (deep) {
>+          var direction = toggleDisplay(toggle, node);
>+          changeChildren(toggle, direction);
>+        } else {
>+          toggleDisplay(toggle, node);
>+        }
>+    }

Indentation should be consistent, so the two-space indents here should become four-space indents.


>+        while (child && child.tagName != "A")
>+           child = child.nextSibling;

Three -> four space indent.


>+        while (sublist && sublist.tagName != "UL")
>+           sublist = sublist.nextSibling;

Three -> four space indent.


>+function duplicated(element)
>+{
>+    var allsumm= document.getElementsByTagName("span");
>+    if (highlighted) {
>+        for(i = 0;i < allsumm.length; i++) {

"for(" -> "for (" per style of "if" and "while" statements in this file and "for" statements in Bugzilla Perl code.


>Index: template/en/default/bug/dependency-tree.html.tmpl

>+[% INCLUDE tree_section ids=dependson_ids bugid=bugid 
>+                        tree_name="dependson_tree" text="depends on" %]

Nit: the INCLUDE directive makes defined variables available to the block being included, so there's no need for "bugid=bugid".

Nit: instead of passing both tree_name and text variables, it would be simpler to just pass a single "type" variable and have the block generate the correct tree name and text given the type.


>+<div class="hint">
>+  <h3>Usage Hints</h3>
>+  <ul>
>+    <li>Click on expander icons to expand or contract a portion of the tree.
>+    <li>Control-clicking on expander icons performs a "deep" change:<br>
>+        all children [% terms.bugs %] are also expanded or contracted accordingly.
>+  </ul>
>+</div>

Nit: this should really be contextual help, f.e. appearing in a tooltip that pops up when you hover over expander icons.

Comment 41

[André Batosti]

(In reply to comment #40)
> >+/* uncomment this if you would like summary text of closed classes to be
> >+ * striked through */
> >+/*
> >+.bz_closed + .summ_text {
> >+    text-decoration: line-through;
> >+}
> >+*/
> 
> Nit: striked through -> struck through
> 
> Also, what are "closed classes"?  Do you perhaps mean "closed bugs"?  And isn't
> the summary text of closed bugs already struck through?
> 
This belonged to the kiko's old code and come out of the patche.
> 
> >+/* for styling summaries that are leaves or not */
> 
> I'm afraid I don't understand this comment.  Can you elaborate?
> 
this comment is old too, in the other patch the css classes are empty for a future use and now it's used. this comment do not make sense now. i will remove from patch

Comment 42

[André Batosti]

Attachment: batosti_v7

Comment 43

[André Batosti]

Attachment: batosti_v7: fixing problem in ie

fixing a problem with bug links in ie, and the tip when the mouse is over the bug.

Comment 44

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

Comment on attachment 204132 [details] [diff] [review]
batosti_v7: fixing problem in ie

>+    <b>[% ids.size %] [%+ terms.Bugs %] that [% terms.bug %] [%+ bugid %] 
>+    [% IF type == 1 %]
>+        depends on
>+    [% ELSIF type == 2 %]
>+        blocks
>+    [% END %]</b>

This information should only appear in the header of each tree.  It's redundant as a footnote. i.e.:

15 Bugs that Bug 1521 Depends On

* foo
  * bar
  * baz
...

13 Bugs that Bug 1521 Blocks

* buz
  * biz
  * bez
...


>+     title="Click to expand or contract this portion of the tree. Hold Ctrl make a deep change."

Hold Ctrl make a deep change -> Hold down the Ctrl key while clicking to expand all subtrees.

Otherwise it looks great!

Comment 45

[André Batosti]

Attachment: batosti_v7_another_fix

Comment 46

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

This is a little too big to take for 2.22, but it can be checked in as soon as the tree opens for 2.24 checkins.

Comment 47

[André Batosti]

Attachment: batosti_v8

Comment 48

[Frédéric Buclin]

Comment on attachment 212577 [details] [diff] [review]
batosti_v8

>Index: template/en/default/bug/dependency-tree.html.tmpl

> [% BLOCK depthControlToolbar %]
>- <table cellpadding="3" border="0" cellspacing="0" bgcolor="#d0d0d0">
>- <tr>

I see no reason to remove these two lines.

Comment 49

[André Batosti]

Attachment: batosti_v8_fix

Comment 50

[Frédéric Buclin]

Comment on attachment 212773 [details] [diff] [review]
batosti_v8_fix

the patch *looks* identical to the one myk reviewed...

Comment 51

[Frédéric Buclin]

Checking in showdependencytree.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencytree.cgi,v  <--  showdependencytree.cgi
new revision: 1.42; previous revision: 1.41
done
Checking in js/expanding-tree.js;
/cvsroot/mozilla/webtools/bugzilla/js/expanding-tree.js,v  <--  expanding-tree.js
new revision: 1.2; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/skins/standard/dependency-tree.css,v
done
Checking in skins/standard/dependency-tree.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/dependency-tree.css,v  <--  dependency-tree.css
initial revision: 1.1
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.63; previous revision: 1.62
done
Checking in template/en/default/bug/dependency-tree.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/dependency-tree.html.tmpl,v  <--  dependency-tree.html.tmpl
new revision: 1.19; previous revision: 1.18
done

Comment 52

[Douglas Calvert]

Maybe I am doing something wrong but I do not see the images needed for:
ul.tree li a.b_open {
    background: url("dependency-tree/tree-open.png") center no-repeat;
    cursor: pointer;
}

ul.tree li a.b_closed {
    background: url("dependency-tree/tree-closed.png") center no-repeat;
    cursor: pointer;

in the cvs tree. 
I think this may need a reopen.

Comment 53

[Frédéric Buclin]

Bah... I didn't notice I had to commit images too... Here we go.

RCS file: /cvsroot/mozilla/webtools/bugzilla/skins/standard/dependency-tree/bug-item.png,v
done
Checking in skins/standard/dependency-tree/bug-item.png;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/dependency-tree/bug-item.png,v  <--  bug-item.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/skins/standard/dependency-tree/tree-closed.png,v
done
Checking in skins/standard/dependency-tree/tree-closed.png;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/dependency-tree/tree-closed.png,v  <--  tree-closed.png
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/skins/standard/dependency-tree/tree-open.png,v
done
Checking in skins/standard/dependency-tree/tree-open.png;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/dependency-tree/tree-open.png,v  <--  tree-open.png
initial revision: 1.1
done

Comment 54

[Frédéric Buclin]

*** Bug 364502 has been marked as a duplicate of this bug. ***