The default bug view has changed. See this FAQ.

Folder Picker in Toolbar is _waaaayyyy_ too condensed.

RESOLVED FIXED in Thunderbird 21.0

Status

Thunderbird
Toolbars and Tabs
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bwinton, Assigned: aceman)

Tracking

18 Branch
Thunderbird 21.0
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

9.40 KB, patch
Magnus Melin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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?  :)

Thanks,
Blake.
(Assignee)

Comment 1

4 years ago
Which one? Screenshot?
(Reporter)

Comment 2

4 years ago
https://dl.dropbox.com/u/2301433/Screenshots/FolderTiny.png
(Assignee)

Comment 3

4 years ago
Created attachment 683273 [details] [diff] [review]
patch

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:
http://mxr.mozilla.org/comm-central/search?string=folderLocationPopup&find=&findi=&filter=^[^\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.
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #683273 - Flags: ui-review?(bwinton)
(Reporter)

Comment 4

4 years ago
Comment on attachment 683273 [details] [diff] [review]
patch

Way better!  The only remaining problem is that in customize mode, it gets really small, like so: https://dl.dropbox.com/u/2301433/Screenshots/SmallFolderCustomize.png

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

Thanks,
Blake.
Attachment #683273 - Flags: ui-review?(bwinton) → ui-review+
(Assignee)

Comment 5

4 years ago
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?
Attachment #683273 - Attachment is obsolete: true
Attachment #683333 - Flags: ui-review?(bwinton)
Attachment #683333 - Flags: feedback?(neil)

Comment 6

4 years ago
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 :-(
Attachment #683333 - Flags: feedback?(neil) → feedback+
(Assignee)

Comment 7

4 years ago
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.
(Reporter)

Comment 8

4 years ago
Comment on attachment 683333 [details] [diff] [review]
patch v2

Seems better, but we've lost the icons somehow…

https://dl.dropbox.com/u/2301433/Screenshots/MissingIcons.png
(We've also lost the icons in the other folder pickers on Mac.  It's fine on Windows, though.)

Thanks,
Blake.
Attachment #683333 - Flags: ui-review?(bwinton) → ui-review-
(Assignee)

Comment 9

4 years ago
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.
(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.
Blake,

Could you please check this patch together with the patch from bug 815220? Maybe you could then revise your ui-r-
Blake, http://ftp.mozilla.org/pub/mozilla.org/thunderbird/try-builds/richard.marti@gmail.com-0ed3fdc6437e has this patch and the one from bug 815220 applied. With this it should be easier for the ui-r.
(Reporter)

Comment 13

4 years ago
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)!
Attachment #683333 - Flags: ui-review- → ui-review+
(Assignee)

Comment 14

4 years ago
bwinton, have you also tried moving the widget between the toolbar and the customization palette?
(Reporter)

Comment 15

4 years ago
Yes.
(Assignee)

Comment 16

4 years ago
Created attachment 687864 [details] [diff] [review]
patch v3

Ok, thanks.
Attachment #683333 - Attachment is obsolete: true
Attachment #687864 - Flags: review?(mkmelin+mozilla)

Comment 17

4 years ago
Comment on attachment 687864 [details] [diff] [review]
patch v3

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

Looks ok to me. r=mkmelin
Attachment #687864 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Blocks: 815220
https://hg.mozilla.org/comm-central/rev/48afd0b23600
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 20.0

Comment 19

4 years ago
Backed out due to orange
http://hg.mozilla.org/comm-central/rev/720ac7edeb08

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
https://tbpl.mozilla.org/php/getParsedLog.php?id=17778794&tree=Thunderbird-Trunk&full=1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

4 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 20

4 years ago
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?
Flags: needinfo?
(Assignee)

Updated

4 years ago
Flags: needinfo?(bwinton)
(Reporter)

Comment 21

4 years ago
Update the test, definitely, since this was an intentional change.

Thanks,
Blake.
Flags: needinfo?(bwinton)
Flags: needinfo?
(Assignee)

Comment 22

4 years ago
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).
Attachment #698046 - Flags: review?(mconley)
Comment on attachment 698046 [details] [diff] [review]
finish of the backout

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

Looks good! Great catch, aceman.
Attachment #698046 - Flags: review?(mconley) → review+
Landed the rest of the backout: https://hg.mozilla.org/comm-central/rev/fabf83f68caa
(Assignee)

Comment 25

4 years ago
Created attachment 698105 [details] [diff] [review]
patch v4

This fixes the test.
Attachment #687864 - Attachment is obsolete: true
Attachment #698046 - Attachment is obsolete: true
Attachment #698105 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 26

4 years ago
Comment on attachment 698105 [details] [diff] [review]
patch v4

Adding mconley if he is interested how the fetching of entities got solved ;)
Attachment #698105 - Flags: feedback?(mconley)

Updated

4 years ago
Attachment #698105 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Target Milestone: Thunderbird 20.0 → ---
https://hg.mozilla.org/comm-central/rev/764a9ff6e1ac
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 21.0
(Assignee)

Updated

4 years ago
Depends on: 814041
Comment on attachment 698105 [details] [diff] [review]
patch v4

Neat technique. :)
Attachment #698105 - Flags: feedback?(mconley)
(Assignee)

Updated

4 years ago
Depends on: 859287
You need to log in before you can comment on or make changes to this bug.