Closed
Bug 236424
Opened 20 years ago
Closed 20 years ago
Allow showdependencies trees to collapse
Categories
(Bugzilla :: Query/Bug List, defect)
Bugzilla
Query/Bug List
Tracking
()
RESOLVED
FIXED
Bugzilla 2.18
People
(Reporter: goobix, Assigned: shaver)
References
()
Details
Attachments
(3 files, 2 obsolete files)
2.03 KB,
patch
|
Details | Diff | Splinter Review | |
1.01 KB,
patch
|
goobix
:
review+
|
Details | Diff | Splinter Review |
806 bytes,
patch
|
Details | Diff | Splinter Review |
Allow showdependencies trees to collapse.
Reporter | ||
Comment 1•20 years ago
|
||
Reporter | ||
Updated•20 years ago
|
Attachment #142891 -
Flags: review?(kiko)
Comment 2•20 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?
Assignee | ||
Comment 3•20 years ago
|
||
> 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•20 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•20 years ago
|
Attachment #142891 -
Flags: review?(kiko)
Assignee | ||
Comment 5•20 years ago
|
||
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...
Assignee | ||
Updated•20 years ago
|
Attachment #142891 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #142960 -
Flags: review?(kiko)
Comment 6•20 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•20 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.
Assignee | ||
Comment 8•20 years ago
|
||
> 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?
Updated•20 years ago
|
Target Milestone: --- → Bugzilla 2.18
Updated•20 years ago
|
Attachment #142960 -
Flags: review?(myk)
Comment 10•20 years ago
|
||
I've put it up locally, see the URL field.
Comment 11•20 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.
Assignee | ||
Comment 12•20 years ago
|
||
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•20 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 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•20 years ago
|
Flags: approval?
Comment 14•20 years ago
|
||
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•20 years ago
|
||
kiko, this is yours to check-in per comment #6, I believe.
Comment 16•20 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: 20 years ago
Resolution: --- → FIXED
Comment 17•20 years ago
|
||
Attachment #142960 -
Attachment is obsolete: true
Updated•20 years ago
|
Flags: documentation2.18?
Comment 18•18 years ago
|
||
Attachment #213535 -
Flags: review?(documentation)
Reporter | ||
Comment 19•18 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•18 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•18 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•18 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
Updated•11 years ago
|
QA Contact: matty_is_a_geek → default-qa
You need to log in
before you can comment on or make changes to this bug.
Description
•