If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Tabs menu does not show the installed sidebars

RESOLVED FIXED

Status

SeaMonkey
Sidebar
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Gilles Durys, Assigned: Chase Tingley)

Tracking

({regression})

Trunk
x86
All
regression

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

v3
2.12 KB, patch
Robert John Churchill
: review+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
The "tabs" menu on the sidebar does not contain any of the installed sidebar,
only "Customize sidebars" and "Sidebars directory".
The customize sidebar dialog shows all of the installed sidebars.

It was functional on a 2003061922 trunk build but not anymore on a 2003062022
trunk build.

Updated

14 years ago
Keywords: smoketest

Comment 1

14 years ago
This is a regression, not smoketest.
Keywords: smoketest → regression
(Assignee)

Comment 2

14 years ago
After a bit of poking, this doesn't look like an RDF bug.  The datasource gets
created fine.  There's a problem with templating, but I'm not sure it's in the
templating code either -- my hunch is either a js/xul bug or (more likely) a xul
issue.

The XUL that should be generating this menu is the <toolbarbutton> item at 
http://lxr.mozilla.org/seamonkey/source/xpfe/components/sidebar/resources/sidebarOverlay.xul#70
and the <menupopup> inside it.

Note that no datasource is initially set to provide data to run through the
template.  The datasource and ref are set by the JS function
SidebarBuildPickerPopup(), which is an onpopupshowing() handler.

For whatever reason, this doesn't seem to be getting called.  I verified this by
moving the code that sets datasource and ref from SidebarBuildPickerPopup() to
sidebar_overlay_init(), and everything worked fine.

I would suspect that there has been some fairly recent patch to XUL that changed
some semantic -- e.g., perhaps toolbarbutons no longer receive onpopupshowing
events.  I could probably track down the exact cause with enough time, but
someone who's been more in tune with xul checkins over the last month might know
better.  
(Assignee)

Comment 3

14 years ago
Created attachment 129037 [details] [diff] [review]
patch

Forcing the menu builder to rebuild solves this problem.  This is a fairly
common hack (actually, I think the semantics of requiring manual rebuild() when
you add a datasource make some sense), although there may still be a bug that
is preventing something from getting notified somewhere in xul.

I'm still not sure why this regressed when it did.

Comment 4

14 years ago
Comment on attachment 129037 [details] [diff] [review]
patch

r=rjc@rjcdb.com
Attachment #129037 - Flags: review+
(Assignee)

Comment 5

14 years ago
I finally figured out the source of the regressin, which is bug 209634.
After adding the datasource in SidebarBuildPickerPopup(), the sidebar code
was making an (idempotent) |menu.setAttribute('ref', sidebarObj.resource);| call.
This triggered the code in nsXULTemplateBuilder::AttributeChange to force a rebuild.
The fix for 209634 filtered out this attribute update, so the menu is never rebuilt.
CC'ing bz for the heck of it; this patch should probably be updated to
remove the (now useless) setAttribute line as well.
Hmm.. didn't know about the rebuild() thing.  Could you possibly apply the same
patch to the code in pref-applications.js?  I just realized that when I patched
it in bug 213914 I forgot to patch all the callsites...
(Assignee)

Updated

14 years ago
Attachment #129037 - Attachment is obsolete: true
(Assignee)

Comment 7

14 years ago
Created attachment 129691 [details] [diff] [review]
patch v2

Removes the now-unnecessary setAttribute line.	Makes the same change to
pref-applications.js.
(Assignee)

Comment 8

14 years ago
Taking
Assignee: shliang → tingley
(Assignee)

Updated

14 years ago
Attachment #129691 - Flags: superreview?(bz-vacation)
Attachment #129691 - Flags: review?(rjc)
Comment on attachment 129691 [details] [diff] [review]
patch v2

What about lines 39-40 in pref-applications.js?  And I assume you tested these
changes, right?
Comment on attachment 129691 [details] [diff] [review]
patch v2

r=rjc@rjcdb.com
Attachment #129691 - Flags: review?(rjc) → review+
(Assignee)

Comment 11

14 years ago
Ah, you're right.  I had skipped 39-40, thinking that without it ref
might never be set, but of course it's in the xul.  I'll make the 
switch there as well, and re-test (code was previously tested as well).
(Assignee)

Comment 12

14 years ago
Created attachment 129826 [details] [diff] [review]
v3

This changes the other instance as well.  I've tested this, and it seems fine.
Attachment #129691 - Attachment is obsolete: true
(Assignee)

Comment 13

14 years ago
Comment on attachment 129826 [details] [diff] [review]
v3

Not sure if I need to get a fresh r from rjc or not.  Can't hurt, though.
Attachment #129826 - Flags: superreview?(bz-vacation)
Attachment #129826 - Flags: review?(rjc)
Comment on attachment 129826 [details] [diff] [review]
v3

Excellent.
Attachment #129826 - Flags: superreview?(bz-vacation) → superreview+
Comment on attachment 129826 [details] [diff] [review]
v3

r=rjc@rjcdb.com
Attachment #129826 - Flags: review?(rjc) → review+
(Assignee)

Comment 16

14 years ago
Comment on attachment 129826 [details] [diff] [review]
v3

Floating for approval, just in case.  The tabs menu in the sidebar currently
displays empty. As a result of an optimization to the setAttribute() code, an
old trick to force tree rebuilds no longer works. The patch replaces it with an
explicit rebuild() to fix the tabs menu, and cleans up the same issue in the
helper apps pref panel as well.
Attachment #129826 - Flags: approval1.5b?

Comment 17

14 years ago
Comment on attachment 129826 [details] [diff] [review]
v3

a=asa (on behalf of drivers) for checkin to 1.5beta
Attachment #129826 - Flags: approval1.5b? → approval1.5b+
(Assignee)

Comment 18

14 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 19

14 years ago
This still seems partially broken, since there are no checkmarks in the tabs
meny to distinguish those sidebars that are visible from those not visible at a
given moment.

There is just a menu that shows the available sidebars, but not showing the
status of the visibility of individual sidebars.

Asking for a reopening of this bug.

Comment 20

14 years ago
Forgot to mention my build, it is Mozilla Suite 2003082104.
(Assignee)

Comment 21

14 years ago
Yeah, I see that too.  Though I'm not completely convinced it's related --
manipulation of the checked attribute ought to be separate from whether or not
the content gets built.  

I've split it off into bug 217340.
Attachment #129691 - Flags: superreview?(bz-vacation)
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.