Closed
Bug 48575
Opened 24 years ago
Closed 23 years ago
window.getSelection() broken
Categories
(Core :: DOM: Editor, defect, P1)
Core
DOM: Editor
Tracking
()
VERIFIED
FIXED
mozilla0.9
People
(Reporter: adamlock, Assigned: mjudge)
References
Details
(Keywords: access, regression, Whiteboard: FIXINHAND)
Attachments
(3 files)
290 bytes,
patch
|
Details | Diff | Splinter Review | |
17.12 KB,
patch
|
Details | Diff | Splinter Review | |
5.07 KB,
patch
|
Details | Diff | Splinter Review |
With reference to this Porkjockeys thread: news://news.mozilla.org/3992F952.EA96F89D%40iol.ie nsIDOMSelection needs to be xpidl'ized instead of idlc + simplified
Comment 1•24 years ago
|
||
Also need to break up nsIDOMSelection to hide non-public bits (Hint-related stuff) and general cleanup.
Comment 3•24 years ago
|
||
is 46847 really dependent on this bug? Looking at the last entries in that bug states that they are almost done with that one.
Comment 4•24 years ago
|
||
Yes, we have to fix this before the emedding APIs are frozen, since part of nsIDOMSelection needs to be a public interface.
Comment 5•24 years ago
|
||
This bug was split out of bug 46847 because we said we'd do this part. That doesn't mean it isn't an important embedding API.
Comment 7•24 years ago
|
||
Simon/Akkana -- if you can help Mike get this resolved, that would be great.
Whiteboard: [nsbeta3+]
Target Milestone: --- → M18
all set to go just have 1 or 2 string questions for scott collins when i can get ahold of him.
Status: NEW → ASSIGNED
Whiteboard: [nsbeta3+] → [nsbeta3+]FIXINHAND
for better or worse. its in ;)
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 10•24 years ago
|
||
can someone verify this one please?
Comment 12•24 years ago
|
||
Seems like the change that went in as a fix for this bug (without module owner approval, btw) broke more than it fixed, unfortunately. The change was to make nsIDOMSelection a XPIDL interface and as a result of this change the return type of window.getSelection() was changed from 'Selection' (a IDLC interface) to 'xpidl nsISelection' (an XPIDL interface). The result of this change is that no web content can use window.getSelection() because web content doesn't have access to XPConnect, and that's not acceptable. The reason window.getSelection() was added was to allow web content access to the range(s) in the current selection. To my knowledge there are to ways to solve this problem. 1) Backing out the nsIDOMSelection --> nsISelection change. 2) Making nsSelection implement nsISecurityCheckedComponent Option 1 is probably the easiest way to fix this problem, option 2 would make this specific XPConnected component usable from web content but this change requires a thorough security review of the component that implements nsISecurityCheckedComponent. Reopening, nominating for rtm and cc:ing myself and mstoltz (for possible comments on the security aspects of option 2) PDT: the fix for this bug caused a regression that makes window.getSelection() useless, window.getSelection() is the only way web content can access and modify the ranges in the current selection on a web page, this must be fixed for RTM.
Status: VERIFIED → REOPENED
OS: Windows NT → All
Priority: P3 → P1
Hardware: PC → All
Resolution: FIXED → ---
Whiteboard: [nsbeta3+]FIXINHAND → [nsbeta3+]
Comment 13•24 years ago
|
||
Mike, please follow the checkin rules that were sent out last week -- get a patch to fix the problems, get it super-reviewed, get module owner approval. The reviewer and module owner must make an entry in the bug, once all of that is done, remove the NEED INFO in the rtm+ block. I will then pop it to pdt for approval.
Whiteboard: [nsbeta3+] → [nsbeta3+][p:1][rtm+ NEED INFO]
Target Milestone: M18 → M19
Comment 14•24 years ago
|
||
Rather than actually backing out, if we have to go back to dom idl, I hope we can still keep the current API and just move it from xpidl to dom idl, since there were some problems with the old API (like ToString failing, causing JS errors).
Comment 15•24 years ago
|
||
Converting the existing API to IDLC sounds good to me.
Comment 16•24 years ago
|
||
If you want to use XPConnect (using nsISecurityCheckedComponent) instead of DOM idl, that's perfectly fine, as long as we do a security review.
Assignee | ||
Comment 17•24 years ago
|
||
ok what does nsISecurityCheckedComponent do and what do I have to do to get it to work?
Status: REOPENED → ASSIGNED
Comment 18•24 years ago
|
||
Jband, care to explain what it takes to make nsSelection.cpp implement nsISecurityCheckedComponent?
Comment 19•24 years ago
|
||
I'm not sure how this particular component is used - and shoot me now! that file is almost 7000 lines - but I'll try... The idea is that the component implements nsISecurityCheckedComponent. When script tries to access the component the security manager will try to QI the component to the nsISecurityCheckedComponent interface and if successful will call methods in that interface to ask if content script callers ought to be able to do what they are trying to do: make a new wrapper around the component, call a methods, get/set a property. You get passed the iid of the interace they are trying to access ('cuz your component may implement many interfaces) and you get passed the name of the method (or property) they are after. You can ignore the name if you are going to give the same answer for all methods on the interface. You return a string of 'AllAccess' or 'NoAccess' to indicate your answer. Since this is an 'out' string you need to clone one to pass back. Do you want to allow scripting of all of the methods? That is easiest. You can look at the other places it was used... http://lxr.mozilla.org/seamonkey/ident?i=nsISecurityCheckedComponent Also, you might ask vidur, mccabe, or mstoltz for detail - they wrote this while I was away and I've only used it once - and that was for a component that was itself written in JS.
Comment 20•24 years ago
|
||
It's probably easier to just move nsISelect back to IDLC.
Assignee | ||
Comment 21•24 years ago
|
||
ok i have the implementation of nsISecurityCheckedComponent on the class nsTypedSelection(formerly nsDOMSelection). it listens for the security checks for the iid of nsISelection. if someone asks for nsISelection they are returned AllAccess. Anything else gets NoAccess. how can i test this out? anyone have a test case allready?
Assignee | ||
Comment 22•24 years ago
|
||
removing CCs. not sure why they were added automatically sorry for the extra noise.
Assignee | ||
Comment 23•24 years ago
|
||
Assignee | ||
Comment 24•24 years ago
|
||
just modified wrong bug added bad attachment i will try to undo my damage. readding CC's and will look for the bug this patch file WAS meant for ;)
Comment 25•24 years ago
|
||
Now we just need to make sure that a malicious scripter can't use nsISelection to get to any other functionality. Can we meet and look this over?
Assignee | ||
Comment 26•24 years ago
|
||
ok sure. when do you want to do that? also did you have a test case for me? if what I did didnt work this is kinda a moot point.
Comment 27•24 years ago
|
||
Adding "window.getSelection()" to summary.
Summary: API review change: Port nsIDOMSelection to xpidl → API review change: Port nsIDOMSelection to xpidl (window.getSelection() broken)
Comment 28•24 years ago
|
||
removing + per pdt sw rules
Whiteboard: [nsbeta3+][p:1][rtm+ NEED INFO] → [nsbeta3+][p:1][rtm NEED INFO]
Comment 29•24 years ago
|
||
mstoltz: what is the procedure for getting the security review here? mjudge: please attach your diff.
Comment 30•24 years ago
|
||
No particular procedure. I (or jband, or someone else) has to review nsISelection and make sure a scripter can't use it to get to any other interfaces or any other bad behavior. Post your diff and I'll take a look. Are you committed to xpidl'izing this interface, or does the option remain for returning it to dom idl?
Assignee | ||
Comment 31•24 years ago
|
||
No there is no option to go back at this point. there was way to many changes to simply go back if all we need to do is make this thing a nsISecutiuryCheckedComponent. whether they get to it through the dom or not its the same implementation. here is the patch. i also changed the name of nsDOMSelection to nsTypedSelection
Assignee | ||
Comment 32•24 years ago
|
||
Comment 33•24 years ago
|
||
The patch looks good to me. I was worried about the security implications of this change, but nsISelection in xpidl should not be able to do anything that it wasn't able to to as part of the DOM, right mjudge? r=mstoltz.
Comment 34•24 years ago
|
||
Restoring rtm+ so this gets looked at by pdt today.
Whiteboard: [nsbeta3+][p:1][rtm NEED INFO] → [nsbeta3+][p:1][rtm+]
Comment 35•24 years ago
|
||
We need a better description of what the RTM problem is in order to consider [rtm++]. If mjudge was doing porkjockey API cleanup, and broke something, back it out. The API cleanup is going to be rtm- now, and the patch is a lot of code change, considering that no user impact is presented, other than the get/setSelection point. Marking [rtm need info]
Whiteboard: [nsbeta3+][p:1][rtm+] → [nsbeta3+][p:1][rtm need info]
Assignee | ||
Comment 36•24 years ago
|
||
Yes this change allows nothing the dom interface allowed anyway. This was allot of work to get this api split up and made into xpidl. we are so close to doing this perfectly and regression free. as far as I know this was the ONLY regression from about 50 files of changes. The reason the patch looks bigger than it should is because i was trying to change the name of a static class in that method from nsDOMSelection (which doesnt make any sense anymore) to nsTypedSelection. other than that this is a very simple implementation of a simple interface. 1. safe and reviewed 2. security impact non-existant since it WAS exposed in the DOM anyway from before. 3. window.getSelection needs to be implemented. 4. amount of changes really for a STATIC class name change to something more understandable. (by static i mean no one outside of this .cpp can even see this implementation so it is a safe local fix) btw. please dont talk about me in the third person when i am the one assigned the bug please. if there is to be a backing out of my code i would like to be talked about it first hand. thanks
Assignee | ||
Comment 37•24 years ago
|
||
changing summary. i am assuming that it is pdt that marks things rtm-. marking rtm+ i guess so that they can mark this -.
Summary: API review change: Port nsIDOMSelection to xpidl (window.getSelection() broken) → (window.getSelection() broken)
Whiteboard: [nsbeta3+][p:1][rtm need info] → [rtm+]
Comment 38•24 years ago
|
||
Mike, sorry to talk to you in the third person, didn't mean to be irritate you. On this bug, there still isn't an explanation of why users care, and we're not taking API cleanup on the branch right now. Marking [rtm-]
Whiteboard: [rtm+] → [rtm-]
Comment 39•24 years ago
|
||
Web developers care because window.getSelection() allows for a bookmarklet implementation of "View Partial Source" (bug 49721). Normal users are more likely to use document.getSelection(), for example when using the Google Search bookmarklet.
Comment 40•24 years ago
|
||
FYI, in PR3/M18, document.getSelection() throws a warning saying that document.getSelection() is deprecated in favor of window.getSelection(). Is the intention for these two method versions to be operationally interoperable as far as scripters are concerned?
Comment 41•24 years ago
|
||
I was wondering why I wasn't seeing that warning message :) I finally found it in the javascript console (it doesn't show up in the -console console for some reason).
Comment 42•24 years ago
|
||
Remove bogus () from summary
Summary: (window.getSelection() broken) → window.getSelection() broken
Comment 44•24 years ago
|
||
This bug needs fixing for accessibility work I'm doing. I need to be able to speak the current text position.
Severity: normal → blocker
Keywords: access
Assignee | ||
Comment 45•24 years ago
|
||
ok will check into trunk soonest.
Comment 46•24 years ago
|
||
(Apply to "Why users care"): Being able to create a range out of a user selection is a very important functionality. Just having a string of text is not enough if the exact start and end nodes/offsets are required. My case: I am an on-line learning developer and I would like my students to be able to highlight text on the page with a faint color (like a highlighter), and save their highlights to restore them later. To be able to do that, I need to know the range of the selection, so I can add a <span style="color: ..."></span> around it. Also, to be able to save the highlights, I need the startnode/endnode, together with offsets. I believe that being able to convert a user selection into a DOM range is a very, very useful feature. Hope this gets implemented! It's already out of NS6.0.
Keywords: mozilla0.9
Assignee | ||
Comment 47•24 years ago
|
||
code checked in to allow getSelection to work i believe. please give it a shot.
Comment 48•24 years ago
|
||
i have been working on a little script to test out events w/ selections <a href="http://randomfoo.net/tutorials/events1.html ">http://randomfoo.net/tutorials/events1.html </a>. maybe i'm not understanding how the selection/range object works, but with build 2000113020, window.getSelection, and the SelectionObj.toString() don't seem to be returning anything.
Comment 49•24 years ago
|
||
This now works for me. lhl: make sure you have something in the content window highlighted. This isn't for textfields.
Comment 50•24 years ago
|
||
lhl: window.getSelection is for the selection in the non-form areas of the window. You can actually have two simultaneous selections inside a form's textfield and within the non-form content. That's just the way it works - for whatever that's worth. Use seltext=selnode.value.substr(target.selectionStart,target.selectionEnd); However this is not working for textarea (multiline textfields) - see bug #58850. We expect that to be fixed soon. Email me directly if you need any more info. -- aaronl@chorus.net
Comment 51•23 years ago
|
||
is this fixed?
Assignee | ||
Comment 52•23 years ago
|
||
i have a fix in hand. i need an SR to take a look I will post patch here for someone to test also.
Whiteboard: [rtm-] → FIXINHAND
Assignee | ||
Comment 53•23 years ago
|
||
Comment 54•23 years ago
|
||
*** Bug 58850 has been marked as a duplicate of this bug. ***
Comment 55•23 years ago
|
||
Mike, can you attach a diff -u? The last diff is confusing, since it just shows a bunch of changed lines. Also, I'm confused about why we need window.getSelection(). Shouldn't it just be window.selection ?
Assignee | ||
Comment 56•23 years ago
|
||
checked in multiline support. if error please reopen. i just changed the implementation if we want a different mapping of getselection or whatever that can be done seperately
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•