54.79 KB, image/png
16.91 KB, patch
Scott MacGregor: review+
|Details | Diff | Splinter Review|
17.37 KB, patch
|Details | Diff | Splinter Review|
714 bytes, patch
Scott MacGregor: review+
Scott MacGregor: approval-branch-1.8.1+
|Details | Diff | Splinter Review|
implement Mail.app / 4.x Mac communicator like versions of the "move/copy" menus sfraser will provide screen shot of 4.x mac and Mail.app I've seen how it looks, and it would allow us to remove the "File Here" / "Choose This Folder" thing we do. jglick, comments?
Using hierarchical menus in popup menus makes them very hard to use, and forces you to have hacks like the 'Choose this folder' item at the top of each submenu. Things are much clearaer, and easier to use, if you flatten the entire accounts/mailbox list into one menu, using indentation to indicate containment.
Created attachment 105085 [details] Screenshot: how Mail.app on OS X does this Both Mail.app and 4.x used flat folder menus.
Sounds good to me.
reviving this bug. I think this would be a big usability win at hopefully a pretty low cost. cc'ing some possible volunteers who might be interested in trying to do this for menu items that contain the user's account / folder hierarchies.
I've got this working for the advanced message search window.
Created attachment 152307 [details] [diff] [review] More progress Fixes: * The icons now work * The dotted lines are now hidden * Removed some event handlers by changing treechildren binding * Clicking on the empty space at the bottom no longer throws an exception Issues: * Mousewheeling crashes in layout code [unchecked null pointer] * Clicking on scrollbarbuttons generates unwanted command events [other mouse events are already blocked, but command got overlooked] * Clicking in the tree generates unwanted focus events [I need to investigate why this happens]
I've been playing around with this latest patch too. Is the fact that selection changes as you move the scrollbar up and down because of the issue you cited where scrollbarbuttons generates unwanted command events? sounds like it probably is. This may not be easy to do, but it is a bit weird to see all of the various folder states effect this menu (i.e. if a server or folder has new mail I see a biff indicator in the menu, if a folder has unread messages it is in bold, etc).
(In reply to comment #8) >I've been playing around with this latest patch too. Is the fact that >selection changes as you move the scrollbar up and down because of the issue >you cited where scrollbarbuttons generates unwanted command events? sounds >like it probably is. Hmm... moving the scrollbar thumb shouldn't be an issue... >This may not be easy to do, but it is a bit weird to see all of the various >folder states effect this menu (i.e. if a server or folder has new mail I see >a biff indicator in the menu, if a folder has unread messages it is in bold, >etc). My patch was based on bug 21344, which does need all of the folder states. I then looked at what Outlook does and it in fact shows the bold and unread count. Presumably it uses the same widget thoughout. Here however we have a bit of flexibility, in that we can create slightly different bindings by changing the properties applied to each cell, in this case by removing biffState, newMessages (mistyped as mewMessages anyway!) and hasUnreadMessages.
I think I can fix the mousewheel issue: <handler event="DOMMouseScroll" preventdefault="true"/>
(In reply to comment #10) > I think I can fix the mousewheel issue: > <handler event="DOMMouseScroll" preventdefault="true"/> Yes, I tested the "More progress" patch with this additional line and now the mousewheel crasher is gone. :-)
yeah that fixed the crash for me too. Are either of you seeing the problem where moving the scroll bar up and down causes the highlight in the menu popup to move up and down as well? i.e. if i drag the scroll bar up and down, the selection matches the position of my cursor even though the cursor is over the scrollbar and not in the menu.
Actually I can still trigger a crash using the scroll wheel if the cursor is outside of the menu and the scroll bar. i.e. if the widget has focus, but i move my mouse to the right of the scrollbars so it's not inside the widget or the scrollbars, then use the scroll wheel, I crash: nsMenuFrame::GetScrollableView with aView being null.
Since I'm spamming the bug, I thought of another requirement for this widget. In some cases (like the search window), the account should be selectable like it is now. i.e. you can search on Local Folders or on a specific account. In other places where we would want to use this widget, an account is not a valid selection. You need to select a folder. For instance, if we used this widget for the menu popup underneath the File toolbar button. Or the "Other" menu list for an account for selecting a Templates/Sent/Drafts folder. Can we add an attribute to the widget which specifies if a server is selectable then leverage the noSelect-true cell property?
I believe that the noselect flag is actually used to indicate folders that cannot contain messages. What I thought was in that case of selecting an account to store drafts it could automatically create the default folder, although that doesn't help in the case of the File button; alternatively you could just ignore clicks on accounts although I can see how it might be neater to make it so that accounts don't even highlight when you mouse over them.
>* Mousewheeling crashes in layout code Still trying to understand scrolling code on this one. >* Clicking on scrollbarbuttons generates unwanted command events Fixed in another bug. >* Clicking in the tree generates unwanted focus events This is caused by the -moz-user-focus: style for <treechildren> being none, rather than ignore. If I can persuade bryner that ignore is correct for all unfocusable XUL elements, then the problem will go away. In the meantime, adding -moz-user-focus: ignore; to the .foldersTreeChildren rule will make the <handler event="mousedown" preventdefault="true"> unnecessary. >* Hovering scrollbar moves the selection I think this is actually caused by hovering the border.
Do you need any help resolving some of these remaining open issues Neil?
(In reply to comment #16) >>* Mousewheeling crashes in layout code >Still trying to understand scrolling code on this one. I think rather than trying to catch bryner on irc I should just email him. >>* Clicking in the tree generates unwanted focus events >This is caused by the -moz-user-focus: style for <treechildren> being none, >rather than ignore. If I can persuade bryner that ignore is correct for all I filed a bug on this, but no review yet. >>* Hovering scrollbar moves the selection >I think this is actually caused by hovering the border. Easiest fix is to do an if (event.target != this) check in the handler.
Created attachment 153930 [details] [diff] [review] Further progress The only remaining issue is the -moz-user-focus: ignore; on the <treechildren> which should be in xul.css as per bug 251465.
Created attachment 154442 [details] [diff] [review] Ready This is now ready to replace the advanced search folder picker :-)
Neil, I still see the selection box move with the position of my mouse as I move the scrollbar thumb up and down the scroll box frame. Do you think that's a bug or it should do that?
I recently re-discovered this bug again and tried Neil's latest patch. And my problem with selection moving with the scroll bars is now gone, things are working really well. I'll attach an updated patch against the trunk.
Created attachment 186521 [details] [diff] [review] updated patch to the trunk including changes for thunderbird Neil, this is still your patch just updated to the trunk. Asking for an r again just to confirm that you are still happy with the work that you did.
Comment on attachment 186521 [details] [diff] [review] updated patch to the trunk including changes for thunderbird >+ <rule nc:CanSearchMessages="false"/> >+ <rule> These rules are no longer correct due to virtual folders. In my tree I collapsed them into the following rule: <rule nc:CanSearchMessages="true" nc:Virtual="false"> Somebody pointed out that I only provided basic keyboard navigation, there's no typeaheadfind feature. Maybe I can implement that at a later date.
(In reply to comment #24) >there's no typeaheadfind feature. By which I mean locating a folder by typing a unique prefix of its name.
Created attachment 186629 [details] [diff] [review] updated patch Updated patch with Neil's review comment. I've verified that virtual folders don't show up in the search dialog menu list now. Good catch. Moving Neil's r forward.
Ok, this patch has been checked in! Woo hoo. I'm going to close this bug out. Once the dust settles we can convert more folder picker widgets to use this new widget. Right now the only consumer is the search dialog folder picker.
It's a very nice view of my folders, but if you have a lot accounts with subfolders, than it could be a little bit complex. It would be easier if you use the default folder icons in this new folder picker. So you will find your junk or trash folder very fast.
Created attachment 187025 [details] [diff] [review] Fix icons for thunderbird Unfortunately Thunderbird's icon CSS is in a different place. This is a quick hack to make the icons show up for Thunderbird too. Or I could move the icons back into the equivalent of the file that the Suite still uses.
Comment on attachment 187025 [details] [diff] [review] Fix icons for thunderbird Scott's going to look at this and see why it makes a difference...
Comment on attachment 187025 [details] [diff] [review] Fix icons for thunderbird Neil, I see that this change works by testing it, but I don't understand why it works. When I look at qute's folderMenus.css file and compare it to classic's (the suite) folderMenus.css file, they have the same rules (but with different images of course). They both have .folderMenuItem style rules for the various types of folders.
(In reply to comment #31) >They both have .folderMenuItem style rules for the various types of folders. Except that the new widget uses a different set of style rules...
Comment on attachment 187025 [details] [diff] [review] Fix icons for thunderbird I'm going to move the folder pane tree children rules back to folderPane.css instead for thunderbird (this is not an issue for seamonkey)
Created attachment 191633 [details] [diff] [review] [patch checked in] move shared folder tree rules into folderPane.css Moves shared folder pane tree children rules from mailWindow1.css to folderPane.css so the new folder picker widget can pick them up.
Created attachment 219277 [details] [diff] [review] Page Down fix I can't believe we missed this all along.