search sidebar does not automatically open on first addressfield search

RESOLVED FIXED

Status

--
minor
RESOLVED FIXED
15 years ago
10 years ago

People

(Reporter: asa, Assigned: doronr)

Tracking

({smoketest})

Trunk
smoketest
Bug Flags:
blocking1.7b -
blocking1.7 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: has patch, needs review)

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
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

Comment 2

15 years ago
- 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+
(Reporter)

Comment 3

15 years ago
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 ?

Comment 5

15 years ago
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?
(Assignee)

Comment 6

15 years ago
bah, -> me
Assignee: search → doronr
(Assignee)

Comment 7

15 years ago
Created attachment 148214 [details] [diff] [review]
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.
(Assignee)

Comment 8

15 years ago
btw, I still think that the pref for this should be off by default - it is an
annoying feature.
(Assignee)

Updated

15 years ago
Attachment #148214 - Flags: review?(neil.parkwaycc.co.uk)

Comment 9

15 years ago
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?
(Assignee)

Comment 10

15 years ago
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

15 years ago
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.
(Assignee)

Comment 12

15 years ago
"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.
(Assignee)

Comment 13

15 years ago
Created attachment 148424 [details] [diff] [review]
Checks the rdf ds to see if the search panel is enabled or not.
Attachment #148214 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #148424 - Flags: review?(neil.parkwaycc.co.uk)

Comment 14

15 years ago
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

15 years ago
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.
(Assignee)

Comment 16

15 years ago
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

15 years ago
Created attachment 148511 [details]
panels.rdf
(Assignee)

Comment 18

15 years ago
Ok, can reproduce this on first time loading the browser witht he sidebar closed
and doing a search.
(Assignee)

Comment 19

15 years ago
    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

15 years ago
so it appears to be asynch datasource loading.
(Assignee)

Comment 21

15 years ago
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...
(Assignee)

Comment 22

15 years ago
Created attachment 148669 [details] [diff] [review]
Load the ds syncly

GetDataSourceBlocking() is what I was looking for.
Attachment #148424 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #148669 - Flags: review?(neil.parkwaycc.co.uk)

Comment 23

15 years ago
Created attachment 148821 [details]
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

15 years ago
Created attachment 148822 [details]
panels.rdf

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

Comment 25

15 years ago
Created attachment 148838 [details] [diff] [review]
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.
(Assignee)

Comment 26

15 years ago
Created attachment 148856 [details] [diff] [review]
fixes the issues.
Attachment #148669 - Attachment is obsolete: true
(Assignee)

Comment 27

15 years ago
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 28

15 years ago
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+
(Assignee)

Updated

15 years ago
Attachment #148856 - Flags: superreview?(jag)
(Reporter)

Comment 29

15 years ago
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 30

15 years ago
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+
(Assignee)

Comment 31

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

Requesting 1.7 approval again.
Flags: blocking1.7- → blocking1.7?
(Reporter)

Comment 32

15 years ago
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)

Updated

15 years ago
Attachment #148424 - Flags: review?(neil.parkwaycc.co.uk)

Updated

15 years ago
Attachment #148214 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 34

15 years ago
Created attachment 149768 [details] [diff] [review]
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();

}
Attachment #148856 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #149768 - Flags: superreview?(jag)
Attachment #149768 - Flags: review?(neil.parkwaycc.co.uk)

Comment 35

15 years ago
(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?
(Assignee)

Comment 36

15 years ago
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+

Comment 38

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

Updated

15 years ago
Whiteboard: has patch, needs review

Comment 39

15 years ago
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

15 years ago
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
(Reporter)

Updated

15 years ago
Flags: blocking1.7+ → blocking1.7-
(Assignee)

Comment 41

15 years ago
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
(Assignee)

Updated

15 years ago
Attachment #148856 - Attachment is obsolete: false

Comment 42

15 years ago
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-
(Assignee)

Updated

15 years ago
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 !?
(Assignee)

Comment 44

15 years ago
fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 15 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.