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

Status: VERIFIED FIXED

(Calendar :: Lightning Only, defect)

Description

[Wim Bos]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.6) Gecko/20070725 Firefox/2.0.0.6
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

Comment 1

[Stefan Sitter [:ssitter]]

Works for me using Lightning 0.7pre (2007082004) with Thunderbird 2.0.0.7pre (20070820) on Windows 2000. Might be a Trunk only issue.

Comment 2

[Stefan Sitter [:ssitter]]

Confirmed as a TRUNK ONLY issue using Lightning 0.6a1 (2007090806) with Thunderbird 3.0a1pre (2007090904).

Using Lightning 0.7pre (2007090903) with Thunderbird 2.0.0.7pre (20070909) everything is OK.

Comment 3

[Stefan Sitter [:ssitter]]

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 2.0.0.7pre (2007093003) + Lightning 0.7 (2007102304)
FAILS in Thunderbird 2.0.0.7pre (2007100104) + Lightning 0.7 (2007102304)

Checkins to MOZILLA_1_8_BRANCH during regression range: 
<http://tinyurl.com/yuefen>

Most probably also regressed by Bug 267833.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Michael Büttner (no reviews TFN)]

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...

[1] http://lxr.mozilla.org/mozilla1.8/source/calendar/lightning/content/messenger-overlay-sidebar.js#248

Comment 6

[Boris Zbarsky [:bzbarsky]]

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).

Comment 8

[Simon Paquet [:sipaq]]

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.

Comment 9

[Stefan Sitter [:ssitter]]

Workaround:
1. Customize toolbar. Change toolbar location to 'Top' or 'Bottom' as desired. 
2. Restart Thunderbird.
3. Customize toolbar. Select 'Restore Default Set'.

Comment 10

[Sven Giermann]

Attachment: Patch v1

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?

Comment 11

[Michael Büttner (no reviews TFN)]

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.

Comment 12

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

Previous version of the patch merged with Boris recommendation. Carrying forward r+.

Comment 13

[Michael Büttner (no reviews TFN)]

patch checked in on trunk and MOZILLA_1_8_BRANCH

-> FIXED

Comment 14

[Andreas Treumann]

Checked on linux (branch and trunk build 2008012318), windows (trunk) and Mac (branch) -> task is fixed and verified.