Last Comment Bug 860093 - Regression from Bug 824150: toolbox has no method getElementById()
: Regression from Bug 824150: toolbox has no method getElementById()
: regression
Product: Thunderbird
Classification: Client Software
Component: Toolbars and Tabs (show other bugs)
: 22 Branch
: All All
: -- normal (vote)
: Thunderbird 23.0
Assigned To: Sebastian Hengst [:aryx][:archaeopteryx]
Depends on:
Blocks: 824150
  Show dependency treegraph
Reported: 2013-04-09 16:36 PDT by alta88
Modified: 2013-04-15 13:25 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch, v1 (1.41 KB, patch)
2013-04-10 03:53 PDT, Sebastian Hengst [:aryx][:archaeopteryx]
mconley: review+
Details | Diff | Splinter Review
patch, v2, r=mconley (1.42 KB, patch)
2013-04-13 13:07 PDT, Sebastian Hengst [:aryx][:archaeopteryx]
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description alta88 2013-04-09 16:36:54 PDT
there's no getElementById on an element (toolbox) so this is throwing, maybe even preventing the after customize event..

diff --git a/mail/base/content/mailCore.js b/mail/base/content/mailCore.js
--- a/mail/base/content/mailCore.js
+++ b/mail/base/content/mailCore.js
@@ -204,28 +204,28 @@ function MailToolboxCustomizeDone(aEvent
   var customizePopup = document.getElementById(customizePopupId);
   // make sure our toolbar buttons have the correct enabled state restored to them...
   if (this.UpdateMailToolbar != undefined)
-  var toolbox = document.getElementsByAttribute("doCustomization", "true")[0];
+  let toolbox = document.querySelector('[doCustomization="true"]');
   if (toolbox) {
     // The GetMail button is stuck in a strange state right now, since the
     // customization wrapping preserves its children, but not its initialized
     // state. Fix that here.
     // Fix Bug 565045: Only treat "Get Message Button" if it is in our toolbox
-    var popup = toolbox.getElementsByAttribute("id", "button-getMsgPopup")[0];
+    let popup = toolbox.getElementById("button-getMsgPopup");
     if (popup) {
       // We can't use _teardown here, because it'll remove the Get All menuitem
-      let sep = toolbox.getElementsByAttribute("id", "button-getAllNewMsgSeparator")[0];
+      let sep = toolbox.getElementById("button-getAllNewMsgSeparator");
       while (popup.lastChild != sep)
Comment 1 :aceman 2013-04-10 02:49:26 PDT
Thanks for catching this.
The original getElementsByAttribute is a method of toolbox, so the change just needs to be reverted (maybe a comment added).
Comment 2 Sebastian Hengst [:aryx][:archaeopteryx] 2013-04-10 03:53:52 PDT
Created attachment 735673 [details] [diff] [review]
patch, v1

Now using querySelector.
Comment 3 :aceman 2013-04-10 04:09:02 PDT
Aryx, can you try if the patch also fixes bug 859287?
Comment 4 Sebastian Hengst [:aryx][:archaeopteryx] 2013-04-10 05:56:22 PDT
*** Bug 859287 has been marked as a duplicate of this bug. ***
Comment 5 Sebastian Hengst [:aryx][:archaeopteryx] 2013-04-10 05:59:08 PDT
It also fixes bug 859287, sorry for the regression.
Comment 6 Josiah Bruner [:JosiahOne] (needinfo for responses) 2013-04-10 07:38:12 PDT
(In reply to Archaeopteryx [:aryx] from comment #5)
> It also fixes bug 859287, sorry for the regression.

Nope. It does not fix bug 859287. So it is not a regression. Or at least not the entire reason.
Comment 7 :aceman 2013-04-10 13:12:36 PDT
Comment on attachment 735673 [details] [diff] [review]
patch, v1

Maybe mconley could look at this simple thing faster.
Comment 8 Mike Conley (:mconley) - (needinfo me!) 2013-04-12 01:15:50 PDT
Comment on attachment 735673 [details] [diff] [review]
patch, v1

Definitely yes. r=me, no need to bother Mark with this one.
Comment 9 Sebastian Hengst [:aryx][:archaeopteryx] 2013-04-13 13:07:04 PDT
Created attachment 737174 [details] [diff] [review]
patch, v2, r=mconley
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-04-14 18:24:53 PDT

Standard practice throughout the project is to resolve bugs when they hit {comm,mozilla}-central and use the status flags for tracking branch uplifts. Please put checkin-needed back on the bug when it gets approval for aurora uplift.
Comment 11 Mark Banner (:standard8) 2013-04-15 00:24:21 PDT
Comment on attachment 737174 [details] [diff] [review]
patch, v2, r=mconley

[Triage Comment]
If the patch is ready, and you want to push this to aurora (or other branch) the correct way is to signal approval-comm-<branch> (set to ?) on the patch.

Then we can come along a approve it. a=me since this is a low risk fix for a regression.
Comment 12 Mark Banner (:standard8) 2013-04-15 13:25:11 PDT

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