Closed Bug 392936 Opened 17 years ago Closed 17 years ago

Switching 'Mode Toolbar' location ('top' to 'bottom') causes Calendar icon to duplicate


(Calendar :: Lightning Only, defect)

Windows 2000
Not set


(Not tracked)



(Reporter:, Assigned: michael.buettner)




(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20070725 Firefox/
Build Identifier: 0.6a1 2007082004 trunk

Upon each update of the 'Mode Toolbar' location (top or bottom), the Calendar icon(s) get(s) duplicated.

Reproducible: Always

Steps to Reproduce:
1. Open 'Mode Toolbar' editor
2. Change 'Location'
3. Change 'Location' again, and again, ...
Actual Results:  
Need to manually remove duplicate icons

Expected Results:  
No need to remove duplicate icons
Works for me using Lightning 0.7pre (2007082004) with Thunderbird (20070820) on Windows 2000. Might be a Trunk only issue.
Confirmed as a TRUNK ONLY issue using Lightning 0.6a1 (2007090806) with Thunderbird 3.0a1pre (2007090904).

Using Lightning 0.7pre (2007090903) with Thunderbird (20070909) everything is OK.
Ever confirmed: true
OS: Windows XP → Windows 2000
Summary: Switching 'Mode Toolbar' location ('top' to 'bottom') causes Calendar icon to duplicate → [Trunk] Switching 'Mode Toolbar' location ('top' to 'bottom') causes Calendar icon to duplicate
Version: unspecified → Trunk
In contrast to my test results from Comment #2 I now see the issue on MOZILLA_1_8_BRANCH too. I found the following regession range:

WORKS in Thunderbird (2007093003) + Lightning 0.7 (2007102304)
FAILS in Thunderbird (2007100104) + Lightning 0.7 (2007102304)

Checkins to MOZILLA_1_8_BRANCH during regression range: 

Most probably also regressed by Bug 267833.
Summary: [Trunk] Switching 'Mode Toolbar' location ('top' to 'bottom') causes Calendar icon to duplicate → Switching 'Mode Toolbar' location ('top' to 'bottom') causes Calendar icon to duplicate
Version: Trunk → unspecified
Blocks: 267833
Can someone give me something resembling a minimal testcase that doesn't involve the entire Calendar UI?  Or some pointers to the relevant code, at least?
See [1] for the relevant source code location that handles this particular toolbar. The reason for this rather elaborate handling of the toolbar is that it should be possible to dynamically place this element. Unfortunately, the constructor of the toolbar binding gets called each time the DOM node gets bound to a different parent. That's why I'm touching the internals of the toolbar to get around this problem. It would be great if there's a cleaner solution to the problem...

Oh, I see.  You're modifying the DOM from inside a mutation listener (a no-no if you want sane behavior, for what it's worth).  That means that the constructor won't fire until after your return from the mutation listener and the end of the attribute mutation update.  Presumably that's screwing up your save/restore stuff (as in, you restore too early)?

If it's reasonable to do so, I'd suggest putting most of the code in the mutation listener off on a timeout (except for the "remove the listener" part, pretty much).
Flags: wanted-calendar0.8?
This bug is not important enough to warrant wanted0.8 status. 

We would however take a fix for this bug into 0.8, if such a fix materializes.
Flags: wanted-calendar0.8? → wanted-calendar0.8-
1. Customize toolbar. Change toolbar location to 'Top' or 'Bottom' as desired. 
2. Restart Thunderbird.
3. Customize toolbar. Select 'Restore Default Set'.
Attached patch Patch v1 (obsolete) — — Splinter Review
Don't know exactly why, but after observing WHEN the duplication occurs, this patch works for me in MOZILLA_1_8_BRANCH (Windows).

Can anyone please verify this in TRUNC and maybe on other platforms?
Attachment #298248 - Flags: review?(michael.buettner)
Comment on attachment 298248 [details] [diff] [review]
Patch v1

Admittedly, I don't understand why this solves the duplication problem. It is necessary it hold a reference to the toolbox palette, then rearrange the dom tree and reassign the reference to the palette. If we don't manually reassign the palette reference, it magically disappears from the tree. I decided to reassign the palette after the tree has been modified. I don't really understand where's the relation to the presumably unrelated other actions in this function. But as it solves this annoying problem I tend to just take the patch for now and investigate the underlying problem later. On top of that, I would like to follow Boris advice and defer the action until the mutation listener has been finished. I'm going to change the patch accordingly before checking it in. Sven, thanks for the patch. r=mickey.
Attachment #298248 - Flags: review?(michael.buettner) → review+
Attached patch patch v2 — — Splinter Review
Previous version of the patch merged with Boris recommendation. Carrying forward r+.
Assignee: nobody → michael.buettner
Attachment #298248 - Attachment is obsolete: true
Attachment #298678 - Flags: review+
patch checked in on trunk and MOZILLA_1_8_BRANCH

Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Checked on linux (branch and trunk build 2008012318), windows (trunk) and Mac (branch) -> task is fixed and verified.  
Flags: wanted-calendar0.8-
You need to log in before you can comment on or make changes to this bug.