Closed
Bug 1071558
Opened 10 years ago
Closed 10 years ago
Middle and right click on search suggestions perform the search in the same tab on about:home/newtab
Categories
(Firefox :: Search, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox38 | --- | verified |
People
(Reporter: phorea, Assigned: shreyaslakhe, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
Reproducible on:
Firefox 33 beta 6 - 20140922173023
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
latest Aurora 34.0a2 - 20140922004004
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
latest Nightly 35.0a2 - 20140922030204
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Steps to reproduce:
1. Enter a string on about:home or about:newtab page
2. Move the mouse on suggestions drop-down list
3. Click on them using the mouse middle and right click buttons
Actual results:
Middle and right click perform the search on the same tab. The contextual menu is opened and remains on the screen when right click is used.
Expected results:
On search toolbox from navigation toolbar, middle click on search suggestions opens a new tab and right click has no action.
Notes: It also occurs under Mac OSX 10.9.5 and Ubuntu 12.10 32-bit.
Comment 2•10 years ago
|
||
It's not really a regression because that's how search suggestions on about:home/newtab have always worked -- it's just a bug. Should be very easy to fix. _onMousedown should make sure event.button == 0: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/searchSuggestionUI.js?rev=37f902fba2de#230
This test should also be updated: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_searchSuggestionUI.js?rev=37f902fba2de#99
I'm reversing the dependency relationships to reflect the fact that this was caused by bug 612453, which implemented these suggestions.
Mentor: adw
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Keywords: regression,
regressionwindow-wanted
Whiteboard: [good first bug]
Comment 3•10 years ago
|
||
Although hopefully bug 1066787 will be fixed soon, making this one unnecessary.
Updated•10 years ago
|
Whiteboard: [good first bug] → [good first bug][lang=js]
Hi, I would like to work in this bug? How should I go about it?
Thanks,
shreyas
Comment 5•10 years ago
|
||
Hi shreyas, thanks for volunteering!
First you'll need to set up your computer to build Firefox. Please see:
https://developer.mozilla.org/en-US/docs/Introduction
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide
Once you've built Firefox, you can start working on the bug.
I think all we have to do here is ignore right-clicks. I tested just now with middle-clicks, and they open the search results for the clicked suggestion in a new tab, like Petruta says, which I guess is fine behavior. (And on second thought, I don't think the test needs to check this.)
So you'll need to modify the _onMousedown method that I linked to in comment 2 to return early when the event button is a right-click. You can see which button value indicates a right-click here: https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent.button
Good luck, and let me know here in the bug when you have questions. You can also join #fx-team on IRC. I'm adw there. https://wiki.mozilla.org/IRC
Assignee: nobody → shreyaslakhe
Status: NEW → ASSIGNED
Comment 6•10 years ago
|
||
Er, I'm wrong, middle-click opens search results in the same tab -- which is actually what Petruta said.
If we want to open them in a new tab, which we should probably, this is more work. Currently the search suggestion controller doesn't forward any click info to clients, and neither of the two clients -- about:home and about:newtab -- are set up to open search results in a new tab.
So there are two parts to this: (1) Update _onMousedown to pass the click event to onClick, and (2) update about:home and about:newtab to open search results in a new tab when the click event is the middle button.
shreyas, about:home passes in its onClick to SearchSuggestionUIController here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/abouthome/aboutHome.js?rev=c8019c6eda14#368
onSearchSubmit already takes an event argument and calls preventDefault on it, which we should not do in this case. Therefore onSearchSubmit will need to take a second clickEvent argument and add clickEvent.button to the eventData object. The eventData object is eventually passed up from content to chrome, and ends up here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.js?rev=29b05d283b00#446 And then here: http://mxr.mozilla.org/mozilla-central/source/browser/modules/AboutHome.jsm?rev=5161e9b47a3c#181
So at that last link, you'll need to load the search URI in a new tab instead of in the current window.
Next, about:newtab passes its onClick to to SearchSuggestionUIController here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/search.js?rev=f1798d79a96f#246
You can see that its search method also takes an event and calls preventDefault, so you'll have to add a second parameter again: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/search.js?rev=f1798d79a96f#45
search.js then forwards the search event to chrome, where it ends up here: http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm
You'll need to modify _onMessageSearch to examine `data` to see which button, if any, was clicked, and then open a new tab in the middle-click case.
Updated•10 years ago
|
Points: 2 → 3
I tried to reproduce the bug I am not able to reproduce it.
Apparently the middle click opens in a new tab and the right click does nothing.
Flags: needinfo?(adw)
Comment 8•10 years ago
|
||
Really? For Petruta and me, middle- and right-click both open the search results page of the clicked suggestion in the current tab. I don't think there's a reason for that to be different for you. Are you sure you're doing it right? You go to about:home or about:newtab, type something in the search field in the page, and click one of the suggestions that pops up.
Flags: needinfo?(adw)
Comment 9•10 years ago
|
||
My experience is the same as Shreya's (see 2015-02-11 07:01:21 PST above). I am not a technical person just a heavy user. Perhaps those who are looking into this problem can fix a similar bug introduced in very recent Firefox versions. If profile option "When I open a link in a new tab, switch to it immediately" is selected and user middle clicks one of the search suggestions in the search bar, a new tab is opened but only in the background. The new tab should be opened (switched to) in the foreground. Thank you.
Assignee | ||
Comment 10•10 years ago
|
||
Reproduced the bug.
Getting a "clickEvent is undefined on this patch"
Attachment #8564721 -
Flags: feedback?(adw)
Comment 11•10 years ago
|
||
(In reply to shreyas from comment #10)
> Created attachment 8564721 [details] [diff] [review]
> bug1071558.diff
>
> Reproduced the bug.
> Getting a "clickEvent is undefined on this patch"
Shreyas: Will this fix the bug I documented above?
Assignee | ||
Comment 12•10 years ago
|
||
No it does not fix the bug. I just submitted it for feedback on the progress I made and the errors I was getting.
Comment 13•10 years ago
|
||
(In reply to shreyas from comment #12)
> No it does not fix the bug. I just submitted it for feedback on the progress
> I made and the errors I was getting.
Thanks, Shreyas. Hopefully this will be addressed by someone.
Comment 14•10 years ago
|
||
Comment on attachment 8564721 [details] [diff] [review]
bug1071558.diff
Review of attachment 8564721 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks shreyas, this is a good start. In addition to my comments below, to fix the problem where right-clicking triggers the suggestion to be loaded, you need to modify searchSuggestionUI.js like I said in comment 5:
> So you'll need to modify the _onMousedown method that I linked to in comment
> 2 to return early when the event button is a right-click. You can see which
> button value indicates a right-click here:
> https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent.button
::: browser/base/content/abouthome/aboutHome.js
@@ +298,4 @@
> }
> }
>
> +function onSearchSubmit(aEvent, clickEvent)
Sorry, I don't know what I was thinking when I said we need a second event argument. That's not true. We should call event.preventDefault() in the click case just like we do in other cases. So in the click case, `aEvent` should be the click event.
@@ +310,5 @@
>
> let eventData = {
> engineName: engineName,
> + searchTerms: searchTerms,
> + clickEventButton: clickEventButton,
Instead of passing the raw button like I suggested, let's pass a useNewTab bool. That's what the chrome code really needs to know. useNewTab should be true for the middle button and false for others.
::: browser/base/content/newtab/search.js
@@ +42,4 @@
> });
> },
>
> + search: function (event, clickEvent) {
Same here, no second click argument needed, sorry.
@@ +54,4 @@
> engineName: this.currentEngineName,
> searchString: searchStr,
> whence: "newtab",
> + clickEvent: clickEvent.button
Same here, pass a useNewTab bool instead.
::: browser/base/content/searchSuggestionUI.js
@@ +251,4 @@
> this.input.setAttribute("selection-kind", "mouse");
> this._hideSuggestions();
> if (this.onClick) {
> + this.onClick.call(event);
This is right, thanks. Please update the comment near the top of the file, here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/searchSuggestionUI.js#32
The description of onClick should say that the function is passed one argument, the click event.
::: browser/modules/AboutHome.jsm
@@ +198,4 @@
> #endif
> // Trigger a search through nsISearchEngine.getSubmission()
> let submission = engine.getSubmission(data.searchTerms, null, "homepage");
> + window.addTab.loadURI(submission.uri.spec, null, submission.postData);
You don't want to unconditionally open a new tab (or unconditionally use the same tab like the current code does). You added the clickEventButton property to the data (but please use useNewTab like I said), so you need to examine that property here to determine whether to use a new tab or the current tab.
You can see that there's a `data` object being used in the code in this function. That's where your clickEventButton/useNewTab property is.
To open a new tab, you can use this openUILinkIn method defined on `window`. The definition of openUILinkIn is here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js?rev=f6d678e3120b#191
Pass "tab" for `where` and false for aAllowThirdPartyFixup. And of course the submission URL and the post data.
::: browser/modules/ContentSearch.jsm
@@ +224,5 @@
> let win = browser.ownerDocument.defaultView;
> win.BrowserSearch.recordSearchInHealthReport(engine, data.whence,
> data.selection || null);
> + if(data == 0) {
> + window.addTabloadURI(submission.uri.spec, null, submission.postData);
The line where ContentSearch loads the URI in the current tab is here: http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm?rev=d9adce044082#214 There's a try-catch around the line. You want to keep that.
So around there is where you need to examine your useNewTab bool. Again, you can see that there's a `data` object. That's where useNewTab will be defined.
Opening a new tab in this case is different from the about:home case, but it's still simple. See this line here that gets the current browser: http://mxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm?rev=d9adce044082#212
If useNewTab is true, what we should do is redefine that `browser` variable to be a new browser, and then after the try-catch, we should make `browser` the currently selected tab.
To get a new browser, do:
browser.getTabBrowser().addTab().linkedBrowser
Set the return value to `browser`. Then the current code that opens the URL in `browser` will just work.
To make the new browser the currently selected tab, do:
browser.getTabBrowser().selectedTab = browser.linkedTab;
Attachment #8564721 -
Flags: feedback?(adw) → feedback+
Assignee | ||
Comment 15•10 years ago
|
||
Updated the patch, but there still seems to be some error.
Attachment #8564721 -
Attachment is obsolete: true
Attachment #8566196 -
Flags: feedback?(adw)
Comment 16•10 years ago
|
||
Comment on attachment 8566196 [details] [diff] [review]
bug1071558.diff
Review of attachment 8566196 [details] [diff] [review]:
-----------------------------------------------------------------
What's the error? I think this should work except for the stray onClick.
::: browser/base/content/abouthome/aboutHome.js
@@ +303,4 @@
> let searchText = document.getElementById("searchText");
> let searchTerms = searchText.value;
> let engineName = document.documentElement.getAttribute("searchEngineName");
> + let useNewTab = false;
Please move this definition inside the `if`.
@@ +307,5 @@
> if (engineName && searchTerms.length > 0) {
> // Send an event that will perform a search and Firefox Health Report will
> // record that a search from about:home has occurred.
> + if (aEvent) {
> + this.onClick.call(aEvent);
There's no onClick defined here. You don't need to call any click handler. Just stick useNewTab in eventData.
::: browser/base/content/searchSuggestionUI.js
@@ +32,5 @@
> * @param onClick
> * A function that's called when a search suggestion is clicked. Ideally
> * we could call submit() on inputElement's ancestor form, but that
> + * doesn't trigger submit listeners. The function is passed one argument,
> + the click event
Please remove the trailing space on the first line, add an asterisk to the front of the second line like all the other lines in this comment, and end the sentence with a period.
::: browser/modules/AboutHome.jsm
@@ +201,5 @@
> + if (data.useNewTab == true) {
> + window.openUILinkIn(submission.uri.spec, "tab", false, submission.postData);
> + }
> + else {
> + window.loadURI(submission.uri.spec, null, submission.postData);
Actually now that I see this, we can use openUILinkIn in both cases. In the same-tab case, pass "current" as the `where` argument. Then you only need a conditional, or better yet a ternary, to define `where`.
::: browser/modules/ContentSearch.jsm
@@ +206,4 @@
> "engineName",
> "searchString",
> "whence",
> + "useNewTab",
This shouldn't be a required property. If it's absent, then treat it as false.
@@ +210,5 @@
> ]);
> let engine = Services.search.getEngineByName(data.engineName);
> let submission = engine.getSubmission(data.searchString, "", data.whence);
> let browser = msg.target;
> + if (data.useNewTab == true) {
if (data.useNewTab) {
@@ +225,4 @@
> // method on it will throw.
> return Promise.resolve();
> }
> + if (data.useNewTab == true) {
if (data.useNewTab) {
Attachment #8566196 -
Flags: feedback?(adw) → feedback+
Assignee | ||
Comment 17•10 years ago
|
||
Updated the patch , but it doesn't produce the expected results. I guess there is something wrong with the assignment of useNewTab.
Attachment #8566196 -
Attachment is obsolete: true
Attachment #8566498 -
Flags: feedback?(adw)
Comment 18•10 years ago
|
||
Comment on attachment 8566498 [details] [diff] [review]
bug1071558.diff
Review of attachment 8566498 [details] [diff] [review]:
-----------------------------------------------------------------
I noticed a couple of problems just now that I didn't before.
One is how you're calling onClick as described below. The other is that we need to update the onClick function that about:newtab's passes to the SearchSuggestionUIController constructor. Right now it's passing a function that doesn't expect any arguments.
You need to change this line: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/newtab/search.js#247
To this:
event => this.search(event));
::: browser/base/content/abouthome/aboutHome.js
@@ +307,5 @@
> // Send an event that will perform a search and Firefox Health Report will
> // record that a search from about:home has occurred.
> + let useNewTab = false;
> + if (aEvent) {
> + if (aEvent.button == 1) {
Please write this like:
let useNewTab = aEvent && aEvent.button == 1;
::: browser/base/content/newtab/search.js
@@ +52,5 @@
> if (this.currentEngineName && searchStr.length) {
>
> + if (event) {
> + if (event.button == 1) {
> + useNewTab = true;
Same here, and please remember to indent properly since you're inside the conditional here.
::: browser/base/content/searchSuggestionUI.js
@@ +255,4 @@
> this.input.setAttribute("selection-kind", "mouse");
> this._hideSuggestions();
> if (this.onClick) {
> + this.onClick.call(event);
The first argument to `call` is the `this` argument that the method is bound to. So what this does is event.onClick(). You need to keep the null and pass `event` as the second argument. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/call
::: browser/modules/ContentSearch.jsm
@@ +224,5 @@
> // method on it will throw.
> return Promise.resolve();
> }
> + if (data.useNewTab) {
> + browser.getTabBrowser().selectedTab = browser.linkedTab;
I told you the wrong thing here, sorry. What you should do instead is save the return value of browser.getTabBrowser().addTab() above in a `newTab` variable. And then here on this line, do:
browser.getTabBrowser().selectedTab = newTab;
Attachment #8566498 -
Flags: feedback?(adw) → feedback+
Assignee | ||
Comment 19•10 years ago
|
||
I made the changes but the patch doesn't seem to be working for about:newtab. For about:newtab it opens a newtab but loads the url in the existing tab.
Flags: needinfo?(adw)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8566498 -
Attachment is obsolete: true
Attachment #8566701 -
Flags: feedback?(adw)
Comment 22•10 years ago
|
||
Comment on attachment 8566701 [details] [diff] [review]
bug1071558.diff
Review of attachment 8566701 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/base/content/abouthome/aboutHome.js
@@ +303,4 @@
> let searchText = document.getElementById("searchText");
> let searchTerms = searchText.value;
> let engineName = document.documentElement.getAttribute("searchEngineName");
> +
Please remove the trailing space here.
::: browser/base/content/newtab/search.js
@@ +48,4 @@
> }
> let searchText = this._nodes.text;
> let searchStr = searchText.value;
> + let useNewTab = event && event.button == 1;
Please move this definition inside the conditional.
::: browser/modules/ContentSearch.jsm
@@ +211,5 @@
> let submission = engine.getSubmission(data.searchString, "", data.whence);
> let browser = msg.target;
> + let newTab;
> + if (data.useNewTab) {
> + newTab = browser.getTabBrowser().addTab().linkedBrowser;
You need to set newTab to the return value of addTab(). addTab returns a tab; linkedBrowser is a browser.
Attachment #8566701 -
Flags: feedback?(adw) → feedback+
Assignee | ||
Comment 23•10 years ago
|
||
Still doesn't work for about:newtab. The same problem persists.
Attachment #8566701 -
Attachment is obsolete: true
Attachment #8566728 -
Flags: feedback?(adw)
Comment 24•10 years ago
|
||
Comment on attachment 8566728 [details] [diff] [review]
bug1071558.diff
Review of attachment 8566728 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/base/content/abouthome/aboutHome.js
@@ -303,4 @@
> let searchText = document.getElementById("searchText");
> let searchTerms = searchText.value;
> let engineName = document.documentElement.getAttribute("searchEngineName");
> -
Please revert this change.
::: browser/modules/ContentSearch.jsm
@@ +211,5 @@
> let submission = engine.getSubmission(data.searchString, "", data.whence);
> let browser = msg.target;
> + let newTab;
> + if (data.useNewTab) {
> + newTab = browser.getTabBrowser().addTab();
It doesn't work because now you're not setting browser to newTab.linkedBrowser anymore. You still need to do that.
Attachment #8566728 -
Flags: feedback?(adw) → feedback+
Assignee | ||
Comment 25•10 years ago
|
||
The patch works but I not able to revert the first change. Adding a newline and then doing a "hg qrefresh" doesn't seem to work.
Flags: needinfo?(adw)
Comment 26•10 years ago
|
||
I don't know what to tell you. Could you post the new patch? I'll take care of it.
Flags: needinfo?(adw)
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8566728 -
Attachment is obsolete: true
Attachment #8566959 -
Flags: feedback?(adw)
Updated•10 years ago
|
Attachment #8566959 -
Flags: feedback?(adw) → review+
Comment 28•10 years ago
|
||
Here's the patch I landed with my nits fixed.
Thanks shreyas! Congratulations on fixing two bugs. :-)
https://hg.mozilla.org/integration/fx-team/rev/2e9ff15fa9c7
Attachment #8566959 -
Attachment is obsolete: true
Attachment #8567234 -
Flags: review+
Comment 29•10 years ago
|
||
Shreyas and Drew: It looks like you both worked very diligently on these issues. I have no knowledge of anything you did, only to say how much I admire you abilities, not only technical, but also your patience and persistence and your ability to communicate well with each other. I was wondering if you could tell me if the bug I reported above is one of the two fixed. If so, could you estimate when it will be released in the product. And if you didn't get a chance to fix it, would you recommend the best way of reporting it. I did report it through Bugzilla but it was declared a duplicate of 1123442, but nobody seems to be working on that. So, is there a better way to submit bugs or should I submit it again. Anyway, thanks for all your work. I have used FF for many years and appreciate its many benefits.
Comment 30•10 years ago
|
||
Hi Peter, no, the patch in this bug did not fix bug 1123442. If you have already reported your bug and it was marked a duplicate, it would not be helpful to submit it again. Unfortunately there are way many more bugs than there are people to work on them, and it looks like bug 1123442 is not a priority for paid Mozilla staff right now, although it's still very new. Volunteers are always welcome to work on bugs though.
Comment 31•10 years ago
|
||
Thanks, Drew for the speedy response. I hear what you say. I guess I'm a bit frustrated because one of the reasons I like FF is the the duel awesome bar/search bar concept and something recently broke in the search bar that prevents middle clicking a search suggestion, opening a new tab, and switching to it immediately. This is a feature that worked well for years. I did find an add-on that helps called Omnibar. This replaces the awesome bar and supports search suggestions. So, my life goes on, but long term I'd rather not have add-ons if I don't need them and particularly when I know FF should and always use to support a feature that is now broken. By the way I don't really believe bug 1123443 is a duplicate of the one I reported. Similar, but different. Perhaps fixing 1123442 will fix my issue, I have no way of knowing. Meanwhile I'll remain patient, try not to annoy anyone, and hope eventually the issue will be resolved. Kindest regards.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Updated•10 years ago
|
Iteration: --- → 38.3 - 23 Feb
Updated•10 years ago
|
QA Contact: petruta.rasa
Reporter | ||
Comment 33•10 years ago
|
||
Verified Nightly 38.0a1 build under Ubuntu 12.04 LTS 32-bit, Windows 7 64-bit and Mac OS X 10.9.5 and noticed:
- Middle click opens the search page in a new tab in foreground - this is different from the search toolbar where the searches performed with middle click are opened in background tabs
- Right click on search suggestions now only opens the contextual menu of the page and dismisses the search suggestion list, while on older versions the searches were performed too
Reporter | ||
Comment 34•10 years ago
|
||
Drew, can you please check if the above issues worth filling? Thank you!
Flags: needinfo?(adw)
Comment 35•10 years ago
|
||
(In reply to Petruta Rasa [QA] [:petruta] from comment #33)
> - Middle click opens the search page in a new tab in foreground
Oh... we should have made it open in the background... I didn't realize that. Yes, could you file a new bug and mark it blocking this one, please?
> - Right click on search suggestions now only opens the contextual menu of
> the page and dismisses the search suggestion list, while on older versions
> the searches were performed too
That's the right behavior.
Flags: needinfo?(adw) → needinfo?(petruta.rasa)
Reporter | ||
Comment 36•10 years ago
|
||
Thanks a lot for responses, Drew!
I logged bug 1137234 and marking this one as verified.
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•