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

VERIFIED FIXED

Status

SeaMonkey
UI Design
VERIFIED FIXED
17 years ago
14 years ago

People

(Reporter: K Chayka, Assigned: Christopher Aillon (sabbatical, not receiving bugmail))

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

17 years ago
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

17 years ago
Confirming on same build/OS. Appeared with this build, I think.
Status: UNCONFIRMED → NEW
Ever confirmed: true
hm, should this go to selection...?
Assignee: pchen → trudelle

Comment 4

17 years ago
->blaker?
Assignee: trudelle → blaker
.
Assignee: blaker → blaker

Comment 6

17 years ago
-> sgehani added the above in rev 1.48 of nsContextMenu.js
Assignee: blaker → sgehani

Comment 7

17 years ago
The fix for this should take care of the symtoms in bug 117805 as well.
Priority: -- → P2
Target Milestone: --- → mozilla0.9.9

Comment 8

17 years ago
made a fix for this
Assignee: sgehani → Morten

Updated

17 years ago
Status: NEW → ASSIGNED

Comment 9

17 years ago
Created attachment 64477 [details] [diff] [review]
prposed fix

Comment 10

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

Comment 11

17 years ago
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

17 years ago
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

17 years ago
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.
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.
Blocks: 117805

Comment 15

17 years ago
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.
Assignee: Morten → sgehani
Status: ASSIGNED → NEW

Comment 16

17 years ago
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

17 years ago
Spilling over into mozilla1.0.
Target Milestone: mozilla0.9.9 → mozilla1.0
*** Bug 126775 has been marked as a duplicate of this bug. ***

Comment 19

17 years ago
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

17 years ago
Created attachment 71923 [details] [diff] [review]
proposed patch

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

Comment 21

17 years ago
Created attachment 72023 [details] [diff] [review]
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

17 years ago
Created attachment 72039 [details] [diff] [review]
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

16 years ago
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 adt@netscape.com.  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.
Target Milestone: mozilla1.0 → mozilla1.2

Comment 24

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

r=sgehani
Attachment #72039 - Flags: review+

Updated

16 years ago
Attachment #64477 - Attachment is obsolete: true

Updated

16 years ago
Attachment #71923 - Attachment is obsolete: true

Updated

16 years ago
Attachment #72023 - Attachment is obsolete: true

Comment 25

16 years ago
rossi@telnet.at,
You need a super-review and approval from drivers@mozilla.org.  I can checkin
the patch once you have that.  See: <http://www.mozilla.org/hacking/reviewers.html>.
Assignee: sgehani → rossi
Keywords: patch, review
Priority: P2 → --
Target Milestone: mozilla1.2 → ---
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
Attachment #72039 - Flags: needs-work+
Created attachment 74451 [details] [diff] [review]
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>
Attachment #72039 - Attachment is obsolete: true

Comment 28

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

looks great =)
Attachment #74451 - Flags: review+
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.
Assignee: rossi → caillon
Summary: right-click uses deprecated method document.getSelection() → Search selected context menu item missing in XHTML as XML framesets, right-click uses deprecated method document.getSelection()

Comment 30

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

sr=jag
Attachment #74451 - Flags: superreview+

Comment 31

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

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #74451 - Flags: approval+
cvs commit -m "Bug 116523, Search selected text context menu missing in XHTML as
XML frameset docs, strict warning on right click.  r=rossi@telnet.at, 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.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.