Closed Bug 431817 Opened 14 years ago Closed 14 years ago

Special folders (Tags, Recent Tags) are not expandable/collapsible (no-more containers)

Categories

(Firefox :: Bookmarks & History, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: whimboo, Assigned: mak)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Attached image Missing drop markers
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050104 Minefield/3.0pre ID:2008050104

Within the Library the special folders Tags, Recently Bookmarked, and Recent Tags doesn't have a drop marker anymore. You wont be able to expand or collapse its content. Instead their names are underlined when hovering over. See the attachment how it looks like.

Its a recent regression. Will try to find the regression window.
Flags: blocking-firefox3?
could be bug 430659?
Regression started between the builds 080430_2138 and 080430_2205. The only checkin within this timeframe is bug 430659. It also happens under Windows.
Depends on: 430659
OS: Mac OS X → All
Hardware: PC → All
Summary: Special folders (Tags, Recently Bookmarked, Recent Tags) are not expandable/collapsible due to missing drop marker → Special folders (Tags, Recent Tags) are not expandable/collapsible due to missing drop marker
It looks like that these both folders aren't handled as folders anymore. It not only happens due to the missing drop marker.
Summary: Special folders (Tags, Recent Tags) are not expandable/collapsible due to missing drop marker → Special folders (Tags, Recent Tags) are not expandable/collapsible
indeed they are considered bookmarks because the left pane folder has expandqueries=0... but i'm talking about Tags... other queries should be correct...
Attached patch patch (obsolete) — Splinter Review
there are 2 possible fix for this problem:
- the former is changing the left pane folder to not use expandQueries=0, but that will affect also History.
- the latter (that i attach) is always expand queries containing queries

this will fix make Tags and Recent Tags to be considered as containers (with expand glyph)
Attachment #318986 - Flags: review?(dietrich)
Blocks: 424286
this blocks bug 424286 since if Tags is not a container, it is clearly not styled as a container.
Summary: Special folders (Tags, Recent Tags) are not expandable/collapsible → Special folders (Tags, Recent Tags) are not expandable/collapsible (no-more containers)
(In reply to comment #6)
> this blocks bug 424286 since if Tags is not a container, it is clearly not
> styled as a container.

See my attached screen shot. For me they are still shown as containers.
(In reply to comment #7)
> (In reply to comment #6
> See my attached screen shot. For me they are still shown as containers.

Tags should have the tag icon, it is styled like a generic query in the sshot, i mean that the treeview does not recognize it as a container and the style is applied accordingly (so the tag icon that is for a container is not applied)
Marco: cannot we use hasChildren instead?
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P1
Target Milestone: --- → Firefox 3
From Aronnax, via email:

>and i have just found in the default theme places/organizer.css
>
>#placesList treechildren::-moz-tree-cell(leaf) ,
>#placesList treechildren::-moz-tree-image(leaf) {
>  cursor: pointer;
>}
>
>#placesList treechildren::-moz-tree-cell-text(leaf, hover) {
>  cursor: pointer;
>  text-decoration: underline;
>}
>
>the  cursor: pointer and   text-decoration: underline are naturally simply wrong
>and at the same place is now  the tree-twisty arrow not visible at the >Library/Tags folder

Seems relevant! :)
Assignee: nobody → mak77
(In reply to comment #9)
> Marco: cannot we use hasChildren instead?
> 

i'd prefer this over the overly-special-case in the current patch.
Attached patch patch (obsolete) — Splinter Review
for sure
Attachment #318986 - Attachment is obsolete: true
Attachment #319149 - Flags: review?(dietrich)
Attachment #318986 - Flags: review?(dietrich)
Mano, I'm sure Dietrich wouldn't mind if you took this review from him :)
Whiteboard: [has patch][needs review dietrich]
Comment on attachment 319149 [details] [diff] [review]
patch

>Index: browser/components/places/content/treeView.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/treeView.js,v
>retrieving revision 1.69
>diff -u -8 -p -r1.69 treeView.js
>--- browser/components/places/content/treeView.js	1 May 2008 15:41:37 -0000	1.69
>+++ browser/components/places/content/treeView.js	3 May 2008 10:31:15 -0000
>@@ -937,18 +937,19 @@ PlacesTreeView.prototype = {
>     if (PlacesUtils.nodeIsContainer(node)) {
>       // the root node is always expandable
>       if (!node.parent)
>         return true;
>
>       // treat non-expandable queries as non-containers

this comment should be updated to "non-expandable childless queries"

>       if (PlacesUtils.nodeIsQuery(node)) {
>         var parent = node.parent;
>-        if(PlacesUtils.nodeIsQuery(parent) ||
>-           PlacesUtils.nodeIsFolder(parent))
>+        if((PlacesUtils.nodeIsQuery(parent) ||
>+            PlacesUtils.nodeIsFolder(parent)) &&
>+           !node.hasChildren)
>           return asQuery(parent).queryOptions.expandQueries;
>       }

hrm, i don't have a clear view of what effect this will have on use-cases outside of Fx's internal ones. we need to take this for now, but i think that these special-cases in the bindings, where we override options specified in the query, are the cause of many a regression.
Attachment #319149 - Flags: review?(dietrich) → review+
Whiteboard: [has patch][needs review dietrich] → [has patch][has review][has approval]
Attached patch for check-inSplinter Review
Status: NEW → ASSIGNED
Keywords: checkin-needed
Attachment #319149 - Attachment is obsolete: true
Duplicate of this bug: 431897
Checking in browser/components/places/content/treeView.js;
/cvsroot/mozilla/browser/components/places/content/treeView.js,v  <--  treeView.js
new revision: 1.70; previous revision: 1.69
done
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has approval]
Verified with:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050606 Minefield/3.0pre ID:2008050606

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050619 Minefield/3.0pre
Status: RESOLVED → VERIFIED
Flags: in-litmus?
not going to add a litmus testcase for this specifically. But will add this check to test cases related to tag containers.
Flags: in-litmus? → in-litmus+
Flags: in-litmus+ → in-litmus-
Blocks: 433525
No longer blocks: 433525
Depends on: 433525
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.