Last Comment Bug 414849 - Support middle-click paste and go behaviour on search icon
: Support middle-click paste and go behaviour on search icon
Status: ASSIGNED
[good first bug][fxsearch]
:
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: Trunk
: All Linux
: P4 enhancement 45 with 2 votes (vote)
: Future
Assigned To: John C. McCabe-Dansted
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-30 07:25 PST by John C. McCabe-Dansted
Modified: 2015-09-23 09:13 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
A preliminary path, middle clicking on the search-go-container will search for the selection (659 bytes, application/octet-stream)
2009-03-23 05:55 PDT, John C. McCabe-Dansted
no flags Details
Draft patch so that middle click on the Search Button initiates search for selected text. (1.79 KB, text/plain)
2009-03-23 11:12 PDT, John C. McCabe-Dansted
no flags Details
Patch: Search for contents of selection buffer when middle clicking search icon. (1.85 KB, patch)
2011-03-26 21:21 PDT, John C. McCabe-Dansted
no flags Details | Diff | Review
Search for contents of selection buffer when middle clicking search icon. (3.25 KB, patch)
2011-04-10 03:46 PDT, John C. McCabe-Dansted
no flags Details | Diff | Review
Search for contents of selection buffer when middle clicking search icon (3.33 KB, patch)
2011-04-10 08:55 PDT, John C. McCabe-Dansted
gavin.sharp: review-
Details | Diff | Review
Search for contents of selection buffer when middle clicking search icon (3.55 KB, patch)
2011-04-11 07:11 PDT, John C. McCabe-Dansted
no flags Details | Diff | Review

Description John C. McCabe-Dansted 2008-01-30 07:25:17 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008013004 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008013004 Minefield/3.0b3pre

On Linux we can middle click the icon on the URL bar to load a URL. However we cannot perform a search by middle clicking the icon in the search bar. This is inconsistent.

Also, together with bug 359546 this means that there is no easy way to do a search on the contents of the middle-click buffer. This can be quite annoying if you have carefully selected an error message in the console and notice that there is something in the search bar. Then you can't just select the text in the search bar and delete it because your message is in the selection buffer rather than the clipboard. Whenever you select text it overwrites the selection buffer.

This bug has exists also in Firefox 2 and Firefox 3.0b2.

Reproducible: Always

Steps to Reproduce:
1. Drag over some text
2. Middle click on the icon in the search bar

Actual Results:  
Nothing

Expected Results:  
I would expect that middle clicking the icon in the Search bar would do a search on the contents of the middle-click  buffer.
Comment 1 John C. McCabe-Dansted 2008-07-27 07:46:10 PDT
Still occurs in Ubuntu 8.04 with all updates.
See also: http://brainstorm.ubuntu.com/idea/7111/
Comment 2 John C. McCabe-Dansted 2009-03-22 06:09:50 PDT
This still occurs in Firefox 3.1 Beta 2.
Comment 3 John Vivirito 2009-03-22 07:08:28 PDT
Can someone pelase mark this as a duplicate of bug 359546 if it is indeed a duplicate.
John is this more of a feature request or a bug?
ubuntu bug is at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.0/+bug/292675
Comment 4 John C. McCabe-Dansted 2009-03-22 07:43:51 PDT
It is not exactly a duplicate. Bug 359546 has to do with middle clicking an empty area. This bug has to do with middle-clicking the icon next to search bar. 

I consider this as a (minor) UI bug: middle click has one behavior with the icon to the left of the URL bar, but does nothing on the icon to the left of the search bar. This seems inconsistent to me. 

Even if the feature (easy way to search for selection buffer) was availiable in some other way, it would seem to make sense to fix this inconsistency, although the importance of the bug would drop.

If it is not possible to fix this "bug" then I guess I'd submit a feature request. However it seems like it would have a one line fix. I've had a look at urlbarBindings.xml to see if I could add something like "<![CDATA[if(middle-click){(searchBar.doSearch(selectionBuffer,?)){" to the definition of the searchbar icon but don't seem to understand the infrastructure of Firefox well enough to do this. I'm not even sure urlbarBindings.xml is the right place to look.
Comment 5 Henrik Skupin (:whimboo) 2009-03-22 16:18:51 PDT
Ryan, is it something which could be easily implemented?
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-03-22 18:13:12 PDT
Sure, just need to add a click handler similar to PageProxyClickHandler(), and call code similar to the code in middleMousePaste().
Comment 8 John C. McCabe-Dansted 2009-03-23 05:55:27 PDT
Created attachment 368882 [details]
A preliminary path, middle clicking on the search-go-container will search for the selection

I am working on this now. FYI, here is the current patch. This almost fixes this bug, but it you have to middle-click on the icon to the right of the search bar instead of to the left. This makes a kind of sense since this button is the one you left-click on to search for the contents of the text area. However on the URL bar you click on the button to the left. I'll have another patch in within a week.
Comment 9 John C. McCabe-Dansted 2009-03-23 11:12:05 PDT
Created attachment 368923 [details]
Draft patch so that middle click on the Search Button initiates search for selected text.

Some questions.

Is the try/catch when updating history of any use in EngineButtonClickHandler? PageProxyClickHandler has it, but handleSearchCommand. (Also why is one of these method names a noun and the other a verb? should EngineButtonClickHandler be a verb?

Is the "textBox.value = textValue;" line a good Idea. This line helps consitancy as the URL bar replaces the current text with the selection. However when searching the text box in the webpage will display the search terms anyway. And being able to search without nuking the contents of textBox.value may be handy.

The line "event.stopPropagation();" does not seem to be necessary for correctness and increases the number of LOC and the size of the diff. Should I remove this.

Since this code is just bloat on MS Windows, should I encapsulate it all in "#ifdef XP_UNIX"?
Comment 10 John C. McCabe-Dansted 2011-03-26 21:21:07 PDT
Created attachment 522177 [details] [diff] [review]
Patch: Search for contents of selection buffer when middle clicking search icon.

I have updated this patch for the latest hg version (roughly ff4.0)
Comment 11 John C. McCabe-Dansted 2011-03-26 21:27:16 PDT
As I understand, if you want to search for the contents of the selection buffer in Firefox the easiest way is still as follows:
1) Click inside the search box. (being very careful not to select and text)
2) Switch back from the mouse to the keyboard.
3) Hold down the backspace for about 10 seconds or however long is required to clear out whatever rubbish may be sitting in the search bar.
4) Switch back to the mouse
5) Middle click inside the search bar
6) Click on the search icon.

Compare this to e.g. Google-Chrome
1) Middle-click the New Tab [+] icon.

Since I do a lot of searches, I find that this on bug (missing feature if you will) is enough to keep me on chrome. I'll go to #developers to see if I can get this reviewed. I don't have the latest mozilla sources on this machine but I have tested it by patching the omni.jar that comes with the firefox binaries.

It appears to be the right approach. If we instead fixed Bug 359546 we may find that the user accidentally pressed the middle mouse button when scrolling, navigating them away from the page (and perhaps losing the contents of forms). Middle clicking on the New Tab icon as in Chrome could be a bit ambiguous since Firefox has both a search and a URL bar. This patch not only provides an easy way of searching for the selection buffer it also makes the UI more consistent as the URL bar behaves similarly to the behavior this patch provides for the search bar.
Comment 12 John C. McCabe-Dansted 2011-03-29 19:16:37 PDT
BTW, with this patch you can click on the binoculars to the right to search in a new tab (as with chrome) or click on the icon to the left of the search bar to search in the current tab (similar to the URL bar). Also I have put the changes within a #ifdef XP_UNIX so they shouldn't affect non Unix users at all.
Comment 13 Ed Morley [:emorley] 2011-04-09 06:17:33 PDT
If it helps, hg log for search.xml shows dao/gavin might be good candidates for the review.

However, the patch isn't currently exported correctly (not enough context, not taken from m-c root etc) - this is something I've just had to figure out, and found the following useful:
- https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3f
- http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Hope that helps! :-)
Comment 14 John C. McCabe-Dansted 2011-04-10 03:46:21 PDT
Created attachment 524937 [details] [diff] [review]
Search for contents of selection buffer when middle clicking search icon.

OK, I have remade the patch using hg qnew as suggested. I have tested that it doesn't break the build, and the Firefox built from source runs correctly. I have only tested on Linux, but every modification is inside a #ifdef XP_UNIX, so I don't see how it could cause regressions on other platforms.
Comment 15 Ed Morley [:emorley] 2011-04-10 04:00:40 PDT
Comment on attachment 524937 [details] [diff] [review]
Search for contents of selection buffer when middle clicking search icon.

Marking review?dao, which should at least get you visibility; even if dao isn't the correct person.
Comment 16 Dão Gottwald [:dao] 2011-04-10 07:28:48 PDT
(In reply to comment #14)
> have only tested on Linux, but every modification is inside a #ifdef XP_UNIX,
> so I don't see how it could cause regressions on other platforms.

XP_UNIX is set on OS X. So depending on whether this should apply in a hypothetical Qt build, you should either exclude XP_MACOSX or use MOZ_WIDGET_GTK2 instead of XP_UNIX.
Comment 17 John C. McCabe-Dansted 2011-04-10 08:55:01 PDT
Created attachment 524956 [details] [diff] [review]
Search for contents of selection buffer when middle clicking search icon

This feature is useful on systems that use an X Server as the native graphical environment so it is useful on Linux and BSD but not Windows or MacOS (regardless of the toolkit used). Note that the suggestion at:
  https://developer.mozilla.org/en/Mac_OS_X_Build_Prerequisites
to use
  #if defined(XP_UNIX) && !defined(XP_MACOSX)
does not seem to work as I get:
  Preprocessor.Error: ('/mnt/btrfs/xp/src/firefox_hg/browser/components/search/content/search.xml', 82, 'SYNTAX_ERR', 'defined(XP_UNIX) && !defined(XP_MACOSX)')
Perhaps mozilla uses a non-standard preprocessor for xml files. Anyway I have decided to use a functional but somewhat ugly two line #ifdef instead. I have also set the reviewer to "dao" for this revision of the patch. Hmm, does this require ui-review as well as review?

I have built and tested this new revision of the patch on Linux.
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-04-10 14:51:47 PDT
Comment on attachment 524956 [details] [diff] [review]
Search for contents of selection buffer when middle clicking search icon

Thanks for the patch, John!

A couple of preliminary comments:
- You can use something along the lines of UNIX_BUT_NOT_MAC (as in e.g. http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#45 ) that's more descriptive (also means there's only one place to change if we want to add/remove this behavior from some platforms). MIDDLEMOUSE_PASTE_ENABLED, or somesuch.
- you can avoid duplicating most of handleSearchCommand by adding an aValue parameter that it prefers over the textbox value, and having the click handler call it and pass the clipboard value in (after checking event.button)

(Just for future reference: the preprocessor used for XUL/JS/CSS is indeed different than the normal C/C++ preprocessor - it's a custom preprocessor written in Python http://mxr.mozilla.org/mozilla-central/source/config/Preprocessor.py ).
Comment 19 John C. McCabe-Dansted 2011-04-11 07:11:34 PDT
Created attachment 525068 [details] [diff] [review]
Search for contents of selection buffer when middle clicking search icon

I am not sure why we'd pass in the clipboard value since the function knows to get that it self if button == 1. I have merged the functions by passing in "where" instead. Does this look about right?
Comment 20 Dão Gottwald [:dao] 2011-04-11 08:08:14 PDT
(In reply to comment #19)
> Created attachment 525068 [details] [diff] [review]
> Search for contents of selection buffer when middle clicking search icon
> 
> I am not sure why we'd pass in the clipboard value since the function knows to
> get that it self if button == 1. I have merged the functions by passing in
> "where" instead. Does this look about right?

It looks like handleSearchCommand could just set where = "current" depending on aEvent.button == 1.
Comment 21 John C. McCabe-Dansted 2011-04-11 08:36:05 PDT
It could do, but then it would presumably open the search in the current tab regardless of whether the user clicks on the icon to the left or the right of the search bar. In the current implementation clicking on the icon to the right opens the search in a new tab, similar to Google Chrome. This can be useful if you switch to the browser and do not want to clobber whatever the last tab you opened had. It also roughly resembles the behavior of the URL bar in firefox, which also opens a new tab if you middle click to the right (on the refresh button).
Comment 22 John C. McCabe-Dansted 2011-07-14 06:20:54 PDT
OK, this seems to have stalled discussing the UI. Does this need a ui-review?
Comment 23 Liz Henry (:lizzard) (needinfo? me) 2013-05-30 17:26:57 PDT
John, are you still working on this bug? I'm going through older "good first bugs" that have been inactive for a while. 
Gavin, is this something you can help John to update? It looks like this just slipped through the cracks unfortunately!
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-10-31 12:23:49 PDT
Happy to help with advice for anyone who wants to pick this up.

Note You need to log in before you can comment on or make changes to this bug.