| Summary: | Add selectionchange event | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||
| Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Enhancement | CC: | ap, arv, ayg, darin, enrica, eric, jparent, justin.garcia, mjs, ojan, sam, timdown, tkent, webkit-ews, webkit | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Bug Depends on: | 48151, 49785 | ||||||||||||||
| Bug Blocks: | |||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Ryosuke Niwa
2010-09-13 15:21:36 PDT
No need to restrict this to contentEditable regions. selectionChange events anywhere on the page are useful. Also, IE documentation claims to have this event already, but I remember there being issues with it: http://msdn.microsoft.com/en-us/library/ms536968(VS.85).aspx Created attachment 71240 [details]
work in progress
onselectionchange event is weird in that the event handler is always attached to document (even if it came from onselectionchange attribute of body element) and doesn't bubble. The key to implementing this is making sure that we're ready for arbitrary DOM mutations after event dispatch. Synchronous events like this are a huge cause of WebKit crashes. (In reply to comment #4) > The key to implementing this is making sure that we're ready for arbitrary DOM mutations after event dispatch. Synchronous events like this are a huge cause of WebKit crashes. After making the event asynchronous, I just realized that SelectionController::setSelection already has a call to setFocusedNodeIfNeeded when the current selection is none. So dispatching one more event in this function should not introduce new vulnerability. This is likely, but not a 100% guarantee of safety. The focus event isn't always dispatched, so we may only be prepared for DOM modifications in cases when it is. Created attachment 71354 [details]
implements selectionchange event
(In reply to comment #7) > Created an attachment (id=71354) [details] > implements selectionchange event The selectionchange event is already implemented by Internet Explorer (http://msdn.microsoft.com/en-us/library/ms536968(VS.85).aspx) and fired whenever selection is changed. The event is not cancelable and does not bubble. An event listener can be attached to a document by body element's onselectionchange attribute or via document.selectionchange. In this implementation, selection change event is fired inside SelectionController::setSelection, which is a function used to update selection in various parts of WebKit. Our implementation slightly differs from Internet Explorer in that we fire focus event before selection change event whereas Internet Explorer fires selectionchange event before focus event. Attachment 71354 [details] did not build on qt: Build output: http://queues.webkit.org/results/4608002 Created attachment 71367 [details]
added onselectionchange to HTMLAttributesNames.idl
(In reply to comment #6) > This is likely, but not a 100% guarantee of safety. The focus event isn't always dispatched, so we may only be prepared for DOM modifications in cases when it is. Do you think we should not add the event until we can make such a guarantee? I briefly looked at the call sites of setSelection and many call sites do modify DOM so they should be resilient to the changes in the DOM. However, there are simply too many to be "verified" in a reasonable amount of time. If your answer to this question is yes, it might make more sense to implement this event asynchronously. We certainly shouldn't add the event if there is a reason to believe that it introduces security bugs! I don't know this code well enough to have an informed opinion on the likelihood of that. I was just pointing out that the existence of focus event is not formally a guarantee that we aren't adding new problems. Attachment 71354 [details] did not build on qt: Build output: http://queues.webkit.org/results/4604005 Attachment 71354 [details] did not build on mac: Build output: http://queues.webkit.org/results/4704004 Comment on attachment 71367 [details]
added onselectionchange to HTMLAttributesNames.idl
I agree with ap.
For example, HTMLInputElement::setValue() updates selection only if the element has focus, and access a data member. If we apply this patch, HTMLInputElement::setValue() will have new crash.
We need to check all of code paths to SelectionController::setSelection() if 'selectionchange' event is synchronous.
(In reply to comment #12) > We certainly shouldn't add the event if there is a reason to believe that it introduces security bugs! > > I don't know this code well enough to have an informed opinion on the likelihood of that. I was just pointing out that the existence of focus event is not formally a guarantee that we aren't adding new problems. (In reply to comment #15) > (From update of attachment 71367 [details]) > I agree with ap. > For example, HTMLInputElement::setValue() updates selection only if the element has focus, and access a data member. If we apply this patch, HTMLInputElement::setValue() will have new crash. > We need to check all of code paths to SelectionController::setSelection() if 'selectionchange' event is synchronous. I talked with ojan & other folks today, and I think we want to add a setSelectionFiringEvent (tentative name) which calls the current setSelection and also fires the event. The event dispatch can be hidden behind ifdef and we can gradually migrate other caller to use the new version. That way, we can verify each change carefully rather than having to locate & fix 100+ possible call sites of setSelection. > I talked with ojan & other folks today, and I think we want to add a setSelectionFiringEvent (tentative name) which calls the current setSelection and also fires the event. The event dispatch can be hidden behind ifdef and we can gradually migrate other caller to use the new version. That way, we can verify each change carefully rather than having to locate & fix 100+ possible call sites of setSelection.
It sounds reasonable.
(In reply to comment #16) > I talked with ojan & other folks today, and I think we want to add a setSelectionFiringEvent (tentative name) which calls the current setSelection and also fires the event. The event dispatch can be hidden behind ifdef and we can gradually migrate other caller to use the new version. That way, we can verify each change carefully rather than having to locate & fix 100+ possible call sites of setSelection. That seems like a strange plan. It means we’ll have an event that doesn’t fire correctly; depending on different end-user use patterns it will either work or fail. The project of changing all the setSelection call sites seems like a long one. Doesn’t seem OK to me. This doesn’t seem like the right way to make incremental progress on this task. (In reply to comment #18) > That seems like a strange plan. It means we’ll have an event that doesn’t fire correctly; depending on different end-user use patterns it will either work or fail. The project of changing all the setSelection call sites seems like a long one. > > Doesn’t seem OK to me. This doesn’t seem like the right way to make incremental progress on this task. But I don't think we can verify the safety of all 100+ call sites in one patch. It's true that we'll have half-broken event while we're implementing the event but the feature will be disabled by default so we won't expose it to users or to the Web. (In reply to comment #19) > (In reply to comment #18) > > That seems like a strange plan. It means we’ll have an event that doesn’t fire correctly; depending on different end-user use patterns it will either work or fail. The project of changing all the setSelection call sites seems like a long one. > > > > Doesn’t seem OK to me. This doesn’t seem like the right way to make incremental progress on this task. > > But I don't think we can verify the safety of all 100+ call sites in one patch. It's true that we'll have half-broken event while we're implementing the event but the feature will be disabled by default so we won't expose it to users or to the Web. An incremental approach is good, but the right incremental approach is one we can test. Changing one call site at a time won’t do any good unless we have actual test cases that change the DOM in response to the event. I think we should take an incremental approach with testing, then turn the event sending on once we can pass all the tests. Sending the event in certain places just invites us to ship broken software and helps create an illusion that we are testing incrementally. (In reply to comment #20) > An incremental approach is good, but the right incremental approach is one we can test. Changing one call site at a time won’t do any good unless we have actual test cases that change the DOM in response to the event. I think the point of breaking into smaller pieces is so that we can verify adding this event doesn't make a new security hole. It's not necessary that we want to incrementally implement the event. > Sending the event in certain places just invites us to ship broken software and helps create an illusion that we are testing incrementally. If you're talking about the completeness, then we can easily ensure it by simply deprecating (renaming or removing) the old function once we replaced all calls to setSelection. It's true that we won't be testing the safety of firing event while adding the event but I don't think we can test it. FYI, https://bug-45712-attachments.webkit.org/attachment.cgi?id=71367 implements selection change event in one patch without verifying the safety of call sites. (In reply to comment #21) > I think the point of breaking into smaller pieces is so that we can verify adding this event doesn't make a new security hole. It's not necessary that we want to incrementally implement the event. The verification comes from the testing, though, not the way we stage calls to each call site. (In reply to comment #22) > (In reply to comment #21) > > I think the point of breaking into smaller pieces is so that we can verify adding this event doesn't make a new security hole. It's not necessary that we want to incrementally implement the event. > > The verification comes from the testing, though, not the way we stage calls to each call site. But how do we add a feature incrementally if we were to expose it to DRT? Should we add a preference / setting that enables onselectionchange event? (In reply to comment #23) > (In reply to comment #22) > > (In reply to comment #21) > > > I think the point of breaking into smaller pieces is so that we can verify adding this event doesn't make a new security hole. It's not necessary that we want to incrementally implement the event. > > > > The verification comes from the testing, though, not the way we stage calls to each call site. > > But how do we add a feature incrementally if we were to expose it to DRT? Should we add a preference / setting that enables onselectionchange event? What's the problem with just using a #IF? Darin is just saying we shouldn't do something incremental. 1. Add the code change with enclosed in a #IF. 2. Add test cases for all the places where we set the selection to make sure we don't crash. 3. When you have reasonable confidence, remove the #IF. During step 2, there may be cases where tests are technically failing, but it's appropriate in those cases to check in a failing expectation. (In reply to comment #24) > 1. Add the code change with enclosed in a #IF. > 2. Add test cases for all the places where we set the selection to make sure we don't crash. > 3. When you have reasonable confidence, remove the #IF. > > During step 2, there may be cases where tests are technically failing, but it's appropriate in those cases to check in a failing expectation. How do make a test in step 2? WebKit doesn't crash without selectionchange event. How do we write a test for a crash that doesn't exist? Of course, I agree that we can and should write tests if we're modifying the existing code to avoid crashes. (In reply to comment #25) > (In reply to comment #24) > > 1. Add the code change with enclosed in a #IF. > > 2. Add test cases for all the places where we set the selection to make sure we don't crash. > > 3. When you have reasonable confidence, remove the #IF. > > > > During step 2, there may be cases where tests are technically failing, but it's appropriate in those cases to check in a failing expectation. > > How do make a test in step 2? WebKit doesn't crash without selectionchange event. How do we write a test for a crash that doesn't exist? Of course, I agree that we can and should write tests if we're modifying the existing code to avoid crashes. It shouldn't crash with or without the event, right? So you can write the test and check it in. When actually working on this, you'll need to modify your build locally to enable the event. The bots won't test the code until the feature is enable. This is how it is with every feature that's enabled behind a define. (In reply to comment #26) > It shouldn't crash with or without the event, right? So you can write the test and check it in. When actually working on this, you'll need to modify your build locally to enable the event. The bots won't test the code until the feature is enable. > Sure, but how do we write a test that might cause a crash under correct circumstances? I mean just attaching an event listener surely doesn't crash WebKit even in my native implementation. Maybe we should modify DOM in such a way to cause a crash? But how do we know how to cause a crash if we didn't make any change to the existing code (i.e. we believe that firing event is safe without modification)? Do we guess a sequence of DOM modifications that may or may not cause crash? (In reply to comment #27) > Sure, but how do we write a test that might cause a crash under correct circumstances? I mean just attaching an event listener surely doesn't crash WebKit even in my native implementation. Maybe we should modify DOM in such a way to cause a crash? But how do we know how to cause a crash if we didn't make any change to the existing code (i.e. we believe that firing event is safe without modification)? Do we guess a sequence of DOM modifications that may or may not cause crash? You can't capture every possible codepath, but you can look at each place where we use setSelection and see what expectations the code following the setSelection call expects of the state of the DOM. Then write tests for those cases. You're never going to get 100% confidence, but you'd like to have reasonable confidence that you're not introducing crashes. Maybe document.open() to remove all document nodes, and then force garbage collection? (In reply to comment #28) > You can't capture every possible codepath, but you can look at each place where we use setSelection and see what expectations the code following the setSelection call expects of the state of the DOM. Then write tests for those cases. You're never going to get 100% confidence, but you'd like to have reasonable confidence that you're not introducing crashes. (In reply to comment #29) > Maybe document.open() to remove all document nodes, and then force garbage collection? That sounds good to me. We still do want to implement it incrementally, correct? It is not clear to me why the option of implementing this asynchronously has been dropped from discussion. Give that we wish to move away from synchronous mutation events, I am not sure it makes sense to add new events that could have many of the same issues. Erik, Maciej, Sam, & I talked on IRC, and came to the consensus that we should impalement selection change event as an asynchronous event to make it easier to verify that we're not making new security vulnerabilities. Because selection change event is not cancelable and does not bubble, it doesn't need to be synchronous. We haven't agreed on the exact method to implement the event, however. One major problem with completely asynchronous implementation of selection change event using a timer is that when a script modifies the selection by some function, say selectAllChildren, it may expect the corresponding selection change event to be fired before we return from the function, namely selectAllChildren in this case. If the script had attached an event handler to the selection change event, it may also expect certain global state or DOM be updated accordingly by this event handler and the fact we implemented selection change event asynchronously will be visible and developers may need to work-around this issue. To reconcile this problem, Erik suggested that we queue up events and fire them all when we cross WebCore and JavaScriptCore/V8 boundary. I suggested that we can also use this solution to resolve the bug 46936 since this approach hides the fact it's implemented asynchronously. Maciej suggested an alternative approach using a scope object but I didn't quite get what he really meant. Maciej & Sam, could you elaborate on how scoping object works? I talked with Maciej about his scoping object idea. Here's summary: We define a class, let's say SelectionOperationScope, which is used as a RAII object. SelectionOperationScope has a static counter which tracks the level of nesting. When there's at least one level of nesting, we'll queue up events, and when the nesting level goes to 0, we'll fire the queued events. We can define this scope object in functions that should prohibit event firing. For example, all editing commands will define this object. (In reply to comment #33) > We can define this scope object in functions that should prohibit event firing. For example, all editing commands will define this object. I’d use the word “defer” here rather than “prohibit”. (In reply to comment #34) > (In reply to comment #33) > > We can define this scope object in functions that should prohibit event firing. For example, all editing commands will define this object. > > I’d use the word “defer” here rather than “prohibit”. Right, that's a better word here. Anyway, here's my thought about implementing this event asynchronously. I'm currently in a conversation with other Googlers who are familiar with challenges in RTE applications, and I've got the feeling that what we really want is selectstart event (http://msdn.microsoft.com/en-us/library/ms536969(VS.85).aspx). This event is cancellable and bubbles so it can't be implemented as an asynchronous event. If we were ever to support selectstart event, then there isn't really a point in implementing selectionchange event asynchronously because we'll run into the same problem when we implement selectstart event. Any thoughts? (In reply to comment #35) > If we were ever to support selectstart event, then there isn't really a point in implementing selectionchange event asynchronously because we'll run into the same problem when we implement selectstart event. selectionchange is useful even if we add a beforeselectionchange-style event. I think we should go ahead and implement it async. We can make it sync later if we really want to. Google's rich text library (http://closuretools.blogspot.com/2010/07/introducing-closure-library-editor.html) had a custom selectionchange event (using mouse and key events) that fires before the selection is changed. That library could be ported to use an async selectionchange event if it had a property that showed what the selection was before it changed. Created attachment 76675 [details]
async implementation
Could someone review my patch? Created attachment 81446 [details]
updated to apply on TOT
Can someone review my patch? Or give me a feedback? Committed r79208: <http://trac.webkit.org/changeset/79208> The upshot is that the selectionchange event fires asynchronously, which leaves web developers (such as me, right now) with precisely the problem Ryosuke described in comment #32. Is there a solution that doesn't involve timers? (In reply to comment #42) > The upshot is that the selectionchange event fires asynchronously, which leaves web developers (such as me, right now) with precisely the problem Ryosuke described in comment #32. Is there a solution that doesn't involve timers? Unfortunately the ship has sailed already. I don't think we can change our behavior at this point. (In reply to comment #43) > (In reply to comment #42) > > The upshot is that the selectionchange event fires asynchronously, which leaves web developers (such as me, right now) with precisely the problem Ryosuke described in comment #32. Is there a solution that doesn't involve timers? > > Unfortunately the ship has sailed already. I don't think we can change our behavior at this point. Yep. A dirty, dirty timer it is then. The only options I can think of to improve matters are: - a new event that is the same as selectionchange except that it fires only in response to a direct user action changing the selection, although I think it could be hard to pin down what should and shouldn't fire such an event. I don't really like it. - a new property of the selectionchange event object that indicates what kind of action initiated the event (e.g. mouse drag, menu option, keyboard, execCommand, method of Selection object) (In reply to comment #44) > - a new event that is the same as selectionchange except that it fires only in response to a direct user action changing the selection, although I think it could be hard to pin down what should and shouldn't fire such an event. I don't really like it. selectstart is like that although it fires before the selection changes. > - a new property of the selectionchange event object that indicates what kind of action initiated the event (e.g. mouse drag, menu option, keyboard, execCommand, method of Selection object) Possible. We should probably discuss this on a standard mailing list first. (In reply to comment #43) > (In reply to comment #42) > > The upshot is that the selectionchange event fires asynchronously, which leaves web developers (such as me, right now) with precisely the problem Ryosuke described in comment #32. Is there a solution that doesn't involve timers? > > Unfortunately the ship has sailed already. I don't think we can change our behavior at this point. Since this event is sync in IE, I expect we could get away with making it sync (but deferred, the way we defer mutation events) in WebKit without backwards compat issues. I don't have strong opinions on which way we implement it. In either case, the editing spec should probably cover this. There's an open spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=13952 As always, if you're interested in working on this and are blocked by a spec bug, please say so explicitly on the bug and I'll see if I can prioritize it. |