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)
SeaMonkey
Search
Tracking
(seamonkey2.7 fixed, seamonkey2.8 fixed)
RESOLVED
FIXED
seamonkey2.8
People
(Reporter: philip.chee, Assigned: philip.chee)
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
3.73 KB,
patch
|
philip.chee
:
review+
Callek
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
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"
Assignee | ||
Comment 1•12 years ago
|
||
Comment 2•12 years ago
|
||
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 3•12 years ago
|
||
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-
Assignee | ||
Comment 4•12 years ago
|
||
> 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 5•12 years ago
|
||
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?
Assignee | ||
Comment 6•12 years ago
|
||
>>+ 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.
Comment 7•12 years ago
|
||
(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 8•12 years ago
|
||
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+
Assignee | ||
Comment 9•12 years ago
|
||
>>+ 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+
Assignee | ||
Updated•12 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
status-seamonkey2.8:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.8
Assignee | ||
Comment 10•12 years ago
|
||
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 11•12 years ago
|
||
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-
Comment 13•12 years ago
|
||
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.
Assignee | ||
Comment 14•12 years ago
|
||
<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
Assignee | ||
Updated•12 years ago
|
status-seamonkey2.6:
fixed → ---
Assignee | ||
Comment 15•12 years ago
|
||
pushed to comm-aurora a=Callek over IRC: http://hg.mozilla.org/releases/comm-aurora/rev/7d46b8c526c9
status-seamonkey2.7:
--- → fixed
Comment 16•12 years ago
|
||
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.
Description
•