Closed Bug 1066558 Opened 5 years ago Closed 5 years ago

Cant collapse a tree in web IDE, after expanding it

Categories

(DevTools :: WebIDE, defect)

34 Branch
x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: r.gautamkrishna, Assigned: manu.jain13, Mentored)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 files, 6 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20140910004000

Steps to reproduce:

open web IDE
open a project
double click on any folder with files, it expands showing the files in it..
You cant collapse the tree back by simply double clicking...


Actual results:

You cant collapse the trees back. That means if you have a large project with a number of files in it. Then you need a lots of effort to complete your work.


Expected results:

As a principle of normal UI designing. An expanded tree must be able to collapsed by simply double clicking or via single click..
If this problem is not fixed, the developers may not be able to work on large projects including use of large number of files and sub-directiories.
Component: Untriaged → Developer Tools: WebIDE
bgrins, maybe this is a good first bug?  I am guessing it's a simple fix.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(bgrinstead)
(In reply to J. Ryan Stinnett [:jryans] from comment #1)
> bgrins, maybe this is a good first bug?  I am guessing it's a simple fix.

Yes, it should just be a matter of refactoring the handler for click event on this.line to match the handler for this.expander right here: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/projecteditor/lib/tree.js#75.

* Make a new function called toggleExpansion or similar with this logic then bind that function as a callback for both of those handlers.
* Update browser_projecteditor_tree_selection_01.js in the isDir case to check that expansion is toggled with container.line.click() and container.expander.click(): http://dxr.mozilla.org/mozilla-central/source/browser/devtools/projecteditor/test/browser_projecteditor_tree_selection_01.js#44.
Mentor: bgrinstead
Flags: needinfo?(bgrinstead)
Whiteboard: [good first bug][lang=js]
Please care to explain the bug and how do I see what's wrong [i.e steps to reproduce the bug].
Also, I am a beginner, assign this bug to me. :)
(In reply to Ayush Mishra from comment #3)
> Please care to explain the bug and how do I see what's wrong [i.e steps to
> reproduce the bug].
> Also, I am a beginner, assign this bug to me. :)

Hello, see Comment 0 for a description of the steps to see the problem.  You can open the web ide and a project by following instructions here: https://developer.mozilla.org/en-US/docs/Tools/WebIDE

Comment 2 pretty much explains what needs to be done to fix it.  If you have not yet built Firefox, then follow the First Build and First Run instructions here: https://wiki.mozilla.org/DevTools/Hacking and let me know when you are ready to start working on it.
Attached patch bug1066558.patch (obsolete) — Splinter Review
Patch attached.
Attachment #8492511 - Flags: review?(bgrinstead)
Comment on attachment 8492511 [details] [diff] [review]
bug1066558.patch

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

Please see Comment 2 with further instructions for how to tackle this patch.  There will need to be a new function created in tree.js and a modification to an existing test, which can be tested with `mach mochitest-devtools browser/devtools/projecteditor/test/browser_projecteditor_tree_selection_01.js`.
Attachment #8492511 - Flags: review?(bgrinstead)
Assignee: nobody → manu.jain13
Status: NEW → ASSIGNED
I've made separate function for toggle expansion. Can you tell how to update tests for checking whether the expansion is toggled or not? Thanks!!
(In reply to Manu Jain from comment #7)
> I've made separate function for toggle expansion. Can you tell how to update
> tests for checking whether the expansion is toggled or not? Thanks!!

You can modify browser_projecteditor_tree_selection_01.js inside the isDir condition here: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/projecteditor/test/browser_projecteditor_tree_selection_01.js#44.  Call container.expander.click() (or whatever you've called this function) then check that container.expander has been toggled. So the condition would change to be something like this:

ok (!container.expanded, "A directory is not expanded by default.");
container.expander.click()
ok (container.expanded, "Clicking on the expander toggles expansion.");
container.expander.click()
ok (!container.expanded, "Clicking on the expander toggles expansion.");
Attached patch bug_1066558_v2.patch (obsolete) — Splinter Review
New function added for toggle expansion. Tests also created and passed successfully.
Attachment #8492511 - Attachment is obsolete: true
Attachment #8500909 - Flags: review?(bgrinstead)
Comment on attachment 8500909 [details] [diff] [review]
bug_1066558_v2.patch

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

Looking good - just a couple of small updates needed before landing it

::: browser/devtools/projecteditor/lib/tree.js
@@ +76,1 @@
>        if (!this.selected) {

You can remove this if (!this.selected) condition here - it won't hurt to call this.select() unconditionally.

::: browser/devtools/projecteditor/test/browser_projecteditor_tree_selection_01.js
@@ +43,5 @@
>    }
>    if (resource.isDir) {
>      ok (!container.expanded, "A directory is not expanded by default.");
> +    container.toggleExpansion();
> +    ok(container.expanded, "Clicking on the expander toggles expansion.");

This message isn't actually what this is testing (it isn't actually clicking on the expander).  You should update this test to use:

`container.line.click()` instead of `container.toggleExpansion()`
Attachment #8500909 - Flags: review?(bgrinstead)
Attached patch bug_1066558_v3.patch (obsolete) — Splinter Review
Changes made.
Attachment #8500909 - Attachment is obsolete: true
Attachment #8501070 - Flags: review?(bgrinstead)
Comment on attachment 8501070 [details] [diff] [review]
bug_1066558_v3.patch

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

Ah I just thought of a case that would be a problem when testing it: clicking on the 'root' element (the project header object in the tree) will collapse all the content below it, but it should never be collapsible.  We need to make sure `!this.resource.isRoot` before setting expanded = false in toggleExpansion.  And update the test to make sure that this doesn't happen.

::: browser/devtools/projecteditor/test/browser_projecteditor_tree_selection_01.js
@@ +38,4 @@
>    let container = projecteditor.projectTree.getViewContainer(resource);
>  
>    if (resource.isRoot) {
>      ok (container.expanded, "The root directory is expanded by default.");

We should add a check to container.line.click(); here and make sure that it does not get collapsed.
Attachment #8501070 - Flags: review?(bgrinstead)
Attached patch bug_1066558_v4.patch (obsolete) — Splinter Review
Added "if" statement.
Attachment #8501070 - Attachment is obsolete: true
Attachment #8501108 - Flags: review?(bgrinstead)
Comment on attachment 8501108 [details] [diff] [review]
bug_1066558_v4.patch

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

Looks good, just a formatting note.  Feel free to reupload after making that change with r+

::: browser/devtools/projecteditor/lib/tree.js
@@ +88,5 @@
>      this.update();
>    },
>  
> +  toggleExpansion: function() {
> +    if(!this.resource.isRoot)

Nit: please add a space after the if and use brackets for the if / else to match the rest of the file
Attachment #8501108 - Flags: review?(bgrinstead) → review+
Attached patch bug_1066558_v4.patch (obsolete) — Splinter Review
formatting done.
Attachment #8501108 - Attachment is obsolete: true
Attachment #8501115 - Flags: review+
Comment on attachment 8501115 [details] [diff] [review]
bug_1066558_v4.patch

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

::: browser/devtools/projecteditor/lib/tree.js
@@ +91,5 @@
> +  toggleExpansion: function() {
> +    if (!this.resource.isRoot) {
> +      this.expanded = !this.expanded;
> +    }
> +    else {

Sorry about this!  But can you do:

if (foo) {

} else {

}
Attached patch bug_1066558_v4.patch (obsolete) — Splinter Review
Attachment #8501115 - Attachment is obsolete: true
Attachment #8501137 - Flags: review+
All the tests passed. :)
Hi, seems the patch failed to apply:
applying bug_1066558_v4.patch
patch failed, unable to continue (try -v)

could you look at this issue and maybe rebase against a current tree, thanks!
Keywords: checkin-needed
Sorry for the mistake. Now solved.
Attachment #8501137 - Attachment is obsolete: true
Attachment #8501632 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/628001a7e7a7
Keywords: checkin-needed
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/628001a7e7a7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 35
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.