Last Comment Bug 701146 - Multi-case window id's breaks toolbar customizations
: Multi-case window id's breaks toolbar customizations
Status: RESOLVED FIXED
[qa+]
: regression
Product: Toolkit
Classification: Components
Component: Toolbars and Toolbar Customization (show other bugs)
: 10 Branch
: All All
: -- normal (vote)
: mozilla11
Assigned To: Mike Conley (:mconley)
:
: :Gijs Kruitbosch
Mentors:
: 702035 703520 (view as bug list)
Depends on:
Blocks: 662173 701005
  Show dependency treegraph
 
Reported: 2011-11-09 12:37 PST by Mike Conley (:mconley)
Modified: 2012-02-01 13:58 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Patch v1 (1.84 KB, patch)
2011-11-09 12:46 PST, Mike Conley (:mconley)
no flags Details | Diff | Splinter Review
Patch v2 (944 bytes, patch)
2011-11-10 13:32 PST, Mike Conley (:mconley)
enndeakin: review+
asa: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
toolbar - restore default set (239.67 KB, image/jpeg)
2012-01-16 07:35 PST, Paul Silaghi, QA [:pauly]
no flags Details

Description Mike Conley (:mconley) 2011-11-09 12:37:09 PST
The Thunderbird team just noticed this one.

Our main window has id="messengerWindow", and when doing the comparison here:  http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/customizeToolbar.js#760

...it appears that the window ID has been automatically lowercased, so the comparison fails, and customization is not possible.

This looks like fallout from bug 662173.
Comment 1 Mike Conley (:mconley) 2011-11-09 12:46:46 PST
Created attachment 573292 [details] [diff] [review]
Patch v1

This might be a bit heavy-handed, but this solution seems to work.  When getting the documentId, we lowercase it before doing any operations (either getting/setting to the Event.dataTransfer object).

Or is there a better way to go here?
Comment 2 Neil Deakin 2011-11-09 13:37:22 PST
You should only need the lowercasing for the 'contains' calls.

As an aside, this] seems like an inconsistent api here. DOMStringList.contains is, presumably, case sensitive yet data transfer apis do automatic lowercase conversion.
Comment 3 Mike Conley (:mconley) 2011-11-10 06:41:25 PST
Neil:

Hrm - so there are 3 instances in that diff where I add toLowerCase where a contains is not used.

Removing any combination of those 3 caused toolbar customization to break.

For example, removing it from onToolbarDragStart didn't let me start the process of dragging toolbar buttons. Removing it from onToolbarDragOver didn't show me the little drop location indicator while dragging the button around, and didn't let me drop.  Removing it from onPaletteDrop didn't let me drag buttons back into the toolbar palette.

Thoughts?
Comment 4 Neil Deakin 2011-11-10 07:51:06 PST
My testing doesn't show that. I got it to work with just changing the two 'contains' lines. Note that I used Firefox with an adjusted id of 'main-windowA'
Comment 5 Mike Conley (:mconley) 2011-11-10 08:02:54 PST
Strange.  Maybe you're going about it differently than I did.  Can you post your patch for me to try?
Comment 6 Mike Conley (:mconley) 2011-11-10 13:32:27 PST
Created attachment 573629 [details] [diff] [review]
Patch v2

Hm, alright, so I just sync'd up with m-c/c-c, and yeah, you're right - this simplified patch works now.  Thanks!
Comment 7 Mike Conley (:mconley) 2011-11-11 06:46:14 PST
Checked into mozilla-inbound here:  https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad06b88a36b
Comment 8 Ed Morley [:emorley] 2011-11-12 04:55:45 PST
https://hg.mozilla.org/mozilla-central/rev/0ad06b88a36b
Comment 9 Mark Banner (:standard8, afk until Dec) 2011-11-15 12:58:07 PST
Note to drivers: the regressing bug landed hours before the last merge and hence we couldn't fix it in time, so the approval is to fix a regression on aurora which is also keeping the Thunderbird aurora tree orange at the moment.
Comment 10 Mike Conley (:mconley) 2011-11-15 15:16:31 PST
Committed to mozilla-aurora as https://hg.mozilla.org/releases/mozilla-aurora/rev/81e7a0f84279
Comment 11 Mark Banner (:standard8, afk until Dec) 2011-11-18 03:14:14 PST
*** Bug 702035 has been marked as a duplicate of this bug. ***
Comment 12 Mark Banner (:standard8, afk until Dec) 2011-11-18 03:14:40 PST
*** Bug 703520 has been marked as a duplicate of this bug. ***
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 14:14:38 PST
Does this affect Firefox as well? If so, is there something QA can do to verify the fix?
Comment 14 Philip Chee 2011-12-29 03:09:55 PST
> Does this affect Firefox as well?
Fortunately the Firefox main window doesn't use camelcase or inicaps so isn't affected by this bug. But there is certainly some potential for extensions to trip over this.

> If so, is there something QA can do to verify the fix?
Er? How about testing with Thunderbird 10b?
Comment 15 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-01-05 11:16:02 PST
Are there any extensions/themes/customizations which were known to have displayed this bug?
Comment 16 Philip Chee 2012-01-05 11:18:54 PST
Not that I know of. But then there are over 8000 active extensions and long gone are the days when I could manually track all of the extensions on AMO.
Comment 17 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-01-05 11:21:45 PST
What is the end-user manifestation of this bug? Is there something that changes in the UI? We will have to track that down in a "broken" build before verification can be done.
Comment 18 Neil Deakin 2012-01-05 11:24:16 PST
This bug caused dragging items to and from the toolbar during toolbar customization to not always work in Thunderbird.
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-01-05 11:26:55 PST
Thanks Neil.

Anyone verifying this fix, you will want to be sure you can reproduce this bug on a previous Thunderbird build (see comment 18).
Comment 20 Mike Conley (:mconley) 2012-01-05 11:30:01 PST
The relevant Thunderbird bug is bug 701005.  That bug states that our automated tests caught the problem on 2001/09/11 - so a nightly from around there should suffice.
Comment 21 Paul Silaghi, QA [:pauly] 2012-01-16 07:35:26 PST
Created attachment 588878 [details]
toolbar - restore default set

I can reproduce the issue on Thunderbird Nightly from 2011-11-09 (ftp://ftp.mozilla.org/pub/thunderbird/nightly/2011/11/2011-11-09-03-00-29-comm-central/thunderbird-11.0a1.en-US.win32.installer.exe) and cannot reproduce it on Thunderbird 10b3.
Dragging items during toolbar customization works fine on Firefox 10b4 on Win 7, XP, MacOS 10.7 and Ubuntu 11.10.
The only strange behavior I've noticed is when clicking first time on "Restore Default Set" button on the Toolbar Customization panel, a lot of items suddenly appear at the bottom of the list.

Note You need to log in before you can comment on or make changes to this bug.