See attached screenshot from Firebug. We already do this if you alt+click on the expando, but for parity and better exposure of the feature we should add the context menu item.

To fix this bug, you need to add a context menu item to the Inspector panel to expand/collapse markup elements recursively. Here are a few pointers to help you get started: The inspector context menu is implemented here: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector.xul To make your new context menu item work, you will need to script its behavior in: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js (look at "copyImageDataUri" for inspiration). You will also need to add a new string + access key to this locale file for your new item: https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/inspector.dtd The function to expand all elements of a container is here: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#912 If you run into anything confusing or unclear, you can ask me anything by replying to this bug. Also, please use needinfo if you know how. Good luck!

Hey Jan, I have already started working on this- can you assign this to me?

Hi Avik, sure! Assigning the bug to you. Please let me know if you have any question / if things aren't clear. Also, please use the "needinfo?" flag on me, this will increase the chances of me seeing your comment :)

This is just a initial mockup- its more of a reference to couple of queries that I have First of all when I call this.markup.expandAll(this.selection) the subsequent call within markup-view.js is failing because apparently this.getContainer(aNode) turns out to be undefined. But I can manually go through dom tree in the inspector panel by clicking the small arrow pointers beside the node- what am missing here? Secondly should the option to expand/contract be blurred out when the selected element is not a node or when it has no child node and so can't be expanded/contracted?

Comment on attachment 8620918 [details] [diff] [review] initialMockUp.patch Review of attachment 8620918 [details] [diff] [review]: ----------------------------------------------------------------- Hi Avik, thanks for your patch! In general, I would prefer two separate context menu items, "Expand All" and "Collapse All", and you could hide whichever item doesn't make sense by setting `.hidden = true` on it (look how the other items are shown/hidden in certain conditions). ::: browser/devtools/inspector/inspector-panel.js @@ +679,5 @@ > let unique = this.panelDoc.getElementById("node-menu-copyuniqueselector"); > let copyInnerHTML = this.panelDoc.getElementById("node-menu-copyinner"); > let copyOuterHTML = this.panelDoc.getElementById("node-menu-copyouter"); > let scrollIntoView = this.panelDoc.getElementById("node-menu-scrollnodeintoview"); > + let expandContractNodeView = this.panelDoc.getElementById("node-menu-expandcontract"); Please create two of these, "collapseAll" and "expandAll", and hide them when necessary. @@ +1097,5 @@ > // remove the node from content > this.walker.removeNode(this.selection.nodeFront); > } > }, > + Nit: Trailing spaces. @@ +1101,5 @@ > + > + expandContractNodeView: function() { > + if (this.selection.isNode()) { > + if (!this.markup.expanded) { > + this.markup.expandAll(this.selection); (In reply to Avik Pal [:avikpal] from comment #4) > First of all when I call this.markup.expandAll(this.selection) the > subsequent call within markup-view.js is failing because apparently > this.getContainer(aNode) turns out to be undefined. But I can manually go > through dom tree in the inspector panel by clicking the small arrow pointers > beside the node- what am missing here? In the rest of the code, it looks like `this.markup.getContainer()` works with `this.selection.nodeFront`, maybe you forgot to write ".nodeFront"? @@ +1104,5 @@ > + if (!this.markup.expanded) { > + this.markup.expandAll(this.selection); > + } > + else { > + this.markup.collapseNode(this.selection); Nit: Please indent using only 2 spaces. @@ +1109,5 @@ > + } > + } > + else { > + return; > + } This `else` block has no effect. In general, it's preferred to place bail-out conditions in small `if` blocks at the start of the function, i.e. "if (!this.selection.isNode()) { return; }". This prevents having a big `if` block with all the function's code inside. @@ +1111,5 @@ > + else { > + return; > + } > + }, > + Nit: Trailing spaces.

Fixing some formatting related issue in the earlier patch

Thanks for your patch, Avik! I'll try it out tomorrow (Monday) and let you know how it works for me. A quick tip: If you click on "Review" next to your attachment, and then "all files", you can see what I will see when reviewing your patch. It's useful for having a quick look before the reviewer, and maybe clean up some details.

Comment on attachment 8621788 [details] [diff] [review] displaySeperateContextMenuItem.patch Review of attachment 8621788 [details] [diff] [review]: ----------------------------------------------------------------- It works for me! Now we just have to fix a few details before moving forward. This patch looks like a diff on top of your previous patch. Please fold them into a single patch/commit, and rebase it by updating your repository to the latest version (e.g. with `git pull --rebase origin master`, or the `hg` equivalent). Also, please add a patch description line (between the line "# User Avik Pal <...>" and the diff) with something like: "Bug 1157789 - Add context menu items Expand All / Collapse to markup view. r=janx" Thought: I know the bug is called "Add expand/collapse all...", but it's not very useful to actually "collapse all" elements. What do you think about renaming the items "Expand All" and "Collapse" (not "All")? That would be closer to the truth (in your code, "Expand All" calls `this.markup.expandAll()`, but "Collapse" just calls `this.markup.collapseNode()`). You also have a lot of trailing spaces. While that's not a huge problem, it is distracting for the reviewer. Please configure your editor to strip trailing spaces, or remove them manually (e.g. with a regular expression like s/\s\+$//). ::: browser/devtools/inspector/inspector-panel.js @@ +680,5 @@ > let copyInnerHTML = this.panelDoc.getElementById("node-menu-copyinner"); > let copyOuterHTML = this.panelDoc.getElementById("node-menu-copyouter"); > let scrollIntoView = this.panelDoc.getElementById("node-menu-scrollnodeintoview"); > + let expandNodeView = this.panelDoc.getElementById("node-menu-expand"); > + let collapseNodeView = this.panelDoc.getElementById("node-menu-collapse"); Nit: Please rename these "expandAll" and "collapse" to respect the apparent naming conventions. @@ +681,5 @@ > let copyOuterHTML = this.panelDoc.getElementById("node-menu-copyouter"); > let scrollIntoView = this.panelDoc.getElementById("node-menu-scrollnodeintoview"); > + let expandNodeView = this.panelDoc.getElementById("node-menu-expand"); > + let collapseNodeView = this.panelDoc.getElementById("node-menu-collapse"); > + Nit: Trailing spaces. @@ +684,5 @@ > + let collapseNodeView = this.panelDoc.getElementById("node-menu-collapse"); > + > + expandNodeView.setAttribute("disabled","true"); > + collapseNodeView.setAttribute("disabled","true"); > + Nit: Trailing spaces. @@ +686,5 @@ > + expandNodeView.setAttribute("disabled","true"); > + collapseNodeView.setAttribute("disabled","true"); > + > + let markUpContainer = this.markup.importNode(this.selection.nodeFront,false); > + if (this.selection.isNode() && markUpContainer.hasChildren) { Why do you check "this.selection.isNode()" here? I ask because for all the other context menu items, ".isNode()" is not verified in "_setupNodeMenu" but in the action function (e.g. "scrollNodeIntoView"). @@ +688,5 @@ > + > + let markUpContainer = this.markup.importNode(this.selection.nodeFront,false); > + if (this.selection.isNode() && markUpContainer.hasChildren) { > + if (markUpContainer.expanded) { > + collapseNodeView.removeAttribute("disabled"); Nit: Trailing spaces. @@ +689,5 @@ > + let markUpContainer = this.markup.importNode(this.selection.nodeFront,false); > + if (this.selection.isNode() && markUpContainer.hasChildren) { > + if (markUpContainer.expanded) { > + collapseNodeView.removeAttribute("disabled"); > + } Nit: Trailing spaces. @@ +690,5 @@ > + if (this.selection.isNode() && markUpContainer.hasChildren) { > + if (markUpContainer.expanded) { > + collapseNodeView.removeAttribute("disabled"); > + } > + expandNodeView.removeAttribute("disabled"); "Expand All" remains enabled even when the markUpContainer is already expanded, was that on purpose? @@ +692,5 @@ > + collapseNodeView.removeAttribute("disabled"); > + } > + expandNodeView.removeAttribute("disabled"); > + } > + Nit: Trailing spaces. @@ +1109,5 @@ > // remove the node from content > this.walker.removeNode(this.selection.nodeFront); > } > }, > Nit: Trailing spaces. @@ +1110,5 @@ > this.walker.removeNode(this.selection.nodeFront); > } > }, > > + expandNodeView: function() { Nit: Please call this function "expandNode", to respect apparent naming conventions. @@ +1113,5 @@ > > + expandNodeView: function() { > + this.markup.expandAll(this.selection.nodeFront); > + }, > + Nit: Trailing spaces. @@ +1114,5 @@ > + expandNodeView: function() { > + this.markup.expandAll(this.selection.nodeFront); > + }, > + > + collapseNodeView: function() { Nit: Please call this function "collapseNode", to respect apparent naming conventions. ::: browser/locales/en-US/chrome/browser/devtools/inspector.dtd @@ +73,2 @@ > mark-up elements --> > +<!ENTITY expandNodeView.label "Expand All"> Nit: To respect the naming conventions of this file, please call this entity something like "inspectorExpandNode". @@ +75,5 @@ > + > +<!-- LOCALIZATION NOTE (collapseNodeView.label): This is the label > + shown in the inspector contextual-menu for recursively collapsing > + mark-up elements --> > +<!ENTITY collapseNodeView.label "Collapse All"> Nit: Same as above, with something like "inspectorCollapseNode". Also, please change this string to "Collapse" (without "All") if you think that makes sense.

(In reply to Jan Keromnes [:janx] from comment #9) > Comment on attachment 8621788 [details] [diff] [review] > displaySeperateContextMenuItem.patch > > Review of attachment 8621788 [details] [diff] [review]: > ----------------------------------------------------------------- > > It works for me! Now we just have to fix a few details before moving forward. > > This patch looks like a diff on top of your previous patch. Please fold them > into a single patch/commit, and rebase it by updating your repository to the > latest version (e.g. with `git pull --rebase origin master`, or the `hg` > equivalent). > > Also, please add a patch description line (between the line "# User Avik Pal > <...>" and the diff) with something like: "Bug 1157789 - Add context menu > items Expand All / Collapse to markup view. r=janx" Will surely do > > Thought: I know the bug is called "Add expand/collapse all...", but it's not > very useful to actually "collapse all" elements. What do you think about > renaming the items "Expand All" and "Collapse" (not "All")? That would be > closer to the truth (in your code, "Expand All" calls Exactly I also had this thought- collapsing seems something which should collapse a node along with its children, collapsing all doesn't make much of a sense > `this.markup.expandAll()`, but "Collapse" just calls > `this.markup.collapseNode()`). > > You also have a lot of trailing spaces. While that's not a huge problem, it > is distracting for the reviewer. Please configure your editor to strip > trailing spaces, or remove them manually (e.g. with a regular expression > like s/\s\+$//). > Really sorry! I thought I had them removed- gotta check once more. > ::: browser/devtools/inspector/inspector-panel.js > @@ +680,5 @@ > > let copyInnerHTML = this.panelDoc.getElementById("node-menu-copyinner"); > > let copyOuterHTML = this.panelDoc.getElementById("node-menu-copyouter"); > > let scrollIntoView = this.panelDoc.getElementById("node-menu-scrollnodeintoview"); > > + let expandNodeView = this.panelDoc.getElementById("node-menu-expand"); > > + let collapseNodeView = this.panelDoc.getElementById("node-menu-collapse"); > > Nit: Please rename these "expandAll" and "collapse" to respect the apparent > naming conventions. > > @@ +681,5 @@ > > let copyOuterHTML = this.panelDoc.getElementById("node-menu-copyouter"); > > let scrollIntoView = this.panelDoc.getElementById("node-menu-scrollnodeintoview"); > > + let expandNodeView = this.panelDoc.getElementById("node-menu-expand"); > > + let collapseNodeView = this.panelDoc.getElementById("node-menu-collapse"); > > + > > Nit: Trailing spaces. > > @@ +684,5 @@ > > + let collapseNodeView = this.panelDoc.getElementById("node-menu-collapse"); > > + > > + expandNodeView.setAttribute("disabled","true"); > > + collapseNodeView.setAttribute("disabled","true"); > > + > > Nit: Trailing spaces. > > @@ +686,5 @@ > > + expandNodeView.setAttribute("disabled","true"); > > + collapseNodeView.setAttribute("disabled","true"); > > + > > + let markUpContainer = this.markup.importNode(this.selection.nodeFront,false); > > + if (this.selection.isNode() && markUpContainer.hasChildren) { > > Why do you check "this.selection.isNode()" here? I ask because for all the > other context menu items, ".isNode()" is not verified in "_setupNodeMenu" > but in the action function (e.g. "scrollNodeIntoView"). Because my understanding is that if something is not a node after selecting that element we won't be showing options to expand/collapse- as the view elements are being handled in _setupNodeMenu I thought of putting it here. > > @@ +688,5 @@ > > + > > + let markUpContainer = this.markup.importNode(this.selection.nodeFront,false); > > + if (this.selection.isNode() && markUpContainer.hasChildren) { > > + if (markUpContainer.expanded) { > > + collapseNodeView.removeAttribute("disabled"); > > Nit: Trailing spaces. > > @@ +689,5 @@ > > + let markUpContainer = this.markup.importNode(this.selection.nodeFront,false); > > + if (this.selection.isNode() && markUpContainer.hasChildren) { > > + if (markUpContainer.expanded) { > > + collapseNodeView.removeAttribute("disabled"); > > + } > > Nit: Trailing spaces. > > @@ +690,5 @@ > > + if (this.selection.isNode() && markUpContainer.hasChildren) { > > + if (markUpContainer.expanded) { > > + collapseNodeView.removeAttribute("disabled"); > > + } > > + expandNodeView.removeAttribute("disabled"); > > "Expand All" remains enabled even when the markUpContainer is already > expanded, was that on purpose? Yes! Think of a scenario where a node is expanded but one of its children has been collapsed then also markUpContainer.expanded for the root node is true- A way out is to recursively check whether all the children are expanded(to be precise that comes down to checking if all the leaf nodes in the DOM tree is visible) but then again say a user collapses couple of small subtrees within the DOM tree but to have the "Expand all" button enabled on the root node one has to again fully expand those small subtrees which actually is finding and then scrolling to those collapsed node within a small inspector window. > > @@ +692,5 @@ > > + collapseNodeView.removeAttribute("disabled"); > > + } > > + expandNodeView.removeAttribute("disabled"); > > + } > > + > > Nit: Trailing spaces. > > @@ +1109,5 @@ > > // remove the node from content > > this.walker.removeNode(this.selection.nodeFront); > > } > > }, > > > > Nit: Trailing spaces. > > @@ +1110,5 @@ > > this.walker.removeNode(this.selection.nodeFront); > > } > > }, > > > > + expandNodeView: function() { > > Nit: Please call this function "expandNode", to respect apparent naming > conventions. > > @@ +1113,5 @@ > > > > + expandNodeView: function() { > > + this.markup.expandAll(this.selection.nodeFront); > > + }, > > + > > Nit: Trailing spaces. > > @@ +1114,5 @@ > > + expandNodeView: function() { > > + this.markup.expandAll(this.selection.nodeFront); > > + }, > > + > > + collapseNodeView: function() { > > Nit: Please call this function "collapseNode", to respect apparent naming > conventions. > > ::: browser/locales/en-US/chrome/browser/devtools/inspector.dtd > @@ +73,2 @@ > > mark-up elements --> > > +<!ENTITY expandNodeView.label "Expand All"> > > Nit: To respect the naming conventions of this file, please call this entity > something like "inspectorExpandNode". > > @@ +75,5 @@ > > + > > +<!-- LOCALIZATION NOTE (collapseNodeView.label): This is the label > > + shown in the inspector contextual-menu for recursively collapsing > > + mark-up elements --> > > +<!ENTITY collapseNodeView.label "Collapse All"> > > Nit: Same as above, with something like "inspectorCollapseNode". > > Also, please change this string to "Collapse" (without "All") if you think > that makes sense. Yep! I will make these changes to adhere to the naming conventions- I will put a patch up for review by tomorrow.

(In reply to Avik Pal [:avikpal] from comment #10) > > Also, please add a patch description line (between the line "# User Avik Pal > > <...>" and the diff) with something like: "Bug 1157789 - Add context menu > > items Expand All / Collapse to markup view. r=janx" > Will surely do Great! > > You also have a lot of trailing spaces. While that's not a huge problem, it > > is distracting for the reviewer. Please configure your editor to strip > > trailing spaces, or remove them manually (e.g. with a regular expression > > like s/\s\+$//). > > > Really sorry! I thought I had them removed- gotta check once more. Not a big issue, don't worry. I just like to complain about small details :) > > @@ +686,5 @@ > > > + expandNodeView.setAttribute("disabled","true"); > > > + collapseNodeView.setAttribute("disabled","true"); > > > + > > > + let markUpContainer = this.markup.importNode(this.selection.nodeFront,false); > > > + if (this.selection.isNode() && markUpContainer.hasChildren) { > > > > Why do you check "this.selection.isNode()" here? I ask because for all the > > other context menu items, ".isNode()" is not verified in "_setupNodeMenu" > > but in the action function (e.g. "scrollNodeIntoView"). > Because my understanding is that if something is not a node after selecting > that element we won't be > showing options to expand/collapse- as the view elements are being handled > in _setupNodeMenu I thought of > putting it here. That makes sense, I'm just curious why the other items do it the other way around. I'll have a closer look when reviewing your next patch. > > "Expand All" remains enabled even when the markUpContainer is already > > expanded, was that on purpose? > Yes! Think of a scenario where a node is expanded but one of its children > has been collapsed Right, I thought about that as well. I think in that particular case you'd naturally right click on the collapsed child to look for "Expand All", instead of right clicking on the already expanded parent node, but I don't feel too strongly either way. Your argument makes sense. > > Also, please change this string to "Collapse" (without "All") if you think > > that makes sense. > Yep! I will make these changes to adhere to the naming conventions- I will > put a patch up for review by tomorrow. That's great, thanks Avik!

Comment on attachment 8624479 [details] [diff] [review] displayContextMenuItemForExpandCollapseNode.patch Review of attachment 8624479 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me! Please just fix the two micro-nits and we'll be good to go. I submitted your patch to "try", our test infrastructure. The results will start appearing here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9a4fe4e8b21 Green is good, orange/red means something is broken, but failures are not always related to your patch. ::: browser/devtools/inspector/inspector-panel.js @@ +688,5 @@ > let scrollIntoView = this.panelDoc.getElementById("node-menu-scrollnodeintoview"); > + let expandAll = this.panelDoc.getElementById("node-menu-expand"); > + let collapse = this.panelDoc.getElementById("node-menu-collapse"); > + > + expandAll.setAttribute("disabled","true"); Nit: Space after comma. @@ +689,5 @@ > + let expandAll = this.panelDoc.getElementById("node-menu-expand"); > + let collapse = this.panelDoc.getElementById("node-menu-collapse"); > + > + expandAll.setAttribute("disabled","true"); > + collapse.setAttribute("disabled","true"); Nit: Space after comma.

Comment on attachment 8624895 [details] [diff] [review] displayContextMenuItemForExpandCollapseNodeUpdated.patch Review of attachment 8624895 [details] [diff] [review]: ----------------------------------------------------------------- Fixed couple of issues related to adhering to coding style.

Comment on attachment 8624895 [details] [diff] [review] displayContextMenuItemForExpandCollapseNodeUpdated.patch Review of attachment 8624895 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Avik! One more nit I missed (sorry). The try run shows everything green, except two unrelated-looking jobs. ::: browser/devtools/inspector/inspector-panel.js @@ +691,5 @@ > + > + expandAll.setAttribute("disabled", "true"); > + collapse.setAttribute("disabled", "true"); > + > + let markUpContainer = this.markup.importNode(this.selection.nodeFront,false); Nit: Space after comma.

I guess I missed that too ;)

Avik, your patch is now ready to be landed in Firefox. You can ask a sheriff to land it for you by adding the "checkin-needed" keyword to this bug. Please also copy/paste the treeherder link for convenience, to show that your patch is safe and that it probably won't break any tests.

As a follow-up patch, it would be great to add tests for this feature. The collapse/expandAll behavior is already covered in several markupview tests (in browser/devtools/markupview/test/, as you can see when searching for "expandAll" on DXR: https://dxr.mozilla.org/mozilla-central/search?q=expandAll ). However, the new context menu items are not. For this you could add a new file in browser/devtools/markupview/test/ (e.g. "browser_markupview_expandcollapse.js"), taking inspiration from https://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/test/browser_markupview_links_04.js (mostly from the code in "add_task") to see how to open a context menu, how to select an item using getElementById() and how to verify that it's enabled / does what it's supposed to do. Please let me know if you're interested. Otherwise, I'll open a new bug to add a test for this feature.

I will put up a patch with the tests for you to review. Then I guess after we have integrated the tests we should be asking for the consolidated patch to be checked in as the work is going to happen under the same bug.

Thanks for your interest in writing a test for this! If you have any question, or if you run into any trouble, please don't hesitate to let us know. As for the patches, we can either check them in separately or consolidated together, you can decide how you prefer to do that.

Actually Jan, I am caught up with some work and it may be a few days before I can take a look into the tests part. I guess for now we can check in the implementation or if this can wait for a few days I'll put up the patches myself- let me know.

No problem, let's go forward with this patch: - The keyword "checkin-needed" puts it in the sheriffs' landing queue. - The keyword "leave-open" asks to not "RESOLVE FIX" the bug. We'll remove it when landing the test. Try push was: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9a4fe4e8b21 Thanks again Avik!

Congratulations Avik, your patch landed in mozilla-central! Today, a snapshot of mozilla-central becomes the new mozilla-aurora, the old mozilla-aurora becomes the new mozilla-beta, and the old mozilla-beta gets sent to everyone that uses Firefox stable. I the snapshot already happened this morning, but we have a new cycle every 6 weeks or so. This means that your patch will be: - In tomorrow's Firefox Nightly, - In Firefox Developer Edition (aurora) 6 weeks from now, - In stable Firefox (release) 3 months from now.

Avik, I will be gone for a month starting Friday. I'll be back the second week of August, but if you would like a review before I'm back, I'd suggest asking someone like :bgrins for help. (Thanks Brian!)

Jan, Thanks a lot for your help.

(In reply to Jan Keromnes [:janx] from comment #19) > As a follow-up patch, it would be great to add tests for this feature. > > The collapse/expandAll behavior is already covered in several markupview > tests (in browser/devtools/markupview/test/, as you can see when searching > for "expandAll" on DXR: > https://dxr.mozilla.org/mozilla-central/search?q=expandAll ). > > However, the new context menu items are not. For this you could add a new > file in browser/devtools/markupview/test/ (e.g. > "browser_markupview_expandcollapse.js"), I'm a bit confused- Shouldn't I put it at mozilla-central/source/browser/devtools/inspector/test/ taking inspiration from > https://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/ > test/browser_markupview_links_04.js (mostly from the code in "add_task") to > see how to open a context menu, how to select an item using getElementById() > and how to verify that it's enabled / does what it's supposed to do. > > Please let me know if you're interested. Otherwise, I'll open a new bug to > add a test for this feature.

Hi Avik, thanks for following up on this! Please place the test where you think makes the most sense. When I gave advice on how to start, I thought it made sense to place it in devtools/markupview/test/, but since your patch changed code that lives in devtools/inspector/, maybe devtools/inspector/test/ is a better place for your test.

proposed patch for testing proper visibility of the expand all/collapse button

tests proper visibility of context menu items expand and collapse

Hi Avik, thanks for writing that test! I'll give it a try tomorrow.

Comment on attachment 8660443 [details] [diff] [review] testContextMenuItemsExpandCollapse.patch Review of attachment 8660443 [details] [diff] [review]: ----------------------------------------------------------------- Works great! Thanks Avik. However, I think your test only verifies the visibility of the context menu items, not that they actually work as expected. Could you please expand your test a little to also verify that nodes are properly expanded/collapsed after a click? I simply added a `return;` statement as the top of both `expandNode()` and `collapseNode()`, it would be nice if the test fails when I do that. ::: browser/devtools/inspector/test/browser_inspector_expand-collapse.js @@ +3,5 @@ > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + > +//Tests that context menu items exapnd all and collapse are shown properly Nit: Please add a space after "//" and "." at the end.

I guess that the inner workings of collapse/expandAll is already tested as part of the basic widget tests[https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/test/browser_treeWidget_basic.js?offset=2500#195] Nonetheless I'm stuck at a road block here. After emulating a click on, say the "expand all" button the test runner need to wait for the markupcontainer/node to get updated before checking whether the expression markUpContainer.expanded is true or not. After going through the existing code base my guess that waiting for some inspector event to occur should do the trick- but till now I've tested with several events mentioned/in use at [https://dxr.mozilla.org/mozilla-central/source/browser/devtools/inspector/inspector-panel.js] but all of them give a timed out error- meaning that the awaited event never occurred; Could you elaborate a bit on how this can be achieved or/and what event gets triggered in the context of inspector (the documentation within the code in some places is not that much detailed)?

Hi Avik, sorry that expanding the test is more complex than I originally thought. The reason I feel that collapseNode and expandAll are not functionally tested is because when I apply a patch like: diff --git a/browser/devtools/markupview/markup-view.js b/browser/devtools/markupview/markup-view.js index e44af44..d3096e6 100644 --- a/browser/devtools/markupview/markup-view.js +++ b/browser/devtools/markupview/markup-view.js @@ -978,6 +978,7 @@ MarkupView.prototype = { * @param aContainer The container to expand. */ _expandAll: function(aContainer) { + return promise.resolve(); return this._expandContainer(aContainer).then(() => { let child = aContainer.children.firstChild; let promises = []; @@ -1005,7 +1006,7 @@ MarkupView.prototype = { */ collapseNode: function(aNode) { let container = this.getContainer(aNode); - container.setExpanded(false); + container.setExpanded(true); }, /** the context menu items are obviously broken, but no devtools test seems to complain about it (I ran `./mach mochitest browser/devtools/shared/test/`). Debugging test time outs can be a pain. The events you're trying seem to make sense at first glance -- could you please upload a work-in-progress patch of your test, with a quick comment where you see the time-out and what you tried? Nothing too fancy, I'd just like to apply it locally and see if I can find out why your test times out.

Regarding the broken tests/timeouts in existing devtools code- there's quite a lot of thing that needs to be done (a bit of refactoring/pruning); for an example many methods may be moved to head.js from individual test files for a more streamlined approach but I guess that would be the purview of some other bug- may be filed and fixed as part of a larger refactoring task. Now coming back to the issue in hand here is a code snippet that I'm trying to use- I've left out the obvious setup/bootstrapping code from this small piece for better readability let nodeMenuExpandElement = inspector.panelDoc.getElementById("node-menu-expand"); let front = yield getNodeFrontForSelector("#parent-node", inspector); yield selectNode(front, inspector); contextMenuClick(getContainerForNodeFront(front, inspector).tagLine); //then I try to see whether the expandAll functionality really works yield testExpandAll(); //in testExpandAll function* testExpandAll() { /* here follows the inspector event which isn't getting resolved- I'm not so sure about the event we are waiting for; is there any documentation regarding this other than the bits of comments in the source file */ let onUpdation = inspector.once("inspector-updated"); dispatchCommandEvent(nodeMenuExpandElement); info("Waiting for updation to occur"); yield onUpdation; let markUpContainer = getContainerForNodeFront(front, inspector); ok(markUpContainer.expanded, "node has been successfully expanded"); }

Hi Avik, (In reply to Avik Pal [:avikpal] from comment #37) > Regarding the broken tests/timeouts in existing devtools code- there's quite > a lot of thing that needs to be done (a bit of refactoring/pruning); Are there broken tests or tests that time out in existing devtools code? That's not supposed to happen, please let us know which tests are broken so we can fix them. > for an example many methods may be moved to head.js from individual test files for > a more streamlined approach but I guess that would be the purview of some > other bug- may be filed and fixed as part of a larger refactoring task. That's a very good suggestion! If you're interested in helping out with that, please have a look at bug 1181833. > Now coming back to the issue in hand here is a code snippet that I'm trying > to use- I've left out the obvious setup/bootstrapping code from this small > piece for better readability Thanks for the snippet, however please don't leave out the "obvious" setup/bootstrapping code. I'm not very familiar with these tests, but I can help you if you upload a full patch (even if your test doesn't work). That way I can apply it locally, see what the errors are, add some logging etc to investigate what goes wrong. This is hard to do on a partial code snippet -- much easier with a real patch. > let nodeMenuExpandElement = inspector.panelDoc.getElementById("node-menu-expand"); > let front = yield getNodeFrontForSelector("#parent-node", inspector); > yield selectNode(front, inspector); > contextMenuClick(getContainerForNodeFront(front, inspector).tagLine); > > //then I try to see whether the expandAll functionality really works > yield testExpandAll(); > > //in testExpandAll > function* testExpandAll() { > /* > here follows the inspector event which isn't getting resolved- I'm not so sure about the > event we are waiting for; is there any documentation regarding this other than the bits of > comments in the source file > */ > let onUpdation = inspector.once("inspector-updated"); > dispatchCommandEvent(nodeMenuExpandElement); > > info("Waiting for updation to occur"); > yield onUpdation; > > let markUpContainer = getContainerForNodeFront(front, inspector); > ok(markUpContainer.expanded, "node has been successfully expanded"); > } I'm not really what "inspector-updated" means or when it is supposed to happen. Maybe you could try to wait for "markupmutation" instead? That's what this test does: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/test/browser_inspector_menu-03-paste-items.js#110

This is a work in progress patch which times out. I had earlier tested with markupmutation but to no avail- As far as I can understand from the code ( for example at https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/breadcrumbs.js#670)that markupmutation events are mostly associated with creation/deletion etc. of nodes and is of not much of an use in this case; correct me if I'm wrong.

Hi Avik, Thanks for your patch! I will give it a try tomorrow. Sorry for the slight delay, I have a lot on my plate these days.

I got your test to pass! The trick (hat tip to :pbrosset) was to use `waitForMultipleChildrenUpdates(inspector)` [1] instead of `inspector.once("inspector-updated")` before verifying that `markUpContainer.expanded` is true. [1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/markupview/test/head.js?offset=0#664

Comment on attachment 8666357 [details] [diff] [review] WIP-testContextMenuItemsExpandCollapse.patch Review of attachment 8666357 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for digging in and finding out the actual event to make this work.

Comment on attachment 8668013 [details] [diff] [review] testContextMenuItemsExpandCollapse.patch Review of attachment 8668013 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for digging in and finding out the actual event to make this work.

Comment on attachment 8668013 [details] [diff] [review] testContextMenuItemsExpandCollapse.patch Review of attachment 8668013 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Avik, works great! Thank you for writing this test. I added a few comments in-line, and you'll also need to rebase your patch (for example, we recently moved all files from browser/devtools/ to devtools/client/). ::: browser/devtools/inspector/test/browser_inspector_expand-collapse.js @@ +31,5 @@ > + > + ok(!nodeMenuExpandElement.hasAttribute("disabled"), "ExpandAll option is enabled"); > + yield testExpandAll(); > + > + yield selectNode(front, inspector); Why do you select that node again? @@ +39,5 @@ > + > + ok(!nodeMenuCollapseElement.hasAttribute("disabled"), "Collapse option is enabled"); > + yield testCollapse(); > + > + function* testExpandAll() { Nit: I don't think you need to create these `testExpandAll` and `testCollapse` helpers. Instead, you could write something more linear like: dispatchCommandEvent(nodeMenuExpandElement); info("Waiting for expansion to occur"); yield waitForMultipleChildrenUpdates(inspector); let markUpContainer = getContainerForNodeFront(front, inspector); ok(markUpContainer.expanded, "node has been successfully expanded");

(In reply to Jan Keromnes [:janx] from comment #45) > Comment on attachment 8668013 [details] [diff] [review] > testContextMenuItemsExpandCollapse.patch > > Review of attachment 8668013 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks Avik, works great! Thank you for writing this test. > > I added a few comments in-line, and you'll also need to rebase your patch > (for example, we recently moved all files from browser/devtools/ to > devtools/client/). > > ::: browser/devtools/inspector/test/browser_inspector_expand-collapse.js > @@ +31,5 @@ > > + > > + ok(!nodeMenuExpandElement.hasAttribute("disabled"), "ExpandAll option is enabled"); > > + yield testExpandAll(); > > + > > + yield selectNode(front, inspector); > > Why do you select that node again? This is a reminiscent of a WIP patch that I should've asked you earlier- the issue is after triggering an expansion of the parent node when I check for the visbility of the collapse option(after simulating a right click onto it)- it turns out to be a failure; What my guess is that in the actual expansion is altering some changes to the inspector state along with the node selection- perhaps the children updates are inavalidating the present node selection. The code of selectNode actually waits for an 'inspector-update' event to get triggered after the nodeFront is set again. The fact the test doesn't time out means that the event is actually getting triggered after the setNodeFront call which interesting enough has a comment section that points to some previously faced issue with reselecting node [https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/selection.js#165] Nonetheless I'm not exactly sure of the cause behind this- could you shed some light? > > @@ +39,5 @@ > > + > > + ok(!nodeMenuCollapseElement.hasAttribute("disabled"), "Collapse option is enabled"); > > + yield testCollapse(); > > + > > + function* testExpandAll() { > > Nit: I don't think you need to create these `testExpandAll` and > `testCollapse` helpers. Instead, you could write something more linear like: > > dispatchCommandEvent(nodeMenuExpandElement); > info("Waiting for expansion to occur"); > yield waitForMultipleChildrenUpdates(inspector); > let markUpContainer = getContainerForNodeFront(front, inspector); > ok(markUpContainer.expanded, "node has been successfully expanded"); Done- I'should've refactored the WIP patch before submitting the final patch- As soon as we've sorted out the aforementioned issue I'll upload the final patch.

Interesting! Thanks for digging that far into the problem. I'm not sure what happens either, but I'm OK with leaving the re-selection line. I just wanted to make sure it wasn't there by mistake. Maybe add a comment above that line, e.g. "We need to select that node again after it has been expanded."? At this point I don't think it's worth it to investigate the issue further, because the test works and it will ensure that your features won't break.

I accidentally marked the actual checkedIn patch[https://bug1157789.bmoattachments.org/attachment.cgi?id=8625017] as obsolete. Is there any way to revert this or should I re-upload the patch with some comments?

(In reply to Avik Pal [:avikpal] from comment #49) > I accidentally marked the actual checkedIn > patch[https://bug1157789.bmoattachments.org/attachment.cgi?id=8625017] as > obsolete. Is there any way to revert this or should I re-upload the patch > with some comments? You can uncheck it from the 'details' page if you click edit: https://bugzilla.mozilla.org/attachment.cgi?id=8625017&action=edit. I went ahead and unmarked it.

Comment on attachment 8669603 [details] [diff] [review] testContextMenuItemsExpandCollapse.patch Review of attachment 8669603 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Brian! And thanks Avik: The test works like a charm, and fails if the feature somehow stops working. That's great! I sent it to treeherder to ensure that it will work in every platform: https://treeherder.mozilla.org/#/jobs?repo=try&revision=63c3a51b8726 Nit: Please add a patch description (e.g. "Bug 1157789 - Add a test for the Inspector's collapse/expandAll context menu items. r=janx").

Comment on attachment 8669603 [details] [diff] [review] testContextMenuItemsExpandCollapse.patch ># HG changeset patch ># Parent 3d7532ce81ac571962abc3b99582fe7f5d685192 ># User Avik Pal <avikpal.me@gmail.com> >Bug 1157789 - Add a test for the Inspector's collapse/expandAll context menu items. r=janx >diff --git a/devtools/client/inspector/test/browser.ini b/devtools/client/inspector/test/browser.ini >--- a/devtools/client/inspector/test/browser.ini >+++ b/devtools/client/inspector/test/browser.ini >@@ -34,16 +34,17 @@ support-files = > [browser_inspector_breadcrumbs_keybinding.js] > [browser_inspector_breadcrumbs_menu.js] > [browser_inspector_breadcrumbs_mutations.js] > [browser_inspector_delete-selected-node-01.js] > [browser_inspector_delete-selected-node-02.js] > [browser_inspector_delete-selected-node-03.js] > [browser_inspector_destroy-after-navigation.js] > [browser_inspector_destroy-before-ready.js] >+[browser_inspector_expand-collapse.js] > [browser_inspector_gcli-inspect-command.js] > skip-if = e10s # GCLI isn't e10s compatible. See bug 1128988. > [browser_inspector_highlighter-01.js] > [browser_inspector_highlighter-02.js] > [browser_inspector_highlighter-03.js] > [browser_inspector_highlighter-04.js] > [browser_inspector_highlighter-by-type.js] > [browser_inspector_highlighter-comments.js] >diff --git a/devtools/client/inspector/test/browser_inspector_expand-collapse.js b/devtools/client/inspector/test/browser_inspector_expand-collapse.js >new file mode 100644 >--- /dev/null >+++ b/devtools/client/inspector/test/browser_inspector_expand-collapse.js >@@ -0,0 +1,54 @@ >+/* vim: set ts=2 et sw=2 tw=80: */ >+/* Any copyright is dedicated to the Public Domain. >+ http://creativecommons.org/publicdomain/zero/1.0/ */ >+ >+"use strict"; >+ >+// Tests that context menu items exapnd all and collapse are shown properly. >+ >+const TEST_URL = "data:text/html;charset=utf-8,<div id='parent-node'><div id='child-node'></div></div>"; >+ >+add_task(function* () { >+ >+ // Test is often exceeding time-out threshold, similar to Bug 1137765 >+ requestLongerTimeout(2); >+ >+ let {inspector, testActor} = yield openInspectorForURL(TEST_URL); >+ >+ let nodeMenuCollapseElement = inspector.panelDoc.getElementById("node-menu-collapse"); >+ let nodeMenuExpandElement = inspector.panelDoc.getElementById("node-menu-expand"); >+ >+ info("Selecting the parent node"); >+ >+ let front = yield getNodeFrontForSelector("#parent-node", inspector); >+ >+ yield selectNode(front, inspector); >+ >+ info("Simulating context menu click on the selected node container."); >+ contextMenuClick(getContainerForNodeFront(front, inspector).tagLine); >+ >+ ok(nodeMenuCollapseElement.hasAttribute("disabled"), "Collapse option is disabled"); >+ >+ ok(!nodeMenuExpandElement.hasAttribute("disabled"), "ExpandAll option is enabled"); >+ >+ info("Testing whether expansion works properly"); >+ dispatchCommandEvent(nodeMenuExpandElement); >+ info("Waiting for expansion to occur"); >+ yield waitForMultipleChildrenUpdates(inspector); >+ let markUpContainer = getContainerForNodeFront(front, inspector); >+ ok(markUpContainer.expanded, "node has been successfully expanded"); >+ >+ //reslecting node after expansion >+ yield selectNode(front, inspector); >+ >+ info("Testing whether collapse works properly"); >+ info("Simulating context menu click on the selected node container."); >+ contextMenuClick(getContainerForNodeFront(front, inspector).tagLine); >+ >+ ok(!nodeMenuCollapseElement.hasAttribute("disabled"), "Collapse option is enabled"); >+ >+ dispatchCommandEvent(nodeMenuCollapseElement); >+ info("Waiting for collapse to occur"); >+ yield waitForMultipleChildrenUpdates(inspector); >+ ok(!markUpContainer.expanded, "node has been successfully collapsed"); >+}); >\ No newline at end of file >diff --git a/devtools/client/inspector/test/head.js b/devtools/client/inspector/test/head.js >--- a/devtools/client/inspector/test/head.js >+++ b/devtools/client/inspector/test/head.js >@@ -477,16 +477,45 @@ function dispatchCommandEvent(node) { > info("Dispatching command event on " + node); > let commandEvent = document.createEvent("XULCommandEvent"); > commandEvent.initCommandEvent("command", true, true, window, 0, false, false, > false, false, null); > node.dispatchEvent(commandEvent); > } > > /** >+ * A helper that simulates a contextmenu event on the given chrome DOM element. >+ */ >+function contextMenuClick(element) { >+ let evt = element.ownerDocument.createEvent('MouseEvents'); >+ let button = 2; // right click >+ >+ evt.initMouseEvent('contextmenu', true, true, >+ element.ownerDocument.defaultView, 1, 0, 0, 0, 0, false, >+ false, false, false, button, null); >+ >+ element.dispatchEvent(evt); >+} >+ >+/** >+ * A helper that fetches a front for a node that matches the given selector or >+ * doctype node if the selector is falsy. >+ */ >+function* getNodeFrontForSelector(selector, inspector) { >+ if (selector) { >+ info("Retrieving front for selector " + selector); >+ return getNodeFront(selector, inspector); >+ } else { >+ info("Retrieving front for doctype node"); >+ let {nodes} = yield inspector.walker.children(inspector.walker.rootNode); >+ return nodes[0]; >+ } >+} >+ >+/** > * Encapsulate some common operations for highlighter's tests, to have > * the tests cleaner, without exposing directly `inspector`, `highlighter`, and > * `testActor` if not needed. > * > * @param {String} > * The highlighter's type > * @return > * A generator function that takes an object with `inspector` and `testActor` >@@ -529,8 +558,38 @@ const getHighlighterHelperFor = (type) = > }, > > finalize: function*() { > yield highlighter.finalize(); > } > }; > } > ); >+ >+// The expand all operation of the markup-view calls itself recursively and >+// there's not one event we can wait for to know when it's done >+// so use this helper function to wait until all recursive children updates are done. >+function* waitForMultipleChildrenUpdates(inspector) { >+// As long as child updates are queued up while we wait for an update already >+// wait again >+ if (inspector.markup._queuedChildUpdates && >+ inspector.markup._queuedChildUpdates.size) { >+ yield waitForChildrenUpdated(inspector); >+ return yield waitForMultipleChildrenUpdates(inspector); >+ } >+} >+ >+/** >+ * Using the markupview's _waitForChildren function, wait for all queued >+ * children updates to be handled. >+ * @param {InspectorPanel} inspector The instance of InspectorPanel currently >+ * loaded in the toolbox >+ * @return a promise that resolves when all queued children updates have been >+ * handled >+ */ >+function waitForChildrenUpdated({markup}) { >+ info("Waiting for queued children updates to be handled"); >+ let def = promise.defer(); >+ markup._waitForChildren().then(() => { >+ executeSoon(def.resolve); >+ }); >+ return def.promise; >+}

By the way there are couple of busted tests at https://treeherder.mozilla.org/#/jobs?repo=try&revision=63c3a51b8726. But the tests related to this seems to be ok but I can not zero in on the actual error.

Hi Avik, I'm not really sure why you're flagging me for review again on this patch - did you change anything in your patch? If you didn't, please feel free to "keep my r+" by setting "r+" on your new patches yourself (this allows you rebase and re-upload without having to wait for review again). As for the busted tests, they do look unrelated even if more severe than usual. I'll retrigger a few jobs, but if you haven't modified the code since my try push, we should be good to go with a "checkin-needed" request soon.

I have my suspicions for the increased test failures: We recently made a big change by moving many files, and this caused many tests to be re-ordered. Sadly, some of our tests get used to being run in a certain order, and sometimes they get upset when we change it...

(In reply to Jan Keromnes [:janx] from comment #54) > Hi Avik, I'm not really sure why you're flagging me for review again on this > patch - did you change anything in your patch? If you didn't, please feel > free to "keep my r+" by setting "r+" on your new patches yourself (this > allows you rebase and re-upload without having to wait for review again). > > As for the busted tests, they do look unrelated even if more severe than > usual. I'll retrigger a few jobs, but if you haven't modified the code since > my try push, we should be good to go with a "checkin-needed" request soon. I just added a line describing the test patch- I'm flagging "checkin-needed" then.

So I've set marked this as checkin-needed. How do I get informed when this finally gets pushed?

Well done Avik! Thanks again for writing that test. Now that your patch is in fx-team, it will soon get merged into mozilla-central, at which point Pulsebot should automatically post in this bug again and close it.

I have reproduced this bug on Firefox nightly Version 40.0a1 according to (2015-04-23) It is fixed and verified on Latest Developer Edition Build ID 20151103004217 User Agent Mozilla/5.0 (Windows NT 6.3; rv:44.0) Gecko/20100101 Firefox/44.0 Tested OS--Windows 8.1 32bit

I've updated the bit on the popup menu to include these items: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML#Element_popup_menu I've also changed the format of that from a screenshot to a bulleted list of links into the table. Hopefully this is a bit easier to navigate, although not as pretty.