1405246 - Replace context menu item "Collapse" by "Collapse all"

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

Attachment: markupview-contextmenu-collapse.png

After Bug 1401889, you can now alt+click on the "arrow expander" of an expanded node to collapse all the nodes under it.

Currently, the context menu displayed for a node contains the options "Expand all" and "Collapse". In bug 1271708, we were discussing about removing "Collapse" as it is really redundant with the existing UI. Now that "Collapse all" is available it would make sense to simply swap "Collapse" with "Collapse all".

This will balance the existing "Expand all" entry, will make the new "Collapse all" feature more discoverable etc...

To fix this bug, you will need to: 
- update the callback used for the context menu item see [1]
- update the label of the menu entry at [2] (note that the localization note already "wrongly" describes a "collapse all" behaviour, so no need to update it). 

Heads up: updating localization strings follows some guidelines, have a look at [3]

Should be a good first bug!

[1] http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/devtools/client/inspector/inspector.js#1251-1256
[2] http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/devtools/client/locales/en-US/inspector.properties#274-277
[3] https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Updating_Entity_Names

Comment 1

[Joshua Longhi]

Hi I am looking to start contributing to Firefox and I already have my development environment set up. I think this would be a good first bug and I would like to fix it!

Comment 2

[Julian Descottes [:jdescottes]]

Hi Joshua, thank you, assigning the bug to you!

I gave a few pointers in the summary, so if your environment is also ready, you should be good to go. (make sure you are using artifact builds, they are so much faster than regular builds!)

If you need any help, feel free to ping me here (with the "need more information" field below) or on irc/slack!

Comment 3

[Joshua Longhi]

Attachment: updated menu entry for collapse, changed to Collapse All

Comment 4

[Joshua Longhi]

I was unsure if the call back in inspector.js needed to update as it is consistent with Expand All

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8914955 [details] [diff] [review]
updated menu entry for collapse, changed to Collapse All

Review of attachment 8914955 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/locales/en-US/inspector.properties
@@ +266,4 @@
>  # LOCALIZATION NOTE (inspectorCollapseNode.label): This is the label
>  # shown in the inspector contextual-menu for recursively collapsing
>  # mark-up elements
> +inspectorCollapseNode.label=Collapse All

As mentioned in the summary, updating localization strings needs to follow some guidelines: https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Updating_Entity_Names

Basically, you need to update the key of the string so that localization teams can know it has to be updated in other languages. So here inspectorCollapseNode.label should become inspectorCollapseNode2.label (or inspectorCollapseAll.label maybe)

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Joshua Longhi from comment #4)
> I was unsure if the call back in inspector.js needed to update as it is
> consistent with Expand All

The callback has to be updated :) The goal of this bug is that clicking the "collapse all" menu item collapses recursively all the subtree below the node. Right now it does collapse instead of collapseAll. Maybe the difference between collapse and collapse all is not clear. Collapse all was introduced in Bug 1401889, so you can read the summary of this other bug.

> as it is consistent with Expand All

You probably saw that the callback for "expandAll" is "this.expandNode()". Now look at how expandNode() is defined (it's in the same file) and see how it differs from the definition of collapseNode(). It is around line 1900-ish. You can keep "this.collapseNode()" as the callback if you want, it is only used here. But the code of the method needs updating.

Comment 7

[John Hawkinson]

Is having a separate Collapse All going to make it difficult to have the same keyboard shortcut bound to Collapse All and Expand All? That is, is the right model that there be a single function whose name in the menu changes depending on the expanded/collapsed state of the context node, and has the same keyboard shortcut?

Again, my reference for this being Firebug that binds * to toggle the state of complete expansion.

Comment 8

[Julian Descottes [:jdescottes]]

(In reply to John Hawkinson from comment #7)
> Is having a separate Collapse All going to make it difficult to have the
> same keyboard shortcut bound to Collapse All and Expand All? That is, is the
> right model that there be a single function whose name in the menu changes
> depending on the expanded/collapsed state of the context node, and has the
> same keyboard shortcut?
> 
> Again, my reference for this being Firebug that binds * to toggle the state
> of complete expansion.

The keyboard shortcuts and the context-menu items are not tightly linked. Shortcuts (such as "H" for hiding a node) are bound in markup.js http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/devtools/client/inspector/markup/markup.js#692

If we were to add a shortcut to toggle expandAll/collapseAll, it probably means modifying the method from markup.js to handle this new shortcut. It could for example call "setNodeExpanded", toggling the current expanded state of the node and applying to descendants. 

From a context menu point of view it makes sense to provide both items, because nodes can be in a state where they can both receive expandAll() or collapseAll(). We could disable expandAll and collapseAll in some cases (node has no children, node is already collapsed etc...) but overall it makes sense to provide two separate menu items here.

The collapseAll() helper was already introduced in Bug 1401889, this bug is just about surfacing it in the context menu. To discuss about a keyboard shortcut, let's move this conversation to a dedicated bug.

Comment 9

[Joshua Longhi]

(In reply to Julian Descottes [:jdescottes] from comment #6)
> (In reply to Joshua Longhi from comment #4)
> > I was unsure if the call back in inspector.js needed to update as it is
> > consistent with Expand All
> 
> The callback has to be updated :) The goal of this bug is that clicking the
> "collapse all" menu item collapses recursively all the subtree below the
> node. Right now it does collapse instead of collapseAll. Maybe the
> difference between collapse and collapse all is not clear. Collapse all was
> introduced in Bug 1401889, so you can read the summary of this other bug.
> 
> > as it is consistent with Expand All
> 
> You probably saw that the callback for "expandAll" is "this.expandNode()".
> Now look at how expandNode() is defined (it's in the same file) and see how
> it differs from the definition of collapseNode(). It is around line
> 1900-ish. You can keep "this.collapseNode()" as the callback if you want, it
> is only used here. But the code of the method needs updating.

Thanks for the guidance! I will get on implementing these changes.

Comment 10

[Joshua Longhi]

Attachment: Changed menu item Collapse to Collapse All

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8915368 [details] [diff] [review]
Changed menu item Collapse to Collapse All

Review of attachment 8915368 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the updated patch Joshua, we are almost ready to land this fix! 
Your patch works fine now, there is just a small comment to update in the localization file.

In the meantime I submitted your patch on try, which is our continuous integration platform. The DevTools test suite will run with your patch to check we didn't introduce any unexpected regression. You can follow the test execution at https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f1a3d23a894673183c4c10be9ec1074d894b587

(for info when your patch is ready to be reviewed, you can add a "review ?" flag on the attachment and assign the review to someone)

::: devtools/client/locales/en-US/inspector.properties
@@ +266,1 @@
>  # LOCALIZATION NOTE (inspectorCollapseNode.label): This is the label

The localization note needs to be in sync with the key:

  # LOCALIZATION NOTE (inspectorCollapseNode.label)

should become

  # LOCALIZATION NOTE (inspectorCollapseAll.label)

Comment 12

[Joshua Longhi]

(In reply to Julian Descottes [:jdescottes] from comment #11)
> Comment on attachment 8915368 [details] [diff] [review]
> Changed menu item Collapse to Collapse All
> 
> Review of attachment 8915368 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the updated patch Joshua, we are almost ready to land this fix! 
> Your patch works fine now, there is just a small comment to update in the
> localization file.
> 
> In the meantime I submitted your patch on try, which is our continuous
> integration platform. The DevTools test suite will run with your patch to
> check we didn't introduce any unexpected regression. You can follow the test
> execution at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3f1a3d23a894673183c4c10be9ec1074d894b587
> 
> (for info when your patch is ready to be reviewed, you can add a "review ?"
> flag on the attachment and assign the review to someone)
> 
> ::: devtools/client/locales/en-US/inspector.properties
> @@ +266,1 @@
> >  # LOCALIZATION NOTE (inspectorCollapseNode.label): This is the label
> 
> The localization note needs to be in sync with the key:
> 
>   # LOCALIZATION NOTE (inspectorCollapseNode.label)
> 
> should become
> 
>   # LOCALIZATION NOTE (inspectorCollapseAll.label)

That's great, thanks for all the help and patience! Is it OK to submit that as a separate commit?

Comment 13

[Julian Descottes [:jdescottes]]

(In reply to Joshua Longhi from comment #12)
> (In reply to Julian Descottes [:jdescottes] from comment #11)
> > Comment on attachment 8915368 [details] [diff] [review]
> > Changed menu item Collapse to Collapse All
> > 
> > Review of attachment 8915368 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks for the updated patch Joshua, we are almost ready to land this fix! 
> > Your patch works fine now, there is just a small comment to update in the
> > localization file.
> > 
> > In the meantime I submitted your patch on try, which is our continuous
> > integration platform. The DevTools test suite will run with your patch to
> > check we didn't introduce any unexpected regression. You can follow the test
> > execution at
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=3f1a3d23a894673183c4c10be9ec1074d894b587
> > 
> > (for info when your patch is ready to be reviewed, you can add a "review ?"
> > flag on the attachment and assign the review to someone)
> > 
> > ::: devtools/client/locales/en-US/inspector.properties
> > @@ +266,1 @@
> > >  # LOCALIZATION NOTE (inspectorCollapseNode.label): This is the label
> > 
> > The localization note needs to be in sync with the key:
> > 
> >   # LOCALIZATION NOTE (inspectorCollapseNode.label)
> > 
> > should become
> > 
> >   # LOCALIZATION NOTE (inspectorCollapseAll.label)
> 
> That's great, thanks for all the help and patience! Is it OK to submit that
> as a separate commit?

It will be cleaner to do it in the same commit. If you are using mercurial hg can use `hg commit --amend` to add some changes to your current commit. Let me know if it gives you any trouble, I can also do this change for you since it's your first patch.

Comment 14

[Joshua Longhi]

Attachment: Changed menu item Collapse to Collapse All

Comment 15

[Joshua Longhi]

(In reply to Julian Descottes [:jdescottes] from comment #13)
> (In reply to Joshua Longhi from comment #12)
> > (In reply to Julian Descottes [:jdescottes] from comment #11)
> > > Comment on attachment 8915368 [details] [diff] [review]
> > > Changed menu item Collapse to Collapse All
> > > 
> > > Review of attachment 8915368 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Thanks for the updated patch Joshua, we are almost ready to land this fix! 
> > > Your patch works fine now, there is just a small comment to update in the
> > > localization file.
> > > 
> > > In the meantime I submitted your patch on try, which is our continuous
> > > integration platform. The DevTools test suite will run with your patch to
> > > check we didn't introduce any unexpected regression. You can follow the test
> > > execution at
> > > https://treeherder.mozilla.org/#/
> > > jobs?repo=try&revision=3f1a3d23a894673183c4c10be9ec1074d894b587
> > > 
> > > (for info when your patch is ready to be reviewed, you can add a "review ?"
> > > flag on the attachment and assign the review to someone)
> > > 
> > > ::: devtools/client/locales/en-US/inspector.properties
> > > @@ +266,1 @@
> > > >  # LOCALIZATION NOTE (inspectorCollapseNode.label): This is the label
> > > 
> > > The localization note needs to be in sync with the key:
> > > 
> > >   # LOCALIZATION NOTE (inspectorCollapseNode.label)
> > > 
> > > should become
> > > 
> > >   # LOCALIZATION NOTE (inspectorCollapseAll.label)
> > 
> > That's great, thanks for all the help and patience! Is it OK to submit that
> > as a separate commit?
> 
> It will be cleaner to do it in the same commit. If you are using mercurial
> hg can use `hg commit --amend` to add some changes to your current commit.
> Let me know if it gives you any trouble, I can also do this change for you
> since it's your first patch.

Thanks, I think I got it!

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8915567 [details] [diff] [review]
Changed menu item Collapse to Collapse All

Perfect! Tests are green, I will now add the keyword checkin-needed and your patch will soon land on our integration platforms. After that it should be available in Nightly very soon and will be released with Firefox 58.

Thanks a lot for your contribution Joshua!

Comment 17

[Joshua Longhi]

Awesome, really appreciate the help!

Comment 18

[Joshua Longhi]

Do I need to push my changes or will you be doing that?

Comment 19

[Julian Descottes [:jdescottes]]

(In reply to Joshua Longhi from comment #18)
> Do I need to push my changes or will you be doing that?

Sheriffs will spot the checkin-needed keyword and will take care of landing the patch.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd010e955f7
Changed menu item Collapse to Collapse All. r=jdescottes

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ebd010e955f7