Closed Bug 322077 Opened 16 years ago Closed 15 years ago

clean up toggleSidebar code

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3 alpha1

People

(Reporter: asqueella, Assigned: asqueella)

References

Details

Attachments

(1 file, 2 obsolete files)

This was intended to be a fix for bug 271022, but gavin fixed that in a simpler way. The to-be-attached patch is a more appropriate (IMO) fix for that problem - it moves the search-box focusing code from the generic toggleSidebar function to onload handlers in history and bookmarks panels. Some unused code is removed:
- The 'synchronous' focusing code in toggleSidebar can only run if toggleSidebar is called for bookmarks or history panel with forceOpen=true. There are no such calls in Firefox code, and it's pretty unlikely that any extensions rely on this. In case they are, the textbox just won't get focused which is a minor problem
- "elementtofocus" is a leftover from seamonkey
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → asqueella
Status: NEW → ASSIGNED
Attachment #207315 - Flags: review?(mconnor)
Attachment #207315 - Flags: review?(mconnor) → review?(gavin.sharp)
gavin had questions about that code, which is a good indicator it needs to be made clearer and documented :)
Attachment #207315 - Attachment is obsolete: true
Attachment #241760 - Flags: review?(gavin.sharp)
Attachment #207315 - Flags: review?(gavin.sharp)
Comment on attachment 241760 [details] [diff] [review]
patch, more code cleanup, and documentation

This is much better, thanks! A couple nits:

>Index: browser/base/content/browser.js

>-function toggleSidebar(aCommandID, forceOpen) {
>+/**
>+ * Opens or closes the sidebar identified by commandID.
>+ *
>+ * @param commandID a string identifying the sidebar to toggle.
>+ *                  Optional if a sidebar is guaranteed to be open.

Should you add something like "the ID of the broadcaster associated with the sidebar being toggled" (this goes together with the @note below, I guess)?

>+ * @param forceOpen a boolean that indicates if the sidebar should be
>+ *                  forced open.  In other words the toggle won't be
>+ *                  allowed to close the sidebar (optional).

How about "boolean indicating whether the sidebar should be opened regardless of it's current state." ?

>-  var elt = document.getElementById(aCommandID);
>-  var sidebar = document.getElementById("sidebar");
>+  var elt = document.getElementById(commandID);
>+  var sidebar = document.getElementById("sidebar"); // xul:browser

Surely we can find something better than "elt"? "broadcaster" or "sidebarBroadcaster", maybe?

>+  if (elt.getAttribute("checked") == "true") {
>+    if(!forceOpen) {
>+      elt.removeAttribute("checked");
>+      sidebarBox.setAttribute("sidebarcommand", "");
>+      sidebarTitle.setAttribute("value", "");

.value?

>+      sidebarBox.hidden = true;
>+      sidebarSplitter.hidden = true;
>+      content.focus();
>+    }
>     return;
>   }
> 
>+  // now we need to show the specified sidebar
>+
>   var elts = document.getElementsByAttribute("group", "sidebar");

Again, is there something better than "elts" here? "otherSidebarBroadcasters"? Guess that's kinda long... Maybe just add a comment saying what "elts" is, at least?
Attached patch patch v3Splinter Review
Attachment #241760 - Attachment is obsolete: true
Attachment #241768 - Flags: review?(gavin.sharp)
Attachment #241760 - Flags: review?(gavin.sharp)
(In reply to comment #3)
> >-function toggleSidebar(aCommandID, forceOpen) {
> >+/**
> >+ * Opens or closes the sidebar identified by commandID.
> >+ *
> >+ * @param commandID a string identifying the sidebar to toggle.
> >+ *                  Optional if a sidebar is guaranteed to be open.
> 
> Should you add something like "the ID of the broadcaster associated with the
> sidebar being toggled" (this goes together with the @note below, I guess)?
> 
The whole broadcaster setup is already described in the @note, so I just added an explicit reference to it.

> Surely we can find something better than "elt"? "broadcaster" or
> "sidebarBroadcaster", maybe?
> [...]
> .value?
> 
OK, since we're starting to clean up the function as a whole :)

> >+  // now we need to show the specified sidebar
> >+
> >   var elts = document.getElementsByAttribute("group", "sidebar");
> 
> Again, is there something better than "elts" here? "otherSidebarBroadcasters"?
> Guess that's kinda long... Maybe just add a comment saying what "elts" is, at
> least?
> 
Renamed to |broadcasters|, even though these are not necessarily broadcasters. Should be clear from the code and comments.
Summary: clean up sidebar's search-box focusing code → clean up toggleSidebar code
Attachment #241768 - Flags: review?(gavin.sharp) → review+
mozilla/browser/components/history/content/history-panel.xul 	1.34
mozilla/browser/components/bookmarks/content/bookmarksPanel.xul 	1.24
mozilla/browser/components/bookmarks/content/bookmarksPanel.js 	1.10
mozilla/browser/base/content/browser.js 	1.729
mozilla/browser/components/history/content/history.js 	1.47
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 alpha1
Every so often I can't close the bookmark sidebar with the close button on the sidebar.  I get these two messages:
Error: sidebarBroadcaster has no properties
Source File: chrome://browser/content/browser.js
Line: 3803

Warning: Empty string passed to getElementById().

Looking up sidebarBroadcaster brought me here.  I don't know how to reproduce it yet.  Any ideas?  
You didn't say what version you have this problem on, is that a clean profile or do you have any extensions installed, whether that problem is a regression for you (and if so, when it started appearing). But even with answers to those questions, your message should be in its own bug, since it's not clear at all it's related to the fix here.

It looks like the value of "sidebarcommand" attribute on an element with the id "sidebar-box" is empty in that case. If, as you say, it's the bookmarks sidebar, something probably messes up the value of the attribute.
I'm using trunk nightly builds.  I've seen this for about a month.  It happens rarely.  I came looking for help because I don't think I have enough information to get a bug going anywhere.  Now I'm wondering if this could be a bug similar to Bug 383204.  So I'll watch that bug.  
This patch appears to strip out the stuff added in Bug 188910 (if I'm reading it right, which is a big if), thus regressing it again. The latest version of that incredibly annoying bug is now filed as Bug 385062. Though I realise its probably a bit late to be mentioning this now. :)
Depends on: 385062
You need to log in before you can comment on or make changes to this bug.