Closed
Bug 491676
Opened 16 years ago
Closed 15 years ago
Customizable Toolbars in SeaMonkey MailNews has problems with multistate-buttons
Categories
(SeaMonkey :: MailNews: Message Display, defect)
SeaMonkey
MailNews: Message Display
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.0b1
People
(Reporter: bugzilla, Assigned: philip.chee)
References
Details
Attachments
(3 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
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 | ||
Updated•16 years ago
|
Assignee: nobody → aqualon
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•16 years ago
|
||
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 | ||
Updated•16 years ago
|
Assignee: aqualon → nobody
OS: Linux → All
Hardware: x86 → All
Assignee | ||
Comment 3•16 years ago
|
||
> 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)
Comment 4•16 years ago
|
||
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•16 years ago
|
||
That sounds correct to me, yes.
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → philip.chee
Assignee | ||
Comment 6•16 years ago
|
||
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)
Assignee | ||
Comment 7•16 years ago
|
||
> 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
Updated•16 years ago
|
Attachment #378243 -
Flags: superreview?(neil)
Attachment #378243 -
Flags: superreview+
Attachment #378243 -
Flags: review?(neil)
Attachment #378243 -
Flags: review+
Assignee | ||
Updated•16 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•16 years ago
|
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
Comment 8•16 years ago
|
||
Re the problem that the junk button never look disabled: How about #button-isJunk, #button-notJunk instead of #button-junk in primaryToolbar.css?
Assignee | ||
Comment 9•16 years ago
|
||
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)
Updated•16 years ago
|
Keywords: checkin-needed
Comment 10•16 years ago
|
||
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 11•16 years ago
|
||
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-
Assignee | ||
Comment 13•15 years ago
|
||
> 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)
Updated•15 years ago
|
Attachment #382732 -
Flags: superreview?(neil)
Attachment #382732 -
Flags: superreview+
Attachment #382732 -
Flags: review?(neil)
Attachment #382732 -
Flags: review+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•15 years ago
|
Attachment #382732 -
Attachment description: Patch Cv1.1 remove direct rules for .toolbarbutton-1 → [for checkin] Patch Cv1.1 remove direct rules for .toolbarbutton-1
Comment 14•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•