With reference to this Porkjockeys thread: news://news.mozilla.org/3992F952.EA96F89D%40iol.ie nsIDOMSelection needs to be xpidl'ized instead of idlc + simplified
Also need to break up nsIDOMSelection to hide non-public bits (Hint-related stuff) and general cleanup.
Bug 49511 should be fixed at the same time.
is 46847 really dependent on this bug? Looking at the last entries in that bug states that they are almost done with that one.
Yes, we have to fix this before the emedding APIs are frozen, since part of nsIDOMSelection needs to be a public interface.
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.
Shouldn't this be nsbeta3?
Simon/Akkana -- if you can help Mike get this resolved, that would be great.
all set to go just have 1 or 2 string questions for scott collins when i can get ahold of him.
for better or worse. its in ;)
can someone verify this one please?
marking verified.
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.
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.
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).
Converting the existing API to IDLC sounds good to me.
If you want to use XPConnect (using nsISecurityCheckedComponent) instead of DOM idl, that's perfectly fine, as long as we do a security review.
ok what does nsISecurityCheckedComponent do and what do I have to do to get it to work?
Jband, care to explain what it takes to make nsSelection.cpp implement nsISecurityCheckedComponent?
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.
It's probably easier to just move nsISelect back to IDLC.
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?
removing CCs. not sure why they were added automatically sorry for the extra noise.
Created attachment 16216 [details] [diff] [review] diff to add .MAP files to each module creation in config.mak
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 ;)
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?
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.
Adding "window.getSelection()" to summary.
removing + per pdt sw rules
mstoltz: what is the procedure for getting the security review here? mjudge: please attach your diff.
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?
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
Created attachment 16915 [details] [diff] [review] dif adding nsISecurityCheckedComponent to nsTypedSelection
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.
Restoring rtm+ so this gets looked at by pdt today.
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]
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
changing summary. i am assuming that it is pdt that marks things rtm-. marking rtm+ i guess so that they can mark this -.
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-]
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.
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?
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).
Remove bogus () from summary
moving to mozilla0.9
This bug needs fixing for accessibility work I'm doing. I need to be able to speak the current text position.
ok will check into trunk soonest.
(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.
code checked in to allow getSelection to work i believe. please give it a shot.
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.
This now works for me. lhl: make sure you have something in the content window highlighted. This isn't for textfields.
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
is this fixed?
i have a fix in hand. i need an SR to take a look I will post patch here for someone to test also.
Created attachment 28446 [details] [diff] [review] dif for multiline GetSelectionRange
*** Bug 58850 has been marked as a duplicate of this bug. ***
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 ?
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
Adan, can you verify this one? thanks...
or
