Closed
Bug 1066558
Opened 10 years ago
Closed 10 years ago
Cant collapse a tree in web IDE, after expanding it
Categories
(DevTools Graveyard :: WebIDE, defect)
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)
87.64 KB,
image/png
|
Details | |
3.02 KB,
patch
|
manu.jain13
:
review+
|
Details | Diff | Splinter Review |
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.
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)
Comment 2•10 years ago
|
||
(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]
Comment 3•10 years ago
|
||
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. :)
Comment 4•10 years ago
|
||
(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.
Patch attached.
Attachment #8492511 -
Flags: review?(bgrinstead)
Comment 6•10 years ago
|
||
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)
Updated•10 years ago
|
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!!
Comment 8•10 years ago
|
||
(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.");
New function added for toggle expansion. Tests also created and passed successfully.
Attachment #8492511 -
Attachment is obsolete: true
Attachment #8500909 -
Flags: review?(bgrinstead)
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
Changes made.
Attachment #8500909 -
Attachment is obsolete: true
Attachment #8501070 -
Flags: review?(bgrinstead)
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
Added "if" statement.
Attachment #8501070 -
Attachment is obsolete: true
Attachment #8501108 -
Flags: review?(bgrinstead)
Comment 14•10 years ago
|
||
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+
Assignee | ||
Comment 15•10 years ago
|
||
formatting done.
Attachment #8501108 -
Attachment is obsolete: true
Attachment #8501115 -
Flags: review+
Comment 16•10 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1829e10b3a4b
Comment 17•10 years ago
|
||
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 { }
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8501115 -
Attachment is obsolete: true
Attachment #8501137 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
All the tests passed. :)
Updated•10 years ago
|
Keywords: checkin-needed
Comment 20•10 years ago
|
||
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
Assignee | ||
Comment 21•10 years ago
|
||
Sorry for the mistake. Now solved.
Attachment #8501137 -
Attachment is obsolete: true
Attachment #8501632 -
Flags: review+
Updated•10 years ago
|
Keywords: checkin-needed
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: 10 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][fixed-in-fx-team] → [good first bug][lang=js]
Target Milestone: --- → Firefox 35
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•4 years ago
|
Product: DevTools → DevTools Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•