Closed Bug 491676 Opened 11 years ago Closed 11 years ago

Customizable Toolbars in SeaMonkey MailNews has problems with multistate-buttons

Categories

(SeaMonkey :: MailNews: Message Display, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0b1

People

(Reporter: bugzilla, Assigned: philip.chee)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
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.
Assignee: nobody → aqualon
Status: NEW → ASSIGNED
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.
Assignee: aqualon → nobody
OS: Linux → All
Hardware: x86 → All
> 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.
Attachment #376890 - Flags: review?(neil)
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?
That sounds correct to me, yes.
Assignee: nobody → philip.chee
Wallpaper over XBL inadequacies. Update the selectedIndex attribute instead of the property.
Attachment #376890 - Attachment is obsolete: true
Attachment #378243 - Flags: superreview?(neil)
Attachment #378243 - Flags: review?(neil)
Attachment #376890 - Flags: review?(neil)
> 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
Attachment #378243 - Flags: superreview?(neil)
Attachment #378243 - Flags: superreview+
Attachment #378243 - Flags: review?(neil)
Attachment #378243 - Flags: review+
Keywords: checkin-needed
Attachment #378243 - Attachment description: Patch v1.1 Update the attribute instead of the property → [for-checkin] Patch v1.1 Update the attribute instead of the property
Re the problem that the junk button never look disabled: How about #button-isJunk, #button-notJunk instead of #button-junk in primaryToolbar.css?
Attached patch Patch Cv1.0 CSS fixes. (obsolete) — Splinter Review
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.
Attachment #378841 - Flags: review?(neil)
Keywords: checkin-needed
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
Attachment #378243 - Attachment description: [for-checkin] Patch v1.1 Update the attribute instead of the property → [checked in] Patch v1.1 Update the attribute instead of the property
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.
Attachment #378841 - Flags: review?(neil) → review-
Duplicate of this bug: 497400
Blocks: 497400
> 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.
Attachment #378841 - Attachment is obsolete: true
Attachment #382732 - Flags: superreview?(neil)
Attachment #382732 - Flags: review?(neil)
Attachment #382732 - Flags: superreview?(neil)
Attachment #382732 - Flags: superreview+
Attachment #382732 - Flags: review?(neil)
Attachment #382732 - Flags: review+
Keywords: checkin-needed
Attachment #382732 - Attachment description: Patch Cv1.1 remove direct rules for .toolbarbutton-1 → [for checkin] Patch Cv1.1 remove direct rules for .toolbarbutton-1
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.
Keywords: checkin-needed
Target Milestone: --- → seamonkey2.0b1
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.