229753 - search sidebar does not automatically open on first addressfield search

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect)

Description

[Asa Dotzler [:asa]]

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.

Comment 1

[Asa Dotzler [:asa]]

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. 

Comment 2

[chris hofmann]

- for beta since we are out of time...
+ for final.  let's try and get this one then.

Comment 3

[Asa Dotzler [:asa]]

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.

Comment 4

[Serge Gautherie (:sgautherie)]

Could it be a duplicate of bug 116584 ?

Comment 5

[Mike Kaply [:mkaply]]

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?

Comment 6

[Doron Rosenberg (IBM)]

bah, -> me

Comment 7

[Doron Rosenberg (IBM)]

Attachment: Make sure the sidebar is open

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.

Comment 8

[Doron Rosenberg (IBM)]

btw, I still think that the pref for this should be off by default - it is an
annoying feature.

Comment 9

[[e-mail]]

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?

Comment 10

[Doron Rosenberg (IBM)]

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?

Comment 11

[[e-mail]]

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.

Comment 12

[Doron Rosenberg (IBM)]

"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.

Comment 13

[Doron Rosenberg (IBM)]

Attachment: Checks the rdf ds to see if the search panel is enabled or not.

Comment 14

[[e-mail]]

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 :-(

Comment 15

[[e-mail]]

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.

Comment 16

[Doron Rosenberg (IBM)]

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?

Comment 17

[[e-mail]]

Attachment: panels.rdf

Comment 18

[Doron Rosenberg (IBM)]

Ok, can reproduce this on first time loading the browser witht he sidebar closed
and doing a search.

Comment 19

[Doron Rosenberg (IBM)]

    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.

Comment 20

[Axel Hecht]

so it appears to be asynch datasource loading.

Comment 21

[Doron Rosenberg (IBM)]

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...

Comment 22

[Doron Rosenberg (IBM)]

Attachment: Load the ds syncly

GetDataSourceBlocking() is what I was looking for.

Comment 23

[[e-mail]]

Attachment: panels.rdf

Attachment 148669 [details] [diff] does not work with this panels.rdf, the sidebar opens even
though there is no search panel.

Comment 24

[[e-mail]]

Attachment: panels.rdf

Attachment 148669 [details] [diff] does not work with this panels.rdf either.

Comment 25

[[e-mail]]

Attachment: Different approach

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.

Comment 26

[Doron Rosenberg (IBM)]

Attachment: fixes the issues.

Comment 27

[Doron Rosenberg (IBM)]

Comment on attachment 148856 [details] [diff] [review]
fixes the issues.

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

Your solution is shorter :)

Comment 28

[[e-mail]]

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?

Comment 29

[Asa Dotzler [:asa]]

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.

Comment 30

[jag (Peter Annema)]

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.

Comment 31

[Doron Rosenberg (IBM)]

Indeed, the sidebar api is rather weak.  Which my rewrite fixes :)

Requesting 1.7 approval again.

Comment 32

[Asa Dotzler [:asa]]

Comment on attachment 148856 [details] [diff] [review]
fixes the issues.

a=asa (on behalf of drivers) for checkin to 1.7

Comment 33

[Simon Paquet [:sipaq]]

Comment on attachment 148669 [details] [diff] [review]
Load the ds syncly

obsolete patch, two more are coming.

Comment 34

[Doron Rosenberg (IBM)]

Attachment: Optimize and fix a small issue.

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();

}

Comment 35

[[e-mail]]

(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?

Comment 36

[Doron Rosenberg (IBM)]

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.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Marking blocking1.7+, although this is at risk for being minused, since we're
hoping to ship early next week.

Comment 38

[chris hofmann]

jag and neil, we could really use your review and evaluation of this patch
quickly.  thanks --chofmann

Comment 39

[[e-mail]]

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.

Comment 40

[Thomas]

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

Comment 41

[Doron Rosenberg (IBM)]

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.

Comment 42

[[e-mail]]

Comment on attachment 149768 [details] [diff] [review]
Optimize and fix a small issue.

I guess you don't need my review any more then.

Comment 43

[Serge Gautherie (:sgautherie)]

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 !?

Comment 44

[Doron Rosenberg (IBM)]

fixed on trunk.

Comment 45

[Serge Gautherie (:sgautherie)]

(In reply to comment #44)
> fixed on trunk.

Good :-)
Any plan/status about v1.7 branch ?