Closed Bug 229753 Opened 21 years ago Closed 20 years ago

search sidebar does not automatically open on first addressfield search

Categories

(SeaMonkey :: Search, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: asa, Assigned: doronr)

Details

(Keywords: smoketest, Whiteboard: has patch, needs review)

Attachments

(5 files, 4 obsolete files)

Tested with 1.7a trunk nightly build 2003123008 on Windows XP as well as a
122603 OS X build.

Steps to reproduce:
1. create a new profile.
2. ensure that "open the search tab in sidebar when search results are
available" is checked in the Internet Search preferences panel.
3. type a search term into the addressfield. 
4. click the search button or the search item at the bottom of the addressfield
popup.

Results:
Search results are loaded in the browser content area.

Expected Results: 
The browser sidebar should open to the search sidebar tab and should be
populated with search results.
This bug appears to be "fixed" for windows and even worse on Mac. Regardless of
the setting, Mac SeaMonkey will not open the search sidebar. 
Flags: blocking1.7b+
OS: All → MacOS X
Hardware: All → Macintosh
- for beta since we are out of time...
+ for final.  let's try and get this one then.
Flags: blocking1.7b-
Flags: blocking1.7b+
Flags: blocking1.7+
hrm. I'm wrong. It's still not fixed for windows and linux and mac is
misbehaving almost identically. tested with the 1.7 beta builds.
OS: MacOS X → All
Hardware: Macintosh → All
Could it be a duplicate of bug 116584 ?
If the sidebar has been opened and then closed, it will work from then on.

Wasn't there work done to prevent the sidebar from using so much resource at
startup?
bah, -> me
Assignee: search → doronr
Attached patch Make sure the sidebar is open (obsolete) — Splinter Review
The whole sidebar api is somewhat of a mess, so have to manually make sure the
sidebar is opened.  We can't just pass an id to a function (SidebarSelectPanel)
and let it do it all currently.
btw, I still think that the pref for this should be off by default - it is an
annoying feature.
Attachment #148214 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 148214 [details] [diff] [review]
Make sure the sidebar is open

What if the sidebar is already open but has a different panel selected? What if
the search panel isn't available?
What if the sidebar is already open but has a different panel selected? 
 - That works fine.

What if the search panel isn't available?
  - This is an issue I hadn't thought of when the sidebar hasn't been shown yet
(no DOM and all).  So if we never opened the sidebar (when this bug happens), we
can't know if the sidebar search panel is actually going to be shown as one of
the panels.  The only way would be to load the rdf ds and manually do the work,
or maybe init the sidebar but not show it.

Do we really want to do that?
I would be happy with initing the sidebar datasource etc. when a search page
comes in and triggers the sidebar, after all if the pref is on we probably need
to open the sidebar. If the search sidebar isn't active then you could prompt
the user to add it or disable the pref :-)

The patch looks as if it would close and open the sidebar if it was open to a
different panel.
"The patch looks as if it would close and open the sidebar if it was open to a
different panel."

Yeah, I fixed that by calling directly into the sidebar api to figure if it is
displayed or not.
Attachment #148214 - Attachment is obsolete: true
Attachment #148424 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 148424 [details] [diff] [review]
Checks the rdf ds to see if the search panel is enabled or not.

When I tried this it always opened the sidebar despite your best efforts to
prevent it when I didn't actually have a search sidebar to open :-(
CCing the RDF gurus in case they can see why doron's patch doesn't work for me.

Alternatively navigator.js could call sidebar_add_data_source() which would be
code factored out of sidebar_open_default_panel to build the template.
Hmm, it works for me if I uncheck the "search" sidebar from the "Tabs" dropdown
in sidebar.

Can you post your panels.rdf after you remove the search sidebar?
Attached file panels.rdf
Ok, can reproduce this on first time loading the browser witht he sidebar closed
and doing a search.
    var datasource = RDF.GetDataSource(sidebarObj.datasource_uri);
    var aboutValue = RDF.GetResource("urn:sidebar:panel:search");
    var property = RDF.GetResource("http://home.netscape.com/NC-rdf#exclude");

    var content = datasource.GetTarget(aboutValue, property, true);    

    if (content instanceof Components.interfaces.nsIRDFLiteral){
      searchPanelExists = (content.Value != "navigator:browser");
    } else {
      // no exclude property exists, but an search panel entry does
      searchPanelExists = true;
    }

What seems to happen is that content is null, and therefore we think that there
is no exclude property, even though there is one.  Running the code after the
sidebar is shown and hidden is successfull and returns the expected value.

I'll play with this on monday to figure out why it isn't finding the property
the first time.
so it appears to be asynch datasource loading.
Is there a way to make it sync or to create a listener to when its done?

http://lxr.mozilla.org/seamonkey/source/xpfe/components/sidebar/resources/sidebarOverlay.js#816
seems to do a timeout wait...
Attached patch Load the ds syncly (obsolete) — Splinter Review
GetDataSourceBlocking() is what I was looking for.
Attachment #148424 - Attachment is obsolete: true
Attachment #148669 - Flags: review?(neil.parkwaycc.co.uk)
Attached file panels.rdf
Attachment 148669 [details] [diff] does not work with this panels.rdf, the sidebar opens even
though there is no search panel.
Attached file panels.rdf
Attachment 148669 [details] [diff] does not work with this panels.rdf either.
I use the "ref" property to see if the template has been created, and rebuild
it blocking if necessary so that we can tell if the panel exists or not.
Attachment #148669 - Attachment is obsolete: true
Comment on attachment 148856 [details] [diff] [review]
fixes the issues.

This should hopefully do it, takes the cases into consideration.

Your solution is shorter :)
Attachment #148856 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 148856 [details] [diff] [review]
fixes the issues.

>+          searchPanelExists = (exclude.Value.indexOf("navigator:browser") < 0);
I'd prefer !/\bnavigator:browser\b/.test(exclude.Value)

>+    // make sure the sidebar is open, else SidebarSelectPanel() will fail
Isn't it the document.getElementById that fails?
Attachment #148856 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #148856 - Flags: superreview?(jag)
not going to block the release for this but we'd consider a fully reviewed patch
for the 1.7 branch if it happens before release.
Flags: blocking1.7+ → blocking1.7-
Comment on attachment 148856 [details] [diff] [review]
fixes the issues.

Wow, that's a lot of new code to do this. It looks like it's all necessary, but
should some of it live in their own functions (that could perhaps be shared)?

+  if (document.getElementById("urn:sidebar:panel:search")) {
+    var myPanel = document.getElementById("urn:sidebar:panel:search");

How about:

var searchPanel = document.getElementById("urn:siderbar:panel:search");
if (searchPanel) {
  ...
}

and then inside |if (searchPanelExists)| drop the |var| from |var searchPanel|.

sr=jag with that I guess.
Attachment #148856 - Flags: superreview?(jag) → superreview+
Indeed, the sidebar api is rather weak.  Which my rewrite fixes :)

Requesting 1.7 approval again.
Flags: blocking1.7- → blocking1.7?
Comment on attachment 148856 [details] [diff] [review]
fixes the issues.

a=asa (on behalf of drivers) for checkin to 1.7
Attachment #148856 - Flags: approval1.7+
Comment on attachment 148669 [details] [diff] [review]
Load the ds syncly

obsolete patch, two more are coming.
Attachment #148669 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #148424 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #148214 - Flags: review?(neil.parkwaycc.co.uk)
Attached patch Optimize and fix a small issue. (obsolete) — Splinter Review
There is a bug with the patch - opening the sidebar, adding the search sidebar
and closing it would break the patch.  This is due to:

document.getElementById("urn:siderbar:panel:search") being collapsed

and:

} else if (sidebarObj.never_built) {

would be false, as it has been built.  

So on top of fixing the small nits, I added a 3rd else:

else {
  // sidebar is hidden and has been built
    searchPanelExists =
!sidebarObj.panels.get_panel_from_id("urn:sidebar:panel:search").is_excluded();

}
Attachment #148856 - Attachment is obsolete: true
Attachment #149768 - Flags: superreview?(jag)
Attachment #149768 - Flags: review?(neil.parkwaycc.co.uk)
(In reply to comment #34)
>There is a bug with the patch - opening the sidebar, adding the search
>sidebar and closing it would break the patch.  This is due to:
> 
>document.getElementById("urn:siderbar:panel:search") being collapsed
Could you explain the steps needed to reproduce this, as I couldn't?
1. Open mozilla with the sidebar closed.
2. Open the sidebar with the search sidebar one of the tab panels shown
3. close the sidebar (f9)
4. Go to google and do a web search

The result with my previous patch is that the search sidebar does not open. 
Basically, the sidebar has been built, but is collapsed.
Marking blocking1.7+, although this is at risk for being minused, since we're
hoping to ship early next week.
Flags: blocking1.7? → blocking1.7+
jag and neil, we could really use your review and evaluation of this patch
quickly.  thanks --chofmann
Whiteboard: has patch, needs review
Comment on attachment 149768 [details] [diff] [review]
Optimize and fix a small issue.

Sorry, I still can't duplicate the problem, or determine what the extra code is
trying to solve.
I don't know if this helps, but I tested with fresh nightly build on Windows
2000 (otherwise sorry for spamming)
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/20040607

Steps to reproduce:
1. create a new profile.
2. ensure that "open the search tab in sidebar when search results are
available" is checked in the Internet Search preferences panel.
3. type a search term into the addressfield. 
4. click the search button or the search item at the bottom of the addressfield
popup.

Results:
1 Search sidebar opens with the search results
2 Google is openend in the browser window with the search results
Flags: blocking1.7+ → blocking1.7-
Comment on attachment 149768 [details] [diff] [review]
Optimize and fix a small issue.

Sorry, I don't know why my 1.7 branch build would have sidebarObj.never_built
be false and ocument.getElementById("urn:siderbar:panel:search") would fail.

I tried a fresh tree and the issue is gone, my bad.
Attachment #149768 - Attachment is obsolete: true
Attachment #148856 - Attachment is obsolete: false
Comment on attachment 149768 [details] [diff] [review]
Optimize and fix a small issue.

I guess you don't need my review any more then.
Attachment #149768 - Flags: review?(neil.parkwaycc.co.uk) → review-
Attachment #149768 - Flags: superreview?(jag)
Attachment #149768 - Flags: review-
Comment on attachment 148856 [details] [diff] [review]
fixes the issues.


Doron:
It seems that this patch was not checked in into the Trunk yet. (I don't know
about the v1.7 branch.)

Could you do it/both ?

Also, I believe 'has patch, needs review' could be removed from the Whiteboard
now !?
fixed on trunk.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
(In reply to comment #44)
> fixed on trunk.

Good :-)
Any plan/status about v1.7 branch ?
Product: Core → SeaMonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: