236424 - Allow showdependencies trees to collapse

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Vlad Dascalu]

Allow showdependencies trees to collapse.

Comment 1

[Vlad Dascalu]

Attachment: Shaver's patch

Comment 2

[Christian Reis]

Comment on attachment 142891 [details] [diff] [review]
Shaver's patch


Hey Mike, cool patch! Some questions and comments about it follow:

>Index: dependency-tree.html.tmpl
>+      [% "<span class='toggle' onclick='listToggle(event)'>[-]</span>"
>+         IF dep.dependencies.size > 0 && !dep.seen %]

Since this is only supported by browsers that implement Javascript, it's
probably a good idea to only display the control in these cases. I've used
document.write() in the past do achieve this, though you may have a suggestion
of a better way.

Is there a reason for setting the class 'toggle' to the span? I don't see it
used anywhere, but perhaps you want to allow styling later?

> [% BLOCK depthControlToolbar %]
>+ <script>

Isn't this script block best defined *before* the toolbar HTML?

>+function toggleDisplay(node)
>+{
>+    var computed = node.style;
>+    var display = computed.getPropertyValue("display");
>+    if (display == "none") {
>+        node.style.display = node.oldDisplay || "block";

Will accessing node.oldDisplay raise javascript strict warnings?

>+function listToggle(event)
>+{
>+    var node = event.target;
>+    var toggle = node.nextSibling;
>+    while (toggle && toggle.tagName != "UL")
>+      toggle = toggle.nextSibling;

So this walks through the elements that follow the span looking for a UL
element, and when it finds it, toggles its display. 

>+    if (toggle) {
>+      node.innerHTML = toggleDisplay(toggle) ? "[-]" : "[+]";

Hmmm.. I wonder, how well is innerHTML supported in other browsers?

Looks like a pretty cool addition. Should the default be expanded or collapsed
on browsers that support it?

Comment 3

[Mike Shaver (:shaver emeritus)]

> Since this is only supported by browsers that implement Javascript, it's
> probably a good idea to only display the control in these cases. I've used
> document.write() in the past do achieve this, though you may have a suggestion
> of a better way.

That's a good idea, thanks.

> Is there a reason for setting the class 'toggle' to the span? I don't see it
> used anywhere, but perhaps you want to allow styling later?

I did want to allow styling, and I was also, in a previous incarnation of this 
code, walking the tree to find all the elements of that class and install the
handler.  I can remove it if you'd like, though.

> > [% BLOCK depthControlToolbar %]
> >+ <script>
> 
> Isn't this script block best defined *before* the toolbar HTML?

I don't have a preference one way or the other, so I'll move it ahead.

> >+function toggleDisplay(node)
> >+{
> >+    var computed = node.style;
> >+    var display = computed.getPropertyValue("display");
> >+    if (display == "none") {
> >+        node.style.display = node.oldDisplay || "block";
> 
> Will accessing node.oldDisplay raise javascript strict warnings?

Likely, yeah.  It's a shame that this useful pattern does that, I have to say;
I'll replace it with something more ugly and warning-proof.

(I've largely given up, in my own code, on avoiding that class of warning, but
when in Rome...)

> >+function listToggle(event)
> >+{
> >+    var node = event.target;
> >+    var toggle = node.nextSibling;
> >+    while (toggle && toggle.tagName != "UL")
> >+      toggle = toggle.nextSibling;
> 
> So this walks through the elements that follow the span looking for a UL
> element, and when it finds it, toggles its display. 

Precisely, yes.

> >+    if (toggle) {
> >+      node.innerHTML = toggleDisplay(toggle) ? "[-]" : "[+]";
> 
> Hmmm.. I wonder, how well is innerHTML supported in other browsers?

I think pretty well (certainly in IE), but it wouldn't be hard to use the
standard DOM API for changing the text contents.  Downstream changes (for using
images instead, perhaps) are easier in this format, but maybe that's not a big deal.

> Looks like a pretty cool addition. Should the default be expanded or collapsed
> on browsers that support it?

I think expanded: most dep lists are small, and type-ahead search and whatnot
work best (read: at all) on expanded lists.  Also, that's the least invasive
change to the current presentation.

Thanks for your comments!

Comment 4

[Christian Reis]

(In reply to comment #3)
> > Is there a reason for setting the class 'toggle' to the span? I don't see it
> > used anywhere, but perhaps you want to allow styling later?
> 
> I did want to allow styling, and I was also, in a previous incarnation of this 
> code, walking the tree to find all the elements of that class and install the
> handler.  I can remove it if you'd like, though.

No, it's fine; I just wanted to understand the rationale.

> I think pretty well (certainly in IE), but it wouldn't be hard to use the
> standard DOM API for changing the text contents.  Downstream changes (for 
> using images instead, perhaps) are easier in this format, but maybe that's
> not a big deal.

Well, even though Opera *does* support it <wink>:

  <Hixie__> uh, it's non-compliant
  <Hixie__> use DOM Core

I don't think downstream changes are very important at this point; we can worry
about that when the problem shows up (if ever).

Comment 5

[Mike Shaver (:shaver emeritus)]

Attachment: use DOM core, use doc.write, fix strict warning, work around safari (mis?)targetting of events

It would be nice to also get a CSS patch to make the cursor look clicky over
the toggle, but I'm already short on time.

Using document.write for all of the spans really stings on my 1200-bug dep
tree, perf-wise.  I'm tempted to make span.toggle default to display:none and
then reset it with JS, so that it's still hidden for the JS-deprived, but
doesn't have to run 1200 script fragments...

Comment 6

[Christian Reis]

Comment on attachment 142960 [details] [diff] [review]
use DOM core, use doc.write, fix strict warning, work around safari (mis?)targetting of events

>Index: dependency-tree.html.tmpl
>+      [% "<script>document.write('<span class=\"toggle\" onclick=\"listToggle(event)\">[-]</span>')</script>"
>+         IF dep.dependencies.size > 0 && !dep.seen %]

Is it too slow to live with, for you?

>+function listToggle(event)
>+{
>+    var node = event.target;
>+    if (node.nodeType == 3)
>+        node = node.parentNode;

A while back I did something similar for the bug reply functionality. caillon
requested I define:

  if (!Node) {
    /* MSIE doesn't define Node, so provide a compatibility array */
    var Node = { TEXT_NODE: 3, };
  }

and use 

  if (node.nodeType == TEXT_NODE)
     // ...

I guess it should probably be used here too, but it's a nit.

So r=kiko with the above considerations. I can add the Node code for you before
checking in if you like.

Comment 7

[Christian Reis]

(In reply to comment #5)
> tree, perf-wise.  I'm tempted to make span.toggle default to display:none and
> then reset it with JS, so that it's still hidden for the JS-deprived, but
> doesn't have to run 1200 script fragments...

For the record, this would be a problem for Lynx and other CSS-deprived nuggets.

Comment 8

[Mike Shaver (:shaver emeritus)]

> Is it too slow to live with, for you?

Nah; on a 1200-bug tree, the layout slowness is nothing compared to the database
slowness.  If I hadn't see it without the document.write, I'd not have ever
thought it slow. =)

> A while back I did something similar for the bug reply functionality. caillon
> requested I define:
> 
>   if (!Node) {
>     /* MSIE doesn't define Node, so provide a compatibility array */
>     var Node = { TEXT_NODE: 3, };
>   }
> 
> and use 
> 
>   if (node.nodeType == TEXT_NODE)

(Node.TEXT_NODE, presumably)

> So r=kiko with the above considerations. I can add the Node code for you before
> checking in if you like.

Sure, that would be great.  I'll ask for approval now as well.

> For the record, this would be a problem for Lynx and other CSS-deprived 
> nuggets.

Yeah, to the extent that they would see the [+] and it wouldn't do anything. 
That's not really a showstopper, IMO, but I don't know what the usual standards
are for that sort of thing around these parts.

Comment 9

[Dave Miller [:justdave]]

Anyone have this running somewhere I can look at it?

Comment 10

[Christian Reis]

I've put it up locally, see the URL field.

Comment 11

[Christian Reis]

It *would* be nicer if we could use custom list bullets instead of [+] text, of
course, but it's something I suggest we can do later.

Comment 12

[Mike Shaver (:shaver emeritus)]

Yeah, I figure we can do something with class-switching (toggle-collapsed vs
toggle-expanded) and clever CSS, if/when it comes to that.

Comment 13

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

Comment on attachment 142960 [details] [diff] [review]
use DOM core, use doc.write, fix strict warning, work around safari (mis?)targetting of events

Hmm, very nice.  It would be good for the toggle to be consistent with what is
used in the patch viewer, but square braces look better than parentheses (more
like the boxes that tree widgets commonly use), so it's the patch viewer that
should change.

>It would be nice to also get a CSS patch to make the cursor look clicky
>over the toggle, but I'm already short on time.

This *should* be as simple as "cursor: pointer;"
(http://www.w3.org/TR/REC-CSS2/ui.html#cursor-props).

Alternatively, make it a link, which also improves discoverability.


>I'm tempted to make span.toggle default to display:none and
>then reset it with JS, so that it's still hidden for the JS-deprived

Sounds good if it doesn't cause confusing flashing and such, but document.write
is OK for now.

r=myk

Comment 14

[Dave Miller [:justdave]]

a=justdave
resolve bug when code is checked in.  documentation+ when end-user docs are
checked in (section 5)

Comment 15

[Vlad Dascalu]

kiko, this is yours to check-in per comment #6, I believe.

Comment 16

[Christian Reis]

Checked in using a link instead of span and the Node compatibility hack; thanks
to Shaver and everyone that helped get this in.

Comment 17

[Christian Reis]

Attachment: patch checked in

Comment 18

[victory <[e-mail]>]

Attachment: docs patch for tip v1

Comment 19

[Vlad Dascalu]

Comment on attachment 213535 [details] [diff] [review]
docs patch for tip v1

+      <para>
+      On the <quote>Dependency tree</quote> page linked from each bug
+      page, you can see the dependency relationship from the bug as a
+      tree structure.
+      </para>

The text inside the <para> should have 2 more spaces in front, because you're treating <para> like a block element here. (in cases where we treat it like an inline element, the text goes on on the same line as the <para> tag)

+      <para>
+      You can change how many depth to show, and can hide Resolved bugs
+      from this page.  You can also collaps/expand dependencies from
+      each bugs on the tree view.
+      </para>

Parts of this text are not, strictly speaking, documentation for this bug, and therefore should be commited to 2.16 as well. Only the:

... You can also collaps/expand dependencies from
+      each bugs on the tree view.

should be commited from 2.18 upwards.

I'll commit this differencially but two patches would have been great.

Comment 20

[Vlad Dascalu]

I've commited parts of this patch to 2.16 as well, but I haven't granted documentation2.16 since those docs were not for this bug (this bug was commited in 2.18).

I've also done upon checkin s/many/much/, s/Resolved/resolved/, s/and can hide/and you can hide/, s/  You can also/ You can also/ .

I guess having this in the 'Using Bugzilla/Anatomy of a Bug' would have been overkill, since we already mention there the dependency fields, and this is only a viewer on top of that. 'Using Bugzilla/Hints and Tips' still seems 'not the perfect' place, but I guess we're out of options with the current docs taxonomy.

Thanks, RSZ!

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.42; previous revision: 1.41
done

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.37.2.3; previous revision: 1.37.2.2
done

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.33.2.6; previous revision: 1.33.2.5
done

Checking in xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.21.2.7; previous revision: 1.21.2.6
done

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.10.2.11; previous revision: 1.10.2.10
done

Comment 21

[Vlad Dascalu]

I've also done 'from each bugs' --> 'for each bug'. (you can't expande/collapse for terminal bugs in the tree, but since there is nothing to expande/collapse in their case...)

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.43; previous revision: 1.42
done

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.37.2.4; previous revision: 1.37.2.3
done

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.33.2.7; previous revision: 1.33.2.6
done

Checking in using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.21.2.8; previous revision: 1.21.2.7
done

Comment 22

[Vlad Dascalu]

Attachment: additional docs

I've also commited this after all, in order to specify what [-] and [+] means:

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.44; previous revision: 1.43
done

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.37.2.5; previous revision: 1.37.2.4
done

Checking in docs/xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.33.2.8; previous revision: 1.33.2.7
done

Checking in xml/using.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v  <--  using.xml
new revision: 1.21.2.9; previous revision: 1.21.2.8
done