Allow showdependencies trees to collapse

RESOLVED FIXED in Bugzilla 2.18

Status

()

defect
RESOLVED FIXED
16 years ago
7 years ago

People

(Reporter: goobix, Assigned: shaver)

Tracking

unspecified
Bugzilla 2.18
Bug Flags:
approval +
documentation +
documentation2.22 +
documentation2.20 +
documentation2.18 +

Details

()

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

16 years ago
Allow showdependencies trees to collapse.
Reporter

Comment 1

16 years ago
Posted patch Shaver's patch (obsolete) — Splinter Review
Reporter

Updated

16 years ago
Attachment #142891 - Flags: review?(kiko)

Comment 2

16 years ago
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?
> 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!
Status: NEW → ASSIGNED

Comment 4

16 years ago
(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).

Updated

16 years ago
Attachment #142891 - Flags: review?(kiko)
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...
Attachment #142891 - Attachment is obsolete: true

Comment 6

16 years ago
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.
Attachment #142960 - Flags: review?(kiko) → review+

Comment 7

16 years ago
(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.
> 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.
Flags: approval?
Target Milestone: --- → Bugzilla 2.18
Anyone have this running somewhere I can look at it?
Flags: approval?

Comment 10

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

Comment 11

16 years ago
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.
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 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
Attachment #142960 - Flags: review?(myk)
Reporter

Updated

16 years ago
Flags: approval?
a=justdave
resolve bug when code is checked in.  documentation+ when end-user docs are
checked in (section 5)
Flags: documentation?
Flags: approval?
Flags: approval+
Reporter

Comment 15

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

Comment 16

16 years ago
Checked in using a link instead of span and the Node compatibility hack; thanks
to Shaver and everyone that helped get this in.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED

Comment 17

16 years ago
Attachment #142960 - Attachment is obsolete: true
Flags: documentation2.18?
Reporter

Comment 19

14 years ago
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.
Attachment #213535 - Flags: review?(documentation) → review+
Reporter

Comment 20

14 years ago
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
Flags: documentation?
Flags: documentation2.22+
Flags: documentation2.20+
Flags: documentation2.18?
Flags: documentation2.18+
Flags: documentation+
Reporter

Comment 21

14 years ago
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
Reporter

Comment 22

14 years ago
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
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.