661263 - Attachment bar loses attachment number and name when action button is removed by toolbar customization

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[rsx11m]

Observed in both TB 5.0b1 candidate build and in a current 7.0a1 try-server build (thus after bug 656045 patch applied) for Windows 7 and Linux.

Steps to reproduce:
1) Load a message with an attachment
2 [review]) right-click on the attachment bar and select "Customize"
3) remove the "Save" button by putting it into the palette, button disappears
4) restart Miramar/Shredder (this step is needed to see the effect)
5) select the same message or another one with one or more attachments
6) only the "twisty" and the attachment icon show up, no attachment number/size
   nor the clickable name of a single attachment
7) open customization palette, the button is gone - click "Restore Default"
8) restart Miramar/Shredder, normal behavior is restored.

Error console shows at step #5 for a message with a single attachment:
> Error: saveAllSingle is null
> Source File: chrome://messenger/content/msgHdrViewOverlay.js
> Line: 2018

For a message with multiple attachment the error reads almost the same:
> Error: saveAllSingle is null
> Source File: chrome://messenger/content/msgHdrViewOverlay.js
> Line: 2030

The general question may be if the customization option was intentional to start with, but it would be an intuitive way to remove the button if a user doesn't want it, thus saving a few pixels of vertical space.

Comment 1

[rsx11m]

(sorry about the bogus "attachment" links in the steps to reproduce, hope this
 would be fixed some day in bugzilla...)

Comment 2

[Jim Porter (:squib)]

This is easy to fix, but I also need to fix the buttons not getting shown/hidden properly after they're moved off of the toolbar palette. I'm sure there's some event I need to be listening for...

Comment 3

[Jim Porter (:squib)]

Attachment: Fix this (no tests)

I'm at a loss as to how to actually customize a toolbar via Mozmill, but this should work; it's a pretty simple patch. Full disclosure, I changed a couple of related things while I was here:

1) I adjusted the padding on the attachment count to make sure that the attachment bar is the same height in the single and multiple case.

2) I changed the message header toolbar to update its junk/reply buttons on customizeChange instead of customizeDone.

3) I added a title to the toolbaritem for the "save all attachments" button so that the title is shown in the customize dialog. (I'm not sure if this will work on Mac; see bug 622343 for a similar issue.)

If anyone knows how to automate toolbar customization via Mozmill, I'll try to write up some tests for this.

Comment 4

[Jim Porter (:squib)]

So, I can customize the toolbar, but I can't get it to persist properly. I know newer Mozmills support some kind of drag-and-drop, but I think it's intra-window...

Comment 5

[Jim Porter (:squib)]

Should this block 5.0? I this it's rare enough that it probably won't hurt people too much, but it is a bit stupid...

Comment 6

[David :Bienvenu]

(In reply to comment #5)
> Should this block 5.0? I this it's rare enough that it probably won't hurt
> people too much, but it is a bit stupid...

no, I don't think it should block 5.0 - it's rare, and a restart fixes it.

Comment 7

[rsx11m]

Actually, a restart is needed to prompt the issue after removal of the button, but I agree that it needs quite a bit user interaction to figure out that the toolbar has a customization feature and the menu can be removed in this way.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 536808 [details] [diff] [review]
Fix this (no tests)

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

Other than the nit below, I like this change, and have verified that it fixes the bug.

Of course, I can't give it an r+ without any tests…  So, r-, but only due to lack of tests.

(And I'm still not entirely convinced that customizing that toolbar makes a lot of sense. ;)

Later,
Blake.

::: mail/base/content/msgHdrViewOverlay.js
@@ +2072,5 @@
> +  else {
> +    saveAllSingle.hidden = true;
> +    saveAllMultiple.hidden = false;
> +    saveAllMultiple.disabled = allDeleted;
> +  }

I think this might be written a little cleaner as:
saveAllSingle.hidden = (currentAttachments.length != 1);
saveAllMultiple.hidden = !saveAllSingle.hidden;
saveAllSingle.disabled = saveAllMultiple.disabled = allDeleted;

Comment 9

[Jim Porter (:squib)]

Any ideas on how to test this? I guess I could try to manually persist the toolbar state, though I'm not sure how well that'll work...

Comment 10

[Blake Winton (:bwinton) (:☕️)]

You can probably get some hints from <http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/folder-display/test-folder-toolbar.js>.

(It looks like add_to_toolbar and remove_from_toolbar might do what you want.  They're around <http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#2170>.)

Later,
Blake.

Comment 11

[Jim Porter (:squib)]

Unfortunately, I tried those already, and they don't work, since they don't persist the state. Thus, when closing and reopening the 3pane, the changes are lost, so the bug never manifests.

Comment 12

[rsx11m]

Could you sneak in a modified localstore.rdf into the test profile?

That's what I see after removal of the button:
> <RDF:Description RDF:about="chrome://messenger/content/messenger.xul#attachment-view-toolbar"
>                    iconsize="small"
>                    mode="full"
>                    currentset="__empty" />

as changed from the previous content (after adding it back):
>                    currentset="attachmentSaveAll" />

Comment 13

[Jim Porter (:squib)]

Attachment: Add some tests

Ok, I managed to get tests working. Apparently all that's needed to manifest this bug is selecting a different message! It's just that it breaks in an ever-so-slightly less obvious way in that case. Who knew! Attached is the patch, and here is the pending try server run, just in case:

http://build.mozillamessaging.com/tinderboxpushlog/?tree=ThunderbirdTry&rev=9684c7e7d56b

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 540685 [details] [diff] [review]
Add some tests

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

I like it!

r=me.

Thanks,
Blake.

Comment 15

[Jim Porter (:squib)]

Checked in: http://hg.mozilla.org/comm-central/rev/c736643477f0