Closed Bug 705452 Opened 13 years ago Closed 12 years ago

BrowserSearch.loadSearch() uses the current search engine if the search sidebar is selected even if the sidebar itself is hidden.

Categories

(SeaMonkey :: Search, defect)

defect
Not set
normal

Tracking

(seamonkey2.7 fixed, seamonkey2.8 fixed)

RESOLVED FIXED
seamonkey2.8
Tracking Status
seamonkey2.7 --- fixed
seamonkey2.8 --- fixed

People

(Reporter: philip.chee, Assigned: philip.chee)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

STR:
Set your default search engine to Google.
Open the sidebar and select the search sidebar.
Select Yahoo as the search engine.
Close the sidebar.
Use the context menu search to search for some text.

Actual results:
Context menu item says 'Search Google for "blahblahblah"'
But actually searches using Yahoo.

Expected results:
The search engine in the context menu item and the search performed should match.

BrowserSearch.loadSearch() does this:

    if (isElementVisible(this.searchBar) ||
        isElementVisible(this.searchSidebar))
      engine = Services.search.currentEngine;
    else
      engine = Services.search.defaultEngine;

But the context menu isTextSelection() method does this:

    var engineName = "";
    if (window.BrowserSearch && isElementVisible(BrowserSearch.searchBar))
      engineName = Services.search.currentEngine.name;
    else
      engineName = Services.search.defaultEngine.name;

Note: isElementVisible(this.searchSidebar) returns true even if the sidebar is closed as long as the selected sidebar panel is the Search Sidebar.

Regression from Bug 410613: SeaMonkey should support "OpenSearch"
Attached patch Patch v1.0 Proposed Fix. (obsolete) — Splinter Review
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #580349 - Flags: review?(neil)
It seems to be slightly more complicated than that; I don't see the context menu label respond to the change of search engine in the sidebar whether or not it is open, but the actual search does either way.
Comment on attachment 580349 [details] [diff] [review]
Patch v1.0 Proposed Fix.

It seems to me that you have a choice of approaches:
* You could keep this.searchSidebar referring to the textbox, but only return it if the panel is visible, rather than simply whether it exists. You could optimise the callers because they no longer need to do the visibility check.
* You could change this.searchSidebar return the sidebar panel, so that the caller still checks its visibility, but then you need to update the focusing code.
Unfortunately it looks as if you have combined the worst of both worlds :-(
Attachment #580349 - Flags: review?(neil) → review-
Attached patch Patch v1.1 Better fix. (obsolete) — Splinter Review
> It seems to be slightly more complicated than that; I don't see the context
> menu label respond to the change of search engine in the sidebar whether or
> not it is open, but the actual search does either way.

> It seems to me that you have a choice of approaches:
> * You could keep this.searchSidebar referring to the textbox, but only return
> it if the panel is visible, rather than simply whether it exists. You could
> optimise the callers because they no longer need to do the visibility check.
I've taken this approach.
Attachment #580349 - Attachment is obsolete: true
Attachment #582683 - Flags: review?(neil)
Comment on attachment 582683 [details] [diff] [review]
Patch v1.1 Better fix.

>+    return !sidebar_is_hidden() && panel && panel.is_selected() &&
Does it work to use isElementVisible(panel) && instead?
>>+    return !sidebar_is_hidden() && panel && panel.is_selected() &&
> Does it work to use isElementVisible(panel) && instead?
Probably. But I don't think it makes sense if the panel is visible in the sidebar but some other panel is selected since the selected search engine won't be visible to the user.
(In reply to from comment #5)
> (From update of attachment 582683 [details] [diff] [review])
> >+    return !sidebar_is_hidden() && panel && panel.is_selected() &&
> Does it work to use isElementVisible(panel) && instead?
Actually I had confused the panels object with the DOM node with a similar id.
And you're right in that the DOM node with which I had confused it is visible.
Comment on attachment 582683 [details] [diff] [review]
Patch v1.1 Better fix.

>+    return !sidebar_is_hidden() && panel && panel.is_selected() &&
Need to check whether the sidebar is collapsed, too. r=me with that fixed.

(Maybe panel && isElementVisible(panel.get_iframe()) would work.)
Attachment #582683 - Flags: review?(neil) → review+
>>+    return !sidebar_is_hidden() && panel && panel.is_selected() &&
> Need to check whether the sidebar is collapsed, too. r=me with that fixed.
Fixed.

> (Maybe panel && isElementVisible(panel.get_iframe()) would work.)
Yes this works correctly for the case of a collapsed sidebar.

Pushed:
http://hg.mozilla.org/comm-central/rev/04e8dbba80bc
Attachment #582683 - Attachment is obsolete: true
Attachment #583094 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.8
Comment on attachment 583094 [details] [diff] [review]
Patch v1.2 as checked in. r=Neil

Request approval for comm-aurora (SeaMonkey 2.7) regression fix. Fallout from switch to OpenSearch.
Attachment #583094 - Flags: approval-comm-aurora?
Comment on attachment 583094 [details] [diff] [review]
Patch v1.2 as checked in. r=Neil

lets land this after the migration today, and I'll be sure to take it in our first beta though.
Attachment #583094 - Flags: approval-comm-beta+
Attachment #583094 - Flags: approval-comm-aurora?
Attachment #583094 - Flags: approval-comm-aurora-
Pushed to comm-beta.
Err Philip, I think Callek meant 2.7b1 ("first beta", cf. comment 11), i.e. push to beta *after* today's merge (again, see comment 11). Otherwise the denied Aurora approval wouldn't make sense... I think you need to back it out.
<RattyAway>	Callek: urk did I misunderstand you? https://bugzilla.mozilla.org/show_bug.cgi?id=705452#c13
<Callek>	RattyAway: yes, but I also don't think backout is worth it, it will just become abandoned with the uplift today
pushed to comm-aurora a=Callek over IRC:
http://hg.mozilla.org/releases/comm-aurora/rev/7d46b8c526c9
Comment on attachment 583094 [details] [diff] [review]
Patch v1.2 as checked in. r=Neil

Decided to have this land on aurora (found out our uplift was a bit away)...
Attachment #583094 - Flags: approval-comm-beta+
Attachment #583094 - Flags: approval-comm-aurora-
Attachment #583094 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.