15176 - [FEATURE] Search context menu item for selected text

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, enhancement, P2)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

[ note to self so I don't forget this. ]

Possibly: add context menu items to navigator that allows user to:

a) open selection as a query in the smartsearch window (e.g. select "cats and
dogs" on a webpage, context-click, and select "Search for [cats and dogs...]"

b) open the selection as a link (it is common for people to insert the URL of a
webpage, but neglect to make it an HTML link, and thus subject the viewer to the
task of copy-pasting it into the location bar. A smarter web browser would allow
user to do this with less effort)

Comment 1

[don]

Re-evaluate after beta 1.

Comment 2

[Asa Dotzler [:asa]]

*** Bug 25799 has been marked as a duplicate of this bug. ***

Comment 3

[Matthew T (active 1999-2002)]

Ideally, this would be extensible. I have bookmarklets in 4.5 set up to do what 
this bug is describing (using document.getSelection()), for
* dictionary definition
* Google search
* Bugzilla bug number.

However this is just my personal set of selection processors, and if this is 
reimplemented using a context menu, I should be able to add/remove others from 
that menu.

You also run the risk of the link context menu getting very crowded. It's got 13 
items in it already, and about 7 of those are what I would call essential. 
Adding an arbitrary number of other items to this menu could make it quite ugly.

Comment 4

[gabriel]

Since I was the one who submitted 25799, I'd like to request at least b) to be
implemented. Being able to do other things with selections sounds fantastic as
well. Ugly menus would be a small price to pay ;-).

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

not a priority, pushing out as far as possible.

Comment 6

[don]

Move to "Future" milestone.

Comment 7

[gabriel]

Now that Mozilla has scrolling menus, this wouldn't look as ugly.

Comment 8

[Jesse Ruderman]

A good way to make this customizable would be to make a special folder in 
bookmarks.  When you right-click a selection, the context menu would include 
each of the items on that menu.

Comment 9

[Jesse Ruderman]

*** Bug 60615 has been marked as a duplicate of this bug. ***

Comment 10

[Jesse Ruderman]

Copying more useful summary from dup bug 60615.

Comment 11

[Jesse Ruderman]

Another thing that would be useful on the context menu is view partial source 
(bug 49721).

Comment 12

[Jesse Ruderman]

*** Bug 17751 has been marked as a duplicate of this bug. ***

Comment 13

[Paul Chen]

nav triage team:

Don't think we'll get to this for beta1, too many other things to fix, marking 
nsbeta1-

Comment 14

[Matthew T (active 1999-2002)]

*** Bug 64454 has been marked as a duplicate of this bug. ***

Comment 15

[Alec Flett]

reassigning to me because ben hasn't shown much interest.

Comment 16

[Paul Chen]

Adding myself and Matt to cc-list.

Comment 17

[gabriel]

Please change OS and platform to ALL.

Comment 18

[gabriel]

OK, bugzilla let me do it myself :-) Apologies for spam.

Comment 19

[matt]

From german:
This does google search in 4.x .   For selected.
We would like it also for any word clicked on.

javascript:docSel=document.getSelection();if(!docSel){void(docSel=prompt('Enter
a search term\n\nTip: you may also use this with selected Text in a
Browser window.',''))};if(docSel){document.location='
http://www.google.com/search?q='+escape(docSel);} <
javascript:docSel=document.getSelection();if(!docSel){void(docSel=prompt(&apos;E
nter
a search term\n\nTip: you may also use this with selected Text in a
Browser
window.&apos;,&apos;&apos;))};if(docSel){document.location=&apos;
http://www.google.com/search?q=&apos;+escape(docSel);}>

Comment 20

[gabriel]

But can we have a function to search for user entered text within the source ?
For me that would be by far the most useful option.

Comment 21

[rbs]

*** Bug 78182 has been marked as a duplicate of this bug. ***

Comment 22

[gabriel]

Sorry I just realised my last comment was attached to the wrong bug. The 
feature I really wanted here was to highlight text and jump to it as a link. 
Would it be possible to see if selected text starts with something 
like 'http://' or 'ftp://' and then add the option 'jump to as link' to the 
context menu ?

Comment 23

[Gavin Jackson]

Is there really a need for that enhancement and why?

Comment 24

[Alec Flett]

like any feature, because someone thought it was a good idea. bugzilla is not
the forum to justify bugs, so let's keep this discussion on how we're going to
actually implement this feature.

Comment 25

[gabriel]

Well, otherwise how else do you jump to a link in a page, if that link is not an
anchor - answer: double click in the location bar, right click, delete; select
text, right click, copy, click in location bar, right click, paste, press enter.

10 operations when it could be reduced to 3 (select text, right click, jump to
link).

Note that this was suggestion b) of the original poster of this enhancement.

Comment 26

[Jesse Ruderman]

It's already possible to jump to a URL in 5 steps: select url, copy, Ctrl+L 
(Moz) or Alt+D (IE), paste, enter.  That doesn't necessarily mean it's not 
worth it to include a "jump to url" option on the selection context menu, 
though.

Comment 27

[rbs]

This bug has been in the system since Sept 1999... Someone could iterate from 
the comments of "[e-mail] 2001-03-12 22:22", and use newURI()
http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIIOService.idl

If newURI() succeeds on the selection, that would mean the selected sting
is a valid URI, and from there a "Go to..."  could be added to the context menu.

Regexp for other fancy things like "Mail to..." could be attempted if 
newURI() fails.

And the JS code for the context menu:
http://lxr.mozilla.org/seamonkey/search?string=contextmenu

Comment 28

[rbs]

A newcomer could start by following bug 32358 "changes to context menu for 
mailtos".

Comment 29

[Jesse Ruderman]

From bug 9274: Another option that would be nice to have on the selection
context menu is "Open Links in New Windows" or "Open Selected Links".

Comment 30

[Boris Zbarsky [:bzbarsky]]

*** Bug 88518 has been marked as a duplicate of this bug. ***

Comment 31

[Alec Flett]

reassign to default owner

Comment 32

[sairuh (rarely reading bugmail)]

arrgh! i keep entering stuff in the wrong field.<apologies/>

Comment 33

[matt]

=> matt

Comment 34

[matt]

Attachment: patch for search

Comment 35

[matt]

Above is a patch for search.
Make regex or URI to find urls and dispatch that page
leave all others the rest as search.

Comment 36

[matt]

Attachment: new patch to include trimming off extra white space

Comment 37

[matt]

Patch above supports search.  Passing teh selected text to openSearch in 
navigator.js to call the search function.

1. Image is not included because there is not an easy way at the moment to 
create a dynamic image attached to a menuitem.

2.This if one highlights a text and it is a URL then it should update the Open 
in new Window...
I believe this would make more sense to the user. If this was the case then we 
would have to add code to the Open in new Window menu command.  This would be 
done in the 

setTarget : function command.

We either should open a new bug for the functionality or make a new bug for the 
patch i created.


Morse, since your the context menu owner I think i need a review from you.
thanks.

Comment 38

[Stephen P. Morse]

+    isTextSelected : function() {
+       var result = false;
+       if (_content.getSelection() != "") {
+          var searchSelect = document.getElementById('context-searchselect'); 
+             searchSelect.setAttribute("label", "Search for \'" + ...
+          var result = true;
+       }
+       return result;
+    },

1. Remove the "var" on the second "var result", otherwise you will have two 
different result variables and you will be assigning "true" to the wrong one.

2. (nit) fix the indentation on the "searchSelect" line

With those two changes, r=morse

Comment 39

[Bill Law]

There's some localization issues with that code.  The label string "Search for
%s" should go in a .dtd/.properties file and the text placed into it, no?

Also, won't the context menu really suck if the text is at all long?  I think it
should be truncated at some reasonable length and an ellipsis appended.  E.g.,
    Search for 'some really big selection...'

Comment 40

[matt]

Attachment: updated patch

Comment 41

[matt]

1. extra var gone
2. no tabs....fixed editor to stop making them when i dont' want them.
3. Localized in props file.  What was I thinking.
4. The popup takes care of the fact that the string is to long it will cut it
off at something like 20 or so chars.  But none the less I added code to only
allow 15 chars and then if it has more i add "..." .  I didn't localize ... but
I can if someone thinks I should.
Thanks.

Comment 42

[Bill Law]

Comment on attachment 53206 [details] [diff] [review]
updated patch

r=law

I didn't realize that the popup would truncate automatically, but I think the ellipsis is a nice touch anyway and I don't think it needs localization (I hope not, I think I've hardcoded some of those myself).

Comment 43

[Stephen P. Morse]

Glad to see that you got rid of the unused label on context-searchselect
r=morse

Comment 44

[David Hyatt]

sr=hyatt

Comment 45

[Keyser Sose]

This going to get checked in now that the tree is open?

Comment 46

[matt]

fixed

Comment 47

[Jesse Ruderman]

Since this bug became restricted to adding "search" to the selection context 
menu, I've opened bug 105580 for continued discussion.

Comment 48

[sairuh (rarely reading bugmail)]

vrfy'd fixed using 2001.11.08.0x-comm bits on linux rh6.2, winnt and mac os
10.1. even nicely strips prepended whitespace and compresses multiple internal
whitespaces in the selection. :)

however, i noticed that this feature is missing in context menus for frames
--see bug 109171.

marlon, here's "another additional context menu item" to consider wrt the spec. :)

Comment 49

[Samir Gehani]

Reopening to complete feature to spec.  For reference see: 
<http://www.mozilla.org/projects/ui/communicator/browser/search/>

Comment 50

[Samir Gehani]

Attachment: Add icon and engine name to search context menu item to match spec.

Comment 51

[Samir Gehani]

morse, please r.
alecf, please sr.

Comment 52

[Stephen P. Morse]

Could you please post a diff with -w so I can see the real changes and not be 
lost in all the whitespace changes.  Thanks.

Comment 53

[Alec Flett]

Comment on attachment 60697 [details] [diff] [review]
Add icon and engine name to search context menu item to match spec.

very cool, but isn't this going to be slow, to retrieve the icon every time the
menu pops up? seems like you can cache the value the first time the menu pops
up, and then watch for the pref to change.

Comment 54

[sairuh (rarely reading bugmail)]

->samir

adding perf kw in case implementation of this does result in slowness...

Comment 55

[Samir Gehani]

Steve: sure, but then be aware that my tabbing will look like it's off.  It
actually isn't because I'm fixing the 3 char tab in these search-related
functions to align with the file-style 4 char tab.  Funny you ask cause I almost
attached my diff -w.  Anyways, will comply with the request next revision.

Alec: will look into your suggestion.

Next patch will also include the fix for the feature missing in context menus in
frames which is sitting in my tree (bug 109171).  

Comment 56

[Samir Gehani]

Alec,
The gContextMenu gets destroyed during the "onpopuphiding" event.  So I can
create my own parallel global, gDefaultEngine, which is an nsIObserver and is
filled in the first time text is selected and we have to show the menu item. 
Only, now the gDefaultEngine will last for the life of the browser session
(i.e., till mozilla is killed) and so adds to bloat.  The bloat is tiny but
would you prefer this to getting the icon each time (my first implementation). 
I run on a 1.5GHz machine so it is hard for me to tell if the context menu is in
fact slowed down (my suspicion is that it isn't since the datasource is in
memory).  I would lean towards the first implementation but have the second
almost ready to go.

Comment 57

[Samir Gehani]

Attachment: Patch with Alec's pref-change observer approach (also diff -w per Steve's request).

Comment 58

[Samir Gehani]

morse/alecf, please r/sr latest patch.  Thanks.

Comment 59

[Stephen P. Morse]

Comment on attachment 60743 [details] [diff] [review]
Patch with Alec's pref-change observer approach (also diff -w per Steve's request).

r=morse

Comment 60

[Rune Skaug]

Attachment: Selection including #10#13 and image w/o alt-tag

Attaching context menu with "funny" content. Is this how it's supposed to be?
The text will also contain the alt-text of an image if one exist.

Comment 61

[Rune Skaug]

Build id 2001120603.

Comment 62

[Samir Gehani]

Rune,
I can't reproduce your bug with a current build on Win2K.  Please open a new bug
report and include the URL and your system info.  Let's continue the
investigation in the new bug report.  Thanks. 

Comment 63

[Rune Skaug]

Samir,

Fair enough. I filed bug 114615.

Comment 64

[Daniel Veditz [:dveditz]]

Comment on attachment 60743 [details] [diff] [review]
Patch with Alec's pref-change observer approach (also diff -w per Steve's request).

sr=dveditz

A comment block delimiting the nsDefaultEngine class would make things clearer.
At first I didn't realize I'd wandered into a constructor and thought "this"
still referred to the context menu.

Comment 65

[Blake Ross]

Note that this menuitem already visibly slows down the opening of the context
menu when you have all text selected (without the icon addition).

Comment 66

[Samir Gehani]

Checked in.

Comment 67

[sairuh (rarely reading bugmail)]

this feature has been checked in (some time ago :).