116523 - Search selected context menu item missing in XHTML as XML framesets, right-click uses deprecated method document.getSelection()

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[K Chayka]

win32 build 2001122103, win98se

This shows in the js console after right-clicking on a link, image, or
highlighted text:

Deprecated method document.getSelection() called.  Please use
window.getSelection() instead.

Comment 1

[Dimitrios]

Confirming on same build/OS. Appeared with this build, I think.

Comment 2

[sairuh (rarely reading bugmail)]

hm, should this go to selection...?

Comment 3

[Henrik Gemal]

http://lxr.mozilla.org/mozilla/source/xpfe/communicator/resources/content/nsContextMenu.js#636

Comment 4

[Peter Trudelle]

->blaker?

Comment 5

[sairuh (rarely reading bugmail)]

.

Comment 6

[John Morrison]

-> sgehani added the above in rev 1.48 of nsContextMenu.js

Comment 7

[Samir Gehani]

The fix for this should take care of the symtoms in bug 117805 as well.

Comment 8

[Morten Nilsen]

made a fix for this

Comment 9

[Morten Nilsen]

Attachment: prposed fix

Comment 10

[Morten Nilsen]

this could very well be a dupe of bug 117805, but I'm leaving it as is for a while.

Comment 11

[Samir Gehani]

I doubt this patch works in documents with framesets so unless you propose to
get to the right fix before me (I've targetted mozilla0.9.9) please reassign
this back to me.  

Comment 12

[Morten Nilsen]

well.. I just tested this patch with a frameset..
located here: http://home.no.net/blan01/

and the context menu displayed the right items...
if you give me some urls to test against, I'll do that...

Comment 13

[Samir Gehani]

Actually, all the context menu items are not displayed.  The |Search Google for
"<selected text>"| menu item (of course your default search engine will show in
place of Google) does not display cause window.getSelection() doesn't return the
text selected in the frame.  But thanks for forcing me to test and confirm
comment 11.  Comment 11 still holds so please advise accordingly whether I
should take this bug.  Thanks.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Morten Nilsen, no that patch is not correct for the same reason the patch which
I attached on bug 117805 is not correct either.  It's a workaround, not the fix.

The reason we are using getSelection in the first place is for getting the
selected text.  Highlight some text, and then right click and you will see a
item that says something to the effect of "Search this page for 'selected text'"

The reason we switched to this.target.ownerDocument.getSelection() is because of
it not working in framesets.  The only reason i attached the patch on bug 117805
is because the search feature not working on XHTML frameset docs is much better
no context menus at all in XML documents.  This patch was meant to only fix a
console error, and didn't take into consideration the feature which is behind this.

If anything this patch should be obsoleted by the patch on bug 117805.  That
patch is intended for 0.9.8 branch maybe.  I don't know if it will get checked
into the trunk.  That patch at least allows for HTML framesets to work with the
search selected text feature, but XHTML as XML frameset docs won't work still
(although context menus will).  This nor the patch on bug 117805 is the proper
fix.  The one on 117805 is better, but let's get the real fix in.

If you still feel you want to own this bug and fix it the real way, go ahead.

Marking this bug as blocking 117805.

Comment 15

[Morten Nilsen]

I have never seen any context menu item with search in it.
neither before nor after making this patch...

but I'm giving it back to Samir...
I don't think I'll be able to do anything more about this.

Comment 16

[Samir Gehani]

Morten, 
The search context menu item only shows up when you highlight (i.e., select)
text and then right click (which is a discoverability glitch that needs
addressing by always leaving the item visible and only enabling it when text is
selected).

Comment 17

[Samir Gehani]

Spilling over into mozilla1.0.

Comment 18

[Tuukka Tolvanen (sp3000)]

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

Comment 19

[rossi]

is there no way to get the window of the document? i fixed this bug by coding a
frameset-walker funtion, which searches the frameset for the window which
contains the document where the right click happened... is this too stupid or
should i attach that?

Comment 20

[rossi]

Attachment: proposed patch

Fabian told me a magic thing... "__parent__" great.. changing that one line
removes the warning...

Comment 21

[rossi]

Attachment: proposed patch v2

this is even better... as we don't need the workaround any more... (at least
that's how i understood this) and i checked with the attachment in bug 117805
which works fine too... (even in a frameset) so... this obsoletes attachment
71923 [details] [diff] [review] ...but i can't do that...

Comment 22

[rossi]

Attachment: proposed patch v3

new version because i just tested with a xul document and that throws an error
that this.target has no properties... that's true also for the the current
state.
sorry for the patch mess but it never worked in xul since the if was
inserted... and i didn't know that...
this patch works with xhtml frames/noframes, html frames/noframes and xul and
never throws any warning or error

Comment 23

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 24

[Samir Gehani]

Comment on attachment 72039 [details] [diff] [review]
proposed patch v3

r=sgehani

Comment 25

[Samir Gehani]

[e-mail],
You need a super-review and approval from [e-mail].  I can checkin
the patch once you have that.  See: <http://www.mozilla.org/hacking/reviewers.html>.

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 72039 [details] [diff] [review]
proposed patch v3

This won't work.  It causes more warnings than we start with on XHTML+XML
frameset documents.

Select and right-click on any of the frames here to test.
http://www.returnzero.com/frameset.xhtml

Comment 27

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: The Real Fix

Use |document.commandDispatcher.focusedWindow| instead of what we were using.
Man, I sure wish I had known about this when I did the fix for 117805.	This is
definitely what we need.

Tested that the 'Search Netscape Search for "your selection"...' feature works
on HTML documents, HTML frameset documents, XML documents, and XML fraeset
documents.  No strict warnings appear at all, either.

<QA>
HTML document: (use this page)
HTML frameset document: http://www.usahistory.com/frames.htm
XML document: http://bugzilla.mozilla.org/showattachment.cgi?attach_id=63292
XNK frameset document: http://www.returnzero.com/frameset.xhtml
</QA>

Comment 28

[rossi]

Comment on attachment 74451 [details] [diff] [review]
The Real Fix

looks great =)

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

Taking to seek super review and approval.  This fix also enables the search
selected text context menu (bug 15176) in XHTML as XML frameset documents, which
was previously not working.  Updating summary.

Comment 30

[jag (Peter Annema)]

Comment on attachment 74451 [details] [diff] [review]
The Real Fix

sr=jag

Comment 31

[Asa Dotzler [:asa]]

Comment on attachment 74451 [details] [diff] [review]
The Real Fix

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 32

[Christopher Aillon (sabbatical, not receiving bugmail)]

cvs commit -m "Bug 116523, Search selected text context menu missing in XHTML as
XML frameset docs, strict warning on right click.  r=[e-mail], sr=jag,
a=asa" nsContextMenu.js 
Checking in nsContextMenu.js;
/cvsroot/mozilla/xpfe/communicator/resources/content/nsContextMenu.js,v  <-- 
nsContextMenu.js
new revision: 1.59; previous revision: 1.58
done

Attachment 74451 [details] [diff] checked in on trunk.

Comment 33

[sairuh (rarely reading bugmail)]

vrfy'd fixed using 2002.04.09 comm bits on linux rh7.2, win2k, mac 10.1.3.
tested with Christopher's tests in comment 27.