491676 - Customizable Toolbars in SeaMonkey MailNews has problems with multistate-buttons

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Bruno 'Aqualon' Escherl]

Attachment: change the CSS rules for button-delete and button-junk

During work on bug 207485 I encountered some issues with multistate-buttons like the junk or delete button and the customizable toolbar feature.

- Moving the buttons from the mailtoolbar to the "parking" area, the junk button has no icon at all and the delete button has the wrong icon. That's fixed by the little css-patch I'll attach to this bug.
- The buttons in the "parking" area keep the state they had before dragging them there, so a junk-button in the NotJunk state stays in this state, and doesn't change after it's dragged back to the mailtoolbar. The same happens for the delete button.

I 'm not exactly sure what happens here, but Thunderbird does a lot of things in MailToolboxCustomizeDone (http://mxr.mozilla.org/comm-central/source/mail/base/content/mailCore.js#103) that we don't do and I couldn't reproduce it there.

CCing Philip cause he ported the customizable mailtoolbar feature.

Comment 1

[Philip Chee]

I think your CSS is totally bogus. Fortunately I am not a reviewer.

-#button-delete {
+#button-delete > #delete-deck > .toolbarbutton-1 {

From some other patch obviously. And for the junk button I think the way Thunderbird does the css makes more sense.

> I 'm not exactly sure what happens here, but Thunderbird does a lot of things
> in MailToolboxCustomizeDone

In Bug 413385 Comment 8 Neil said:

>>+  UpdateMailToolbar(); // XXXRatty: do we need this?
> No, you shouldn't need it.

So I took his word for it.

> that we don't do and I couldn't reproduce it there.
RDF is magic. Moving to a non-RDF implmentation means that Thunderbird has to do a lot of things manually.

Comment 2

[Philip Chee]

So if we:
1. Drag the junk button out of the UI.
2. Finish customization.
3. Focus on a different message.
4. Drag the button back into the UI.

The visual state is stuck in what ever it was in step [1]. However Inspecting the deck element via |document.getElementById("junk-deck").selectedIndex;| I see that the selectedIndex is actually changed depending on whether the focused message is junk or not.

Comment 3

[Philip Chee]

Attachment: Patch v1.0 Wallpaper over stuck junk button state.

> The visual state is stuck in what ever it was in step [1]. However Inspecting
> the deck element via |document.getElementById("junk-deck").selectedIndex;| I
> see that the selectedIndex is actually changed depending on whether the focused
> message is junk or not.

The selectedIndex property is changed by javascript but this has no effect on the selectedIndex attribute so I suspect that the wheels have fallen off the XBL binding.

I doubt that wallpapering over this bug is the right thing to do but I don't know what else to do.

Comment 4

[[e-mail]]

Boris, I guess removing the element from the DOM kills its XBL, and then reinserting it has no effect until it is displayed, for a similar reason to all those problems with missing XBL on newly created but undisplayed elements?

Comment 5

[Boris Zbarsky [:bzbarsky]]

That sounds correct to me, yes.

Comment 6

[Philip Chee]

Attachment: [checked in] Patch v1.1 Update the attribute instead of the property

Wallpaper over XBL inadequacies. Update the selectedIndex attribute instead of the property.

Comment 7

[Philip Chee]

> Moving the buttons from the mailtoolbar to the "parking" area, the junk
> button has no icon at all and the delete button has the wrong icon.

The problem is that, unlike Thunderbird/Firefox we overlay the customize window with CSS from several stylesheets (Navigator, Messenger) and this will get worse when we turn on customization for additional toolbars/toolboxes. In this particular case due to the order and precedence of the CSS stylesheets and the rules contained in them, the junk button gets the list-style-image from one CSS file but the -moz-image-region from another, so the button image is there, but it's just a non-existent region of communicatoricons.png. The workaround is to use sufficiently specific CSS

Comment 8

[Stefan [:stefanh]]

Re the problem that the junk button never look disabled: How about #button-isJunk, #button-notJunk instead of #button-junk in primaryToolbar.css?

Comment 9

[Philip Chee]

Attachment: Patch Cv1.0 CSS fixes.

There are two problems here with the superficial design fault obscuring the fundamental design fault.

1. The junk button doesn't respect the disabled state.
2. When parked in the customize window, the junk button does not have an icon.

The first problem is that we have the observes attribute on the #junk-deck but the CSS is using the #junk-button. Changing the CSS to use the #junk-deck reveals the deeper problem that causes {2}.

The old CSS pre-customization only worked by accident. The list style image (messengericons.png) on the junk deck wasn't inherited by the .toolbarbutton-1 children but were getting the (messengericons.png) from the direct rule for .toolbarbutton-1. But they were getting the -moz-image-region from the deck.

Now when we move the junk button into the customize window they then obtained their list style image from either communicator or navigator so the -moz-image-region was pointing to an invalid region.

This patch is a minimal fix. We could perhaps instead follow Thunderbird and add a class of .junk-button to the child buttons and use that class instead but we would still need more specific CSS than Thunderbird due to the fact that we are overlaying the customizeToolbar window with significantly more stylesheets.

Comment 10

[Robert Kaiser]

Comment on attachment 378243 [details] [diff] [review]
[checked in] Patch v1.1 Update the attribute instead of the property

Pushed as http://hg.mozilla.org/comm-central/rev/6b5fe74359f4

Comment 11

[[e-mail]]

Comment on attachment 378841 [details] [diff] [review]
Patch Cv1.0 CSS fixes.

I'd like to see a patch that removes the direct rules for .toolbarbutton-1 as it makes no sense to overlay them all onto the customise toolbar dialog.

Comment 13

[Philip Chee]

Attachment: [for checkin] Patch Cv1.1 remove direct rules for .toolbarbutton-1

> I'd like to see a patch that removes the direct rules for .toolbarbutton-1 as
> it makes no sense to overlay them all onto the customise toolbar dialog.

Fixed. I also took the opportunity to fix a problem with the delete button not responding to the [disabled="true"] rule.

Comment 14

[Robert Kaiser]

Pushed attachment 382732 [details] [diff] [review] as http://hg.mozilla.org/comm-central/rev/4e75f2a17e39 - leaving to Philip to mark FIXED if everything needed has been done here.