Last Comment Bug 813218 - Folder Picker in Toolbar is _waaaayyyy_ too condensed.
: Folder Picker in Toolbar is _waaaayyyy_ too condensed.
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: 18 Branch
: All All
: -- normal (vote)
: Thunderbird 21.0
Assigned To: :aceman
Depends on: 814041 859287
Blocks: 815220
  Show dependency treegraph
Reported: 2012-11-19 10:06 PST by Blake Winton (:bwinton) (:☕️)
Modified: 2013-04-10 11:34 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch (4.63 KB, patch)
2012-11-19 12:51 PST, :aceman
bwinton: ui‑review+
Details | Diff | Splinter Review
patch v2 (6.46 KB, patch)
2012-11-19 15:20 PST, :aceman
bwinton: ui‑review+
neil: feedback+
Details | Diff | Splinter Review
patch v3 (6.70 KB, patch)
2012-12-03 10:55 PST, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review
finish of the backout (853 bytes, patch)
2013-01-04 13:01 PST, :aceman
mconley: review+
Details | Diff | Splinter Review
patch v4 (9.40 KB, patch)
2013-01-04 15:01 PST, :aceman
mkmelin+mozilla: review+
Details | Diff | Splinter Review

Description Blake Winton (:bwinton) (:☕️) 2012-11-19 10:06:13 PST
Aceman, since you're dabbling in this code already, could you take a look at this one, and make it look like all the other folder pickers?  :)

Comment 1 :aceman 2012-11-19 11:45:23 PST
Which one? Screenshot?
Comment 2 Blake Winton (:bwinton) (:☕️) 2012-11-19 12:07:39 PST
Comment 3 :aceman 2012-11-19 12:51:43 PST
Created attachment 683273 [details] [diff] [review]

This removes the special folderLocationPopup class that removes padding and uses the menulist-menupopup class that is used on picker e.g. in the filter list dialog. The folderLocationPopup class (NOT id) is used here:^[^\0]*%24&hitlimit=&tree=comm-central

It is not used in Seamonkey but I also fix the one usage in RSS, so please try it out too. Open the subscription dialog and select a folder of an already subscribed feed. The picker will appear in the last line of the dialog.

The patch does not seem to have any visual effect on Linux. Please try on Mac and Win.
Comment 4 Blake Winton (:bwinton) (:☕️) 2012-11-19 14:10:40 PST
Comment on attachment 683273 [details] [diff] [review]

Way better!  The only remaining problem is that in customize mode, it gets really small, like so:

Perhaps we can set a min-width to avoid that?

Comment 5 :aceman 2012-11-19 15:20:12 PST
Created attachment 683333 [details] [diff] [review]
patch v2

So we intentionally shrink the menulist to nothing (remove all children) when it is in a wrapper (i.e. in the customize mode). There is a comment in folderWidgets.xml about it.

So this patch sets it to a sane width=100 when in customize mode and then restore to the previous width when out of customize mode. Do we want to do this?
Comment 6 2012-11-20 08:17:48 PST
Comment on attachment 683333 [details] [diff] [review]
patch v2

Not sure what feedback you want here; SeaMonkey has a different folder picker in its toolbar. The feed-subscriptions.xul change looks fine though, although I've only just noticed that bug 355789 regressed the menulist-menupopup class :-(
Comment 7 :aceman 2012-11-20 08:25:58 PST
Neil, if you see any other place where we would use this picker AND it could get into a wrapper AND we would want it to shrink to almost nothing as it was before the patch.
Comment 8 Blake Winton (:bwinton) (:☕️) 2012-11-26 09:23:34 PST
Comment on attachment 683333 [details] [diff] [review]
patch v2

Seems better, but we've lost the icons somehow…
(We've also lost the icons in the other folder pickers on Mac.  It's fine on Windows, though.)

Comment 9 :aceman 2012-11-26 09:59:19 PST
I don't see why this would happen on Mac only (it is also fine on Linux). The change is the same to all themes.

I suspect that was caused by bug 795989 where they removed the icon files used by folderMenus.css. Notice they only updated folderPane.css.
Comment 10 Mike Conley (:mconley) 2012-11-26 10:16:18 PST
(In reply to :aceman from comment #9)
> I suspect that was caused by bug 795989 where they removed the icon files
> used by folderMenus.css. Notice they only updated folderPane.css.

Ah, yikes. :/ Slipped by my radar.

I've filed bug 815220.
Comment 11 Richard Marti (:Paenglab) 2012-12-02 02:29:16 PST

Could you please check this patch together with the patch from bug 815220? Maybe you could then revise your ui-r-
Comment 12 Richard Marti (:Paenglab) 2012-12-02 12:01:22 PST
Blake, has this patch and the one from bug 815220 applied. With this it should be easier for the ui-r.
Comment 13 Blake Winton (:bwinton) (:☕️) 2012-12-03 07:14:05 PST
Comment on attachment 683333 [details] [diff] [review]
patch v2

The try-build is good, so ui-r=me, once bug 815220 has landed.  :)

Thanks, Richard (and aceman)!
Comment 14 :aceman 2012-12-03 08:18:32 PST
bwinton, have you also tried moving the widget between the toolbar and the customization palette?
Comment 15 Blake Winton (:bwinton) (:☕️) 2012-12-03 08:53:38 PST
Comment 16 :aceman 2012-12-03 10:55:53 PST
Created attachment 687864 [details] [diff] [review]
patch v3

Ok, thanks.
Comment 17 Magnus Melin 2012-12-09 03:03:08 PST
Comment on attachment 687864 [details] [diff] [review]
patch v3

Review of attachment 687864 [details] [diff] [review]:

Looks ok to me. r=mkmelin
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-12-10 05:16:28 PST
Comment 19 Magnus Melin 2012-12-10 11:38:08 PST
Backed out due to orange

SUMMARY-UNEXPECTED-FAIL | C:\talos-slave\test\build\mozmill\folder-display\test-folder-toolbar.js | test-folder-toolbar.js::test_add_folder_toolbar
  EXCEPTION: Uninitialized Folder doesn't have a blank label.: 'Folder Location' != ' '.
    at: test-folder-display-helpers.js line 2831
       assert_true test-folder-display-helpers.js 2831
       assert_equals test-folder-display-helpers.js 2818
       test_add_folder_toolbar test-folder-toolbar.js 42
       Runner.prototype.wrapper frame.js 582
       Runner.prototype._runTestModule frame.js 652
       Runner.prototype.runTestModule frame.js 698
       Runner.prototype.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge.prototype._execFunction server.js 179
Comment 20 :aceman 2012-12-10 23:58:48 PST
Yeah, I made this change intentionally:
-                label=" "
+                label="&folderLocationToolbarItem.title;"

I thought it looks better than blank. Should I revert it or do we update the test?
Comment 21 Blake Winton (:bwinton) (:☕️) 2013-01-01 12:18:15 PST
Update the test, definitely, since this was an intentional change.

Comment 22 :aceman 2013-01-04 13:01:31 PST
Created attachment 698046 [details] [diff] [review]
finish of the backout

The backout missed the file mail/themes/pinstripe/mail/folderMenus.css (probably because there were recent changes and the backout failed on that file, unnoticed).
Comment 23 Mike Conley (:mconley) 2013-01-04 13:04:09 PST
Comment on attachment 698046 [details] [diff] [review]
finish of the backout

Review of attachment 698046 [details] [diff] [review]:

Looks good! Great catch, aceman.
Comment 24 Mike Conley (:mconley) 2013-01-04 13:12:11 PST
Landed the rest of the backout:
Comment 25 :aceman 2013-01-04 15:01:23 PST
Created attachment 698105 [details] [diff] [review]
patch v4

This fixes the test.
Comment 26 :aceman 2013-01-04 15:02:16 PST
Comment on attachment 698105 [details] [diff] [review]
patch v4

Adding mconley if he is interested how the fetching of entities got solved ;)
Comment 27 Ryan VanderMeulen [:RyanVM] 2013-01-13 06:42:27 PST
Comment 28 Mike Conley (:mconley) 2013-01-26 12:45:00 PST
Comment on attachment 698105 [details] [diff] [review]
patch v4

Neat technique. :)

Note You need to log in before you can comment on or make changes to this bug.