48575 - window.getSelection() broken

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Adam Lock]

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

[Simon Fraser [no longer active]]

Also need to break up nsIDOMSelection to hide non-public bits (Hint-related 
stuff) and general cleanup.

Comment 2

[Simon Fraser [no longer active]]

Bug 49511 should be fixed at the same time.

Comment 3

[rubydoo123]

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

[Simon Fraser [no longer active]]

Yes, we have to fix this before the emedding APIs are frozen, since part of 
nsIDOMSelection needs to be a public interface.

Comment 5

[Akkana Peck]

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 6

[Akkana Peck]

Shouldn't this be nsbeta3?

Comment 7

[rubydoo123]

Simon/Akkana -- if you can help Mike get this resolved, that would be great.

Comment 8

[mjudge]

all set to go just have 1 or 2 string questions for scott collins when i can get 
ahold of him.

Comment 9

[mjudge]

for better or worse. its in ;)

Comment 10

[sujay]

can someone verify this one please?

Comment 11

[sujay]

marking verified.

Comment 12

[Johnny Stenback (:jst)]

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.

Comment 13

[rubydoo123]

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.

Comment 14

[Akkana Peck]

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

[Johnny Stenback (:jst)]

Converting the existing API to IDLC sounds good to me.

Comment 16

[Mitchell Stoltz (not reading bugmail)]

If you want to use XPConnect (using nsISecurityCheckedComponent) instead of DOM 
idl, that's perfectly fine, as long as we do a security review.

Comment 17

[mjudge]

ok what does nsISecurityCheckedComponent do and what do I have to do to get it 
to work?

Comment 18

[Johnny Stenback (:jst)]

Jband, care to explain what it takes to make nsSelection.cpp implement
nsISecurityCheckedComponent?

Comment 19

[John Bandhauer]

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

[Simon Fraser [no longer active]]

It's probably easier to just move nsISelect back to IDLC.

Comment 21

[mjudge]

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?

Comment 22

[mjudge]

removing CCs. not sure why they were added automatically sorry for the extra 
noise.

Comment 23

[mjudge]

Attachment: diff to add .MAP files to each module creation in config.mak

Comment 24

[mjudge]

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

[Mitchell Stoltz (not reading bugmail)]

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?

Comment 26

[mjudge]

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

[Jesse Ruderman]

Adding "window.getSelection()" to summary.

Comment 28

[rubydoo123]

removing + per pdt sw rules

Comment 29

[Simon Fraser [no longer active]]

mstoltz: what is the procedure for getting the security review here?
mjudge: please attach your diff.

Comment 30

[Mitchell Stoltz (not reading bugmail)]

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?

Comment 31

[mjudge]

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

Comment 32

[mjudge]

Attachment: dif adding nsISecurityCheckedComponent to nsTypedSelection

Comment 33

[Mitchell Stoltz (not reading bugmail)]

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

[Mitchell Stoltz (not reading bugmail)]

Restoring rtm+ so this gets looked at by pdt today.

Comment 35

[Phil Peterson]

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]

Comment 36

[mjudge]

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

Comment 37

[mjudge]

changing summary. i am assuming that it is pdt that marks things rtm-. marking 
rtm+ i guess so that they can mark this -.

Comment 38

[Phil Peterson]

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-]

Comment 39

[Jesse Ruderman]

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

[dannyg]

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

[Jesse Ruderman]

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

[Simon Fraser [no longer active]]

Remove bogus () from summary

Comment 43

[rubydoo123]

moving to mozilla0.9

Comment 44

[Aaron Leventhal]

This bug needs fixing for accessibility work I'm doing. I need to be able to
speak the current text position.

Comment 45

[mjudge]

ok will check into trunk soonest.

Comment 46

[Konstantin Riabitsev]

(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.

Comment 47

[mjudge]

code checked in to allow getSelection to work i believe. please give it a shot.

Comment 48

[leonard]

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

[Aaron Leventhal]

This now works for me. lhl: make sure you have something in the content window
highlighted. This isn't for textfields.

Comment 50

[Aaron Leventhal]

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. -- [e-mail]

Comment 51

[Asa Dotzler [:asa]]

is this fixed?

Comment 52

[mjudge]

i have a fix in hand. i need an SR to take a look I will post patch here for 
someone to test also.

Comment 53

[mjudge]

Attachment: dif for multiline GetSelectionRange

Comment 54

[anthonyd]

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

Comment 55

[Simon Fraser [no longer active]]

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 ?

Comment 56

[mjudge]

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

Comment 57

[sujay]

Adan, can  you verify this one? thanks...