Internet Explorer supports additional clipboard events, oncut oncopy onpaste onbeforecut onbeforecopy onbeforepaste These events are very useful in that you can know when cut/copy/paste has happened in an entry field, and modify the data if necessary (for example prevent the pasting of text in a numeric entryfield. In addition, you can even not allow the paste if the data is not appropriate for the entry field.

There is a bug for onpaste, bug 280683.

what's the use case for oncopy and oncut?

(In reply to comment #2) > what's the use case for oncopy and oncut? That's a really good question. The middleware team asked for all three but only gave the use case for onpaste. I'll check on that.

Jesse has an interest in clipboard issues.

I can't think of any security issues with onpaste. But for Mozilla's implementation to be compatible with Internet Explorer's, Mozilla would have to add support for window.clipboardData.getData(). Web pages would only be allowed to call this function during legitimate onpaste events. (See example on http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onpaste.asp.) I'm a little worried about oncut/oncopy: a web page might convince you to copy something from the page and paste it back into the same page, but use oncopy to cancel the copy command, causing the paste command to reveal what was in your clipboard before. There might be other ways for web pages to prevent cut/copy commands from succeeding, but I'd prefer to avoid legitimizing this security hole by turning it into a feature. Onbeforepaste (etc) sound like they give scripts a new way to detect or slow down the display of context menus. Should onbeforepaste be disabled when the user unchecks "allow scripts to... disable or replace context menus"?

Here's the use case, although when I read it, it still seems to only be about onpaste to me: Here's an example of where we trap it and use it. Form designer sets up an <input type="text"> field. Tells us he wants it to be a number of the format (Java number format) $ #,###,###0.00;($ #,###,##0.00). Sets some limitations (e.g., value must be provided, must be between -10,000 and 1,500,000), and they ask for full keyboard assist while the field is active (i.e., has focus). We attach a huge blob of Javascript to the field which currently traps the following events: onFocus, onBlur, onKeyDown, onKeyUp, onKeyPress, onClick, and in IE, onPaste, onCut, and onCopy. Now lets say the field's initial value is 789. While the field doesn't have focus, we'll display: $ 789.00 When the field gets focus, we'll display: $ ****789.00 And initially position the cursor after the 9 (before the decimal point). The use types a 1, this will result in the following being displayed: $ ***7,891.00 And the cursor will stay just before the decimal point. User now selects "7,89". Then they do a Copy (either CTRL+C or menu command). They position the cursor in front of the first asterick and do a paste. In IE we trap the paste, get the content of the clipboad and strip it down to pure digits, then do the insert (and return false in the event handler so IE doesn't do the paste) which will result in: $ 7,897,891.00 In Mozilla, we can't trap the event other than an "onchange" occurred. We don't know what the user did, but we do know the current value of the field is ($ 789***7,891.00 and that this is invalid so we discard what happened and revert to the previous value. The net is, when we're doing keystroke-by-keystoke assist on input, in IE you can do cut/copy/paste. In Mozilla, we don't allow it.

Incidentally, the oncut/oncopy/paste events aren't necessarily just about touching clipboard data - you want to know when the event happens so you can manipulate the entryfield data.

Seems like type="number" would be a better way of doing the use case described above. I'm still not really convinced this is a good thing.

for their use case, can't they just strip out the *'s (and ,'s) and then put them back? i really am not compelled by their case.

Here's the cut case: We currently only modify the data in the onCopy/onCut in one case (though we'll probably extend this). Consider a "date" field with the value: February 8, 2004 When you copy it, we convert it to 02-02-2004 00:00:00 (and put this on the clipboard). In other words, date (date/time) are always copied to the clipboard in a "neutral" format rather than as a raw string. Then when you do a paste (at least into another "date" field), we can use the date in this canonical form and not have to worry about trying to parse out something like "February" (which might be a different word if the field used a different locale than where we're doing the paste). One problem with the MS implementation of the clipboard is that it doesn't allow you to set multiple versions of the data in the clipboard object (e.g., we really should be able to set the data as a "datetime" object and as a string so that on a paste we can choose the 'best" data format given the target of the paste.

i've very scared of oncopy/oncut (we already have problems stemming from anonymous/hidden content). protected by security requirements, i suppose this could be ok. i think that the web app should probably just deal with bad input, after all, what happens when i copy "February 8, 2004" from notepad? And I certainly don't expect to have "February 8, 2004" pasted into notepad as 02-02-2004 00:00:00.

The date use case would be better handled by a native type="date" control, as proposed in Web Forms 2: http://www.whatwg.org/specs/web-forms/current-work/#date

I understand there are better ways to do it, but none of those exist today. There is an API in IE that we could emulate today for apps that use it.

(In reply to comment #13) > I understand there are better ways to do it, but none of those exist today. Support in Gecko does not exist today either.

For reference http://msdn.microsoft.com/workshop/author/dhtml/reference/objects/clipboarddata.asp

Note that this is already possible using other methods. Catching the CTRL+C event and put the text in a (hidden) textarea and when someone does CTRL+V you put the (modified) text back. (Although that is only possible on one page, obviously.)

No, they could be using the cut/copy/paste menuitems as well.

Apple has a write-up on Safari's support for "Pasteboard operations" (onbeforecut, oncut, onbeforecopy, oncopy, onbeforepaste, onpaste). http://developer.apple.com/documentation/AppleApplications/Conceptual/SafariJSProgTopics/Tasks/CopyAndPaste.html Absent from these implementations is "onundo".

(In reply to my own comment #5) > I'm a little worried about oncut/oncopy: a web page might convince you to copy > something from the page and paste it back into the same page, but use oncopy to > cancel the copy command, causing the paste command to reveal what was in your > clipboard before. There might be other ways for web pages to prevent cut/copy > commands from succeeding, but I'd prefer to avoid legitimizing this security > hole by turning it into a feature. I filed bug 363132 on this issue.

The attached proposed patch adds support for oncopy, oncut, onpaste, onbeforecopy, onbeforecut, and onbeforepaste events. onbefore[cut,copy,paste] events are fired when the selection is changed, and are intended to inform the browser whether the operation is permitted. If the event handler returns false, that prevents the default calculation that takes place to determine whether the operation is permitted. For example, if you create a <div onbeforepaste="return false;">, and select text within the div, you'll find that the Edit > Paste menu option is enabled. The implementation of these event handlers is consistent with IE, but IE fires the events whenever the menu or context menu is opened, where this patch fires the events only when the selection is changed. on[copy,cut,paste] occurs before the operation. They can abort the operation if the event handler returns false. The implementation of these event handlers is consistent with IE and Safari. I will attach a test file that can be used to test the behaviour.

Attaching "playground" file with a few different tests of the event handlers.

I should add a note that this patch does not modify or permit additional clipboard access in any way. I didn't want to trod on that security concern until some consensus could be reached as to what additional access, if any, these events should provide.

At first glance, this patch doesn't guard against malicious script (e.g. script calling window.close() in the oncopy handler will null out mPresShell, causing the browser to crash). Also, the event name detection code in nsDOMClassInfo.cpp probably needs changign too. You probably want to ask jst@mozilla.com for review...

Boris, thank-you for the feedback. Along the same line, I've put together a number of potential problems with the patch. I'll submit an updated one with a fixes to your concerns and more tomorrow. Thanks.

Comment on attachment 271833 [details] [diff] [review] proposed patch >RCS file: /cvsroot/mozilla/layout/base/nsDocumentViewer.cpp,v > NS_IMETHODIMP DocumentViewerImpl::CopySelection() >+ if (!NS_FAILED(rv)) use NS_SUCCEEDED not, !NS_FAILED, use NS_FAILED, not !NS_SUCCEEDED >+nsresult DocumentViewerImpl::GetClipboardEventTarget(nsIDOMNode** eventTarget) >+{ >+ NS_ENSURE_ARG_POINTER(eventTarget); >+ if (!mPresShell) { >+ return NS_ERROR_NOT_INITIALIZED; >+ } you're using two different bracing styles (above/below), that's not generally correct. >+ if (NS_FAILED(rv)) >+ return rv; >+ if (!sel) >+ return NS_ERROR_FAILURE; >+ // should skip default behaviour here if event handler returns false, but please spell behavior the American way. (yes there are a handful of behaviours in our code, but there are more than 1000 for the American way and being relatively consistent is a win.)

Attaching updated patch. This patch addresses bzbarsky's concerns about crashing if window.close() is called by a malicious event handler, and updating event name detection code in nsDOMClassInfo.cpp. This patch also addresses the comments by timeless. Thanks. Additionally, this patch also adds support for the events on nsPlaintextEditor (<input>, <textarea>, and by inheritance Midas rich-text editors).

Comment on attachment 272018 [details] [diff] [review] proposed patch v2 Smaug, can you review this? Once you're done, I can sr...

I have to say this: I don't see a compelling reason to bring this code into our tree. Several times, use cases have been requested, and not much has come back. I believe this whole concept is a bit risky, at best a corner case.

This is certainly something I've heard requested *very* many times, and I would not be opposed to see this code in our tree. Whether it's 1.9 material or not is another question (probably safer not to take it given how close to beta we are and the possible security implications of a change like this).

I'm interested in the addition of these events to facilitate smart rich-text editing with the new contentEditable areas (bug 237964). Specifically, the addition of these events allows complete monitoring of when a contentEditable area has changed. Previously, using iframes (w/ designMode=on), it was possible to monitor for changes using DOM mutation events. Those events are fired at the document level though, which makes it quite difficult (and a big performance killer) to use on contentEditable areas. I would definitely like to see this patch in 1.9, as I believe that content editable (237964), also targeted for 1.9, brings an increased need for it. I don't believe that there are security concerns regarding clipboard contents, as the patch implementation makes no clipboard contents readable or modifiable. The current patch does allow a page to disable copy/cut/paste without any possibility of a user override, and I could see this being used in an annoying fashion (although it's a limited annoyance). If it would eliminate a barrier to getting this patch into 1.9, I'd be glad to remove that functionality, as personally I just need event notification.

Nominating for blocking1.9 based on that feedback. Thanks. Jesse, any more feedback on this issue wrt privacy/security?

Comment on attachment 272018 [details] [diff] [review] proposed patch v2 >+ >+ static nsresult GetClipboardEventTarget(nsISelection *aSel, nsIDOMNode **target); Add comment about what the method does here. Not |target| but aTarget. >+nsresult nsCopySupport::GetClipboardEventTarget(nsISelection *aSel, nsIDOMNode **eventTarget) >+{ >+ NS_ENSURE_ARG(aSel); >+ NS_ENSURE_ARG_POINTER(eventTarget); >+ Not eventTarget (or should it be target as in header) but aEventTarget. Initialize the out parameter to nsnull, *aEventTarget = nsnull; >+ nsCOMPtr<nsIDOMRange> range; >+ nsresult rv = aSel->GetRangeAt(0, getter_AddRefs(range)); >+ if (!NS_FAILED(rv)) >+ rv = range->GetStartContainer(eventTarget); ... NS_ENSURE_SUCCESS(rv, rv); rv = range->GetStartContainer(eventTarget); Is it possible that GetRangeAt returns null range but NS_OK? Then you should check also that range is not null. > NS_IMETHODIMP nsPlaintextEditor::Paste(PRInt32 aSelectionType) > { >+ if (!mPresShellWeak) >+ return NS_ERROR_NOT_INITIALIZED; Do you need this null check? Isn't do_QueryReferent null-safe. Same thing also elsewhere. >+ if (NS_SUCCEEDED(res)) >+ { I'd prefer having { in the same line as |if|, though the file doesn't seem to have consistent coding style conventions. >+ if ((evt.flags & NS_EVENT_FLAG_NO_DEFAULT) == NS_EVENT_FLAG_NO_DEFAULT) >+ return NS_OK; >+ } Check (status == nsEventStatus_eConsumeNoDefault) and then return. Status will have that value if PreventDefault was called. Same thing also elsewhere. (I know, we should get rid of nsEventStatus eventually.) > NS_IMETHODIMP nsPlaintextEditor::CanPaste(PRInt32 aSelectionType, PRBool *aCanPaste) ... >+ // Fire the onbeforepaste event. If the event handler requests to prevent >+ // default behavior, set *aCanPaste = true. (IE-style behavior) This sounds odd. I would have thought that if preventDefault is called, then aCanPaste would be false. But I don't have IE here to test. >+ nsCOMPtr<nsIDOMNode> eventTarget; >+ nsresult res = GetClipboardEventTarget(getter_AddRefs(eventTarget)); >+ // On failure to get event target, just forget about it and don't fire. >+ if (NS_SUCCEEDED(res)) >+ { >+ nsEventStatus status = nsEventStatus_eIgnore; >+ nsEvent evt(PR_TRUE, NS_BEFOREPASTE); >+ nsEventDispatcher::Dispatch(eventTarget, ps->GetPresContext(), &evt, nsnull, &status); >+ // if event handler return'd false (PreventDefault) >+ if ((evt.flags & NS_EVENT_FLAG_NO_DEFAULT) == NS_EVENT_FLAG_NO_DEFAULT) >+ { >+ *aCanPaste = PR_TRUE; >+ return NS_OK; >+ } >+ } > > // can't paste if readonly > if (!IsModifiable()) > return NS_OK; Should the event dispatching happen after this check? What does IE do with readonly controls? And Safari? > NS_IMETHODIMP nsPlaintextEditor::CanCut(PRBool *aCanCut) ... > { >+ *aCanCut = !isCollapsed && IsModifiable(); Should IsModifiable() check happen before event dispatch? Make sure this is consistent with what IE does with readonly controls. > NS_IMETHODIMP nsPlaintextEditor::CanCopy(PRBool *aCanCopy) > { > if (!aCanCopy) > return NS_ERROR_NULL_POINTER; >+ if (!mPresShellWeak) >+ return NS_ERROR_NOT_INITIALIZED; >+ nsCOMPtr<nsIPresShell> ps = do_QueryReferent(mPresShellWeak); >+ if (!ps) >+ return NS_ERROR_NOT_INITIALIZED; >+ > *aCanCopy = PR_FALSE; In all CanXXX methods keep *aCanCopy = PR_FALSE; statement right after if (!aCanCopy) return NS_ERROR_NULL_POINTER; > NS_IMETHODIMP DocumentViewerImpl::CutSelection() ... >+ if (NS_SUCCEEDED(rv)) >+ { if (...) { Also elsewhere in this file. >+ nsEventStatus status = nsEventStatus_eIgnore; >+ nsEvent evt(PR_TRUE, NS_CUT); >+ nsEventDispatcher::Dispatch(eventTarget, mPresContext, &evt, nsnull, &status); >+ // should skip default behavior here if event handler returns false, but >+ // there is no default behavior to worry about. In that case status parameter isn't needed at all, 3 first parameters to Dispatch should be enough. Also elsewhere.

New patch v3 addresses all style comments from Smaug, out parameter initialization, remove redundant null checks for mPresShellWeak, and so on. (In reply to comment #33) > This sounds odd. I would have thought that if preventDefault is called, then > aCanPaste would be false. But I don't have IE here to test. IE & Safari's behavior is definitely that aCanPaste would be true. I believe the reasoning is that this is the only way you could enable the "Paste" menu option for areas where pasting is normally not permitted (for example, a plain 'ole <div>). The default behavior being prevented by a "return false" is the correct calculation of aCanPaste. > Should the event dispatching happen after this check? What does IE do with > readonly controls? And Safari? The event dispatching can replace the IsModifiable check, to permit pasting in readonly controls (or even in places where it doesn't normally make sense). I've tested against readonly textareas in IE and Safari, and both will permit pasting if onbeforepaste="return false". (However, Safari will prevent Ctrl-V from working or firing onpaste, but since it is internally inconsistent [the menu option works, but the key combo doesn't], I'm pretty sure it's a Safari bug.) > Should IsModifiable() check happen before event dispatch? Make sure this is > consistent with what IE does > with readonly controls. IE and Safari appear to fire the event before all other checks, giving it absolute priority.

> In all CanXXX methods keep *aCanCopy = PR_FALSE; statement right after > if (!aCanCopy) > return NS_ERROR_NULL_POINTER; I'd prefer we didn't null-check XPCOM out params at all (since in JS you can't screw it up, and in C++ people really shouldn't), but if we do it, please use NS_ENSURE_ARG_POINTER(aCanCopy)....

Updated patch v3 uses NS_ENSURE_ARG_POINTER for null-checking out params.

FYI, http://beaufour.dk/jst-review/?patch=https%3A%2F%2Fbugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D272188&file= You may have to ignore the "res" gripes; the local file uses res already.

Alex, thanks for the pointer to jst-review. I've corrected the details it has pointed it in a new attached patch. The only problems not corrected were long line lengths in a mapping of event handler atoms to event handler codes in nsContentUtils.cpp, which matches the style of surrounding code. v4 of this patch fixes a crash that was possible when an onpaste event handler on an input element caused the element to be removed from the document. The check for 'if (mDidPreDestroy)' in nsPlaintextEditor::Paste addresses this crash. v4 also fixes a problem where onpaste and onbeforepaste weren't fired for content editable areas due to nsHTMLEditor overriding the ::Paste and ::CanPaste methods.

Attaching updated testing file.

Attaching mochitest unit tests for oncopy / oncut / onpaste event handlers.

Comment on attachment 272227 [details] [diff] [review] proposed patch v4 >+ { &nsGkAtoms::oncopy, { NS_COPY, EventNameType_HTMLXUL }}, >+ { &nsGkAtoms::oncut, { NS_CUT, EventNameType_HTMLXUL }}, >+ { &nsGkAtoms::onpaste, { NS_PASTE, EventNameType_HTMLXUL }}, >+ { &nsGkAtoms::onbeforecopy, { NS_BEFORECOPY, EventNameType_HTMLXUL }}, >+ { &nsGkAtoms::onbeforecut, { NS_BEFORECUT, EventNameType_HTMLXUL }}, >+ { &nsGkAtoms::onbeforepaste, { NS_BEFOREPASTE, EventNameType_HTMLXUL }}, Does Safari (or Opera?) support these attributes also in SVG? If it does, we should also. >+ >+ // Given the current selection, find the target that >+ // onbefore[copy,cut,paste] and on[copy,cut,paste] events will fire on. Nit, the event names are (before){0,1}(copy|cut|paste). Attribute and JS property names start with 'on'. Same also elsewhere. Maybe a folloup bug to reduce code duplication in nsXXXEditor::CanPaste and nsXXXEditor::Paste etc methods. It would be great if you could add the tests for possible crashes also to mochitest. All cases when during event dispatch window is closed or element removed from dom. Does IE also dispatch beforecut,beforecopy,beforepaste and again beforecut when clicking for example 'Content here is editable!'?

(In reply to comment #41) > Does Safari (or Opera?) support these attributes also in SVG? If it does, we > should also. A quick test shows that neither Safari nor Opera support these events in SVG. > [snip -- nit, will fix; add crash tests, will do; file bug to reduce > code duplication, will do] > > Does IE also dispatch beforecut,beforecopy,beforepaste and again beforecut > when clicking for example 'Content here is editable!'? IE dispatches the before(cut|copy|paste) events only when the "Edit" menu or the right-click context menu is opened. That seems to be when it determines whether the menu options are enabled or disabled. This patch fires the before(cut|copy|paste) events whenever the selection is changed, as that appears to be when the menu option status is updated. This does result in the events likely firing more often than IE, depending on the what the user is doing. beforecut appears to be fired twice because the delete command calculates whether the delete menu is enabled by calling CanCut. It's probably not the right behavior to potentially enable/disable the Delete menu option based upon the return value of the beforecut event, so I will change this to use it's own code path (unless anyone has another suggestion?)

Attached proposed patch v5. Changed comments to reference events by the proper name (thanks Smaug). Fixed bug in nsCopySupport::GetClipboardEventTarget where NS_ENSURE_SUCCESS was logging errors whenever the selection was collapsed (I misunderstood the purpose of this macro).

Attached updated unit tests (v2). Includes tests for crashes caused by closing the window on event handlers, or removing the HTML element being affected by the event. I wasn't sure where to put these tests, by the way, so I put them into dom/tests/mochitest/bugs/test_bug280959.html. This patch also includes a modification to widget/src/windows/nsWindow.cpp. SendKeyEvent was crashing when the window was closed in the new unit tests. A patch to nsWindow::DispatchEvent checks whether the window has been destroyed during the event handler. I wasn't sure if this should be filed as a separate bug with a separate patch... but since without this patch, the unit tests crash, I thought it was pretty important it be included here.

Comment on attachment 272429 [details] [diff] [review] mochitest tests (v2) Is this really the right version of the test? Where are the open/remove element tests? >+ >+// Randomize the order of the tests. They should operate in any order. >+// The possibility of randomly occuring test failures is quite dreadful, >+// so only use this code when working on this file to ensure that object >+// states are restored properly between tests. >+// testFunctions.sort(function() { Math.round(Math.round()) - 0.5; }); I think you were going to write Math.round(Math.random()). But randomizing tests this way makes debugging difficult if tests start to fail in some cases. >Index: widget/src/windows/nsWindow.cpp My guess is that this might not be the right fix. I'd try to change nsIWidget* widget to nsCOMPtr<nsIWidget> widget in nsDOMWindowUtils::SendKeyEvent. See also Bug 381993.

Attaching updated mochitest unit tests. This version has the new crash tests. I was working on the unit tests yesterday in my OBJDIR so I could just keep reloading the tests as I worked on them (Windows, no symlinks). Then I forgot to copy them up into the right location before creating the patch. And then I ran a build and overwrote the new tests. The good thing about writing the same code twice in 12 hours is that it comes out cleaner the second time. Also, I tried changing the fix for the SendKeyEvent crash, as you suggested. It worked perfectly, so I have included that fix in this patch, rather than my previous attempt.

Oops, forgot to comment out the random sorting of the tests. :-) This was intended to be commented out in the submitted patch to make debugging a failed test easier (but enabled when writing the tests, to make sure no state changes leak between tests).

Comment on attachment 272428 [details] [diff] [review] proposed patch v5 >+ >+ if (!(*aEventTarget)) >+ return NS_ERROR_FAILURE; >+ >+ return NS_OK; Nit, return (*aEventTarget) ? NS_OK : NS_ERROR_FAILURE; >+} >\ No newline at end of file Nit ^^^ >@@ -1292,16 +1293,28 @@ const char* nsDOMEvent::GetEventName(PRU To keep nsDOMEvent::GetEventName in sync with nsDOMEvent::SetEventType, please update also the latter one. The few "extra" beforeXXX events are probably ok, and anyway needed by ControllerCommands. Did you test that events are dispatched to right targets when bidi-text is used? With comments addressed, r=me, but if bidi needs for some reason more changes, I could re-review.

Comment on attachment 272491 [details] [diff] [review] mochitest tests (v4) >- nsIWidget* widget = GetWidget(); >+ nsCOMPtr<nsIWidget> widget = GetWidget(); You could put this to your "main patch". >+// Randomize the order of the tests. They should operate in any order. >+// The possibility of randomly occuring test failures is quite dreadful, >+// so only use this code when working on this file to ensure that object >+// states are restored properly between tests. >+// testFunctions.sort(function() { Math.round(Math.random() - 0.5); }); Please do not leave this here. Could you use 2 space indentation and |for (expr; expr; expr) {| and |if (expr) {| and |function name(params) {| coding style. >+// Run delayed tests. >+var num = delayedTests.length; >+var delay = 1000; // 1 sec >+for (let i = 0; i < num; i++) >+ setTimeout(delayedTests[i], i * delay); >+setTimeout(SimpleTest.finish, num * delay) Couldn't you change this so that when the previous test has run, the next one starts immediately. Should take less time that way to run the tests.

Revised patch based upon Smaug's comments: added code for SetEventType, changed an if-return-return into a return (x ? y : z), added newline at the end of nsCopySupport.cpp, and included the testing bug fix in nsDOMWindowUtils.cpp. Tested patch w/ dir="rtl". Appears to be no issues, events fire as expected and clipboard contains expected text. Requesting superreview.

Attaching new mochitest file. Fixes JS formatting as requested, removes offending random test ordering code, and runs tests without delay (and, not too surprisingly, much quicker). I certainly remember trying to have the tests run one-after-another rather than using a 1 second delay, and running into problems getting that to work. However, a fresh look at the problem came up with a solution very quickly.

Comment on attachment 272790 [details] [diff] [review] mochitest tests (v5) >+ var handlers = ['oncopy', 'oncut', 'onpaste']; ... >+ var handlers = ['oncopy', 'oncut', 'onpaste']; Test also onbeforeXXX r=me

Added crash tests for before events as requested.

Jonas, bz, Jesse, Damon and I got together and talked about whether we want to take this for 1.9 or not and the outcome of that was that we do want this for 1.9, so marking this as a blocker. That means it needs to go in before the code freeze next Wednesday (7/25). I'll be sr'ing this, and Jonas will help audit the new places in the code where we will now end up dispatching events where we didn't already do so, just to make sure we don't introduce event dispatching at unexpected and potentially unsafe times. Thank you Mathieu for the patch and tests!

Comment on attachment 272789 [details] [diff] [review] proposed patch v6 - In nsPlaintextEditor::Cut(): + // Fire the copy event. s/copy/cut/. sr=jst, but I'm not marking this as such yet as Jonas will have a look at this one as well before we'll land it. Thanks for the patch!

So the functions called from isCommandEnabled scares me a bit. These are GetCopyable, GetCutable, GetPasteable. The other callsites look ok, though it might be good to see if we can whack at the events a bit.

(In reply to comment #56) > So the functions called from isCommandEnabled scares me a bit. These are > GetCopyable, GetCutable, GetPasteable. The other callsites look ok, though it > might be good to see if we can whack at the events a bit. The before[cut|copy|paste] events respond to indicate whether the commands are enabled. Given that purpose, it's a logical map to fire them from isCommandEnabled. Do you think this will cause problems? Or is it just that it's unusual? I can't think of any other event that fires to enable/disable commands, so it probably is unusual. I certainly don't like the before* event names, since I think they don't describe the events well. But they're implemented to match IE & Safari.

Comment on attachment 272789 [details] [diff] [review] proposed patch v6 So I *think* that the current callsites are safe enough as far as crashing goes. I would feel better if we fired the onbefore* stuff somewhere else though. I'd say lets land with these events, but lets try to hammer on them some extra and do stuff like set document.location or remove document.documentElement from within an onbefore* event.

What i'm worried about is page javascript intentionally doing something evil when those events are fired, such as navigate to a new page, or make sure that the document is being destroyed. It is possible that the calling code will not be able to deal with this and crash in exploitable ways. I agree it is the logical place to fire the events. I'm just worried that executing untrusted code at that point is unsafe.

> do stuff like set document.location or remove document.documentElement Or remove window.frameElement from the parent document's DOM. That should tear down the docshell and editor nicely when using contenteditable, and might be interesting to test. Also worth settng window.frameElement to display:none and flushing out layout on the parent. That'll destroy the frame tree bit not the docshell, which might also be an interesting condition to be in. Most importantly, we want automated tests doing that in case someone changes some codepaths around.

Should this be marked as FIXED?

Apparently mochitest wasn't checked in

The patch was apparently checked in at 2007-07-25 21:14.

What a mess. Yes, this landed (the patch, not the tests yet), and it landed with a checkin comment (by me) that didn't include the bug number, and the comment I wrote in this bug about that never made it into the bug. But yes, this is fixed, but I don't think we should mark it as such before the tests are checked in (I'm planning on doing that as soon as I'm permitted to).

jst, want to file a bug to get that comment cvs adminned to include the bug#?

Bug 390375 filed to add the bug reference to the checkin comment.

Is it permissible for the tests to be checked in now, under the new trunk rules?

Yes, go ahead.

checkin-needed for mochitest patch.

Tests checked in. Marking bug FIXED. Thanks Mathieu Fenniak for all your work here!

The tests didn't stick, turned the tree orange (due to test failure). Please provide updated tests and I can land them...

The tests that are failing are the copy/cut/paste event handlers being placed on DOM elements (as opposed to editors). The tests run successfully & reliably when the single test is run (just test_280959.html), but fail when run from the batch test page. I narrowed the problem down to synthesizeKey in test_dom_* -- it appears that sometimes the window didn't have focus, and so the key events to copy/cut/paste weren't being received by the window. I've addressed this by starting the script off with a "window.focus()" and running the tests in "window.onfocus". Now the tests run successfully, consistently, when run from /tests/dom/tests/mochitest/bugs/... but not from one level up (/tests/dom/tests/mochitest/). I'm not sure what the problem is. Is it possible that other tests can be interfering with these tests? I will continue to investigate tomorrow. (attaching current improved tests that still don't work in a large batch test run, in case anyone has input)

This is probably unrelated to the test failure, but I see this error in the error console while carrying out the test: Error: synthesizeKey is not defined With one test, you have: <body onload="var sel = window.getSelection(); sel.removeAllRanges(); sel.selectAllChildren(document.body); synthesizeKey('v', {accelKey: 1});" onunload="window.opener.runNextTest();" onbeforepaste="window.close();"> It seems to me that as soon selectAllChildren is called, the code in onbeforepaste is called, so that's why the synthesizeKey error, I guess. + ok(true, handler + " did not cause crash by closing window."); I´m not sure what this is testing, it seems to me that handler can never be false here.

After long searching, I think I found out that running the test_Prototype.html test beforehand is causing this bug's test to fail. So it seems like some test in the test_Prototype.html tests is causing it to fail.

I have come to the same answer, after testing each DOM ajax test suite. Removing test_Prototype.html causes these tests to always run successfully. I am glad to hear that I am not crazy for thinking this, since it was a satisfying conclusion that my code isn't completely faulty. I would guess that test_Prototype.html is somehow polluting the test environment, but I haven't the faintest idea what could cause only a couple of these tests to fail. I'm planning on hacking off parts of test_Prototype.html until the cause of failure is left standing, and we'll see what that is.

The test error only occurs when: 1. The Minefield window has focus for the test. If "Run Tests" is hit, and another program activated (eg. the debugger), the tests succeed. 2. And testFormElementMethodsChaining in dom\tests\mochitest\ajax\prototype\test\unit\form.html is run before test_bug280959.html. More specifically, when testFormElementMethodsChaining calls the Prototype .activate() method on any form element on the page, which then calls the .focus() method on the element. When these components of the test failure are in place, the following occurs: 3. Error messages on the console: WARNING: NS_ENSURE_TRUE(docShell) failed: file p:/moz-hg/mozilla/dom/src/base/nsGlobalWindowCommands.cpp, line 493 WARNING: NS_ENSURE_TRUE(contentEdit) failed: file p:/moz-hg/mozilla/dom/src/base/nsGlobalWindowCommands.cpp, line 446 4. The command context that the clipboard command receives is null, and this appears to be the result of the focus controller's mCurrentWindow being mis-set. mCurrentWindow has mDocument == nsnull. I am still not sure how these elements all tie together. Given that point 2 is required for the failure to occur, it suggests to me that the focus controller has bad state leftover in it somehow. Does anyone reading this think, "oh, that's got to be...."?

Thanks for finding this out, Mathieu! I was trying to making a testcase out of this, but I failed. However, I tripped on bug 385478 again, while doing that. I'm wondering if it's related to this issue. Maybe the patch from bug 261074 (which was backed out) would fix this test failure? Needless to say a minimized testcase would help a lot here of what is going on.

(In reply to comment #77) > Maybe the patch from bug 261074 (which was backed out) would fix this test > failure? No, the patch from bug 261074 doesn't fix it.

(In reply to comment #71) > The tests didn't stick, turned the tree orange (due to test failure). Please > provide updated tests and I can land them... Which tinderboxes turned orange? All of them? Or only Windows?

Not blocking on getting the tests in. But please see if you can get them working because not having tests at all makes the whole feature much more likely to regress.

A simple test of the onbeforeXxx events (http://people.mozilla.com/~dholbert/tests/394818/onbeforecopy.html) show that they do not mimic IE behavior at all.

Uh.... we're firing these events when the document gets focus? That seems _very_ weird. Very very weird.

Weird enough that it seems like it would break pages actually using these events. Can we get a bug filed on sorting that out? That bug should block, imo.

It seems to me that is what bug 390238 was about.

Except the testcase cited in comment 81 doesn't have a text control. In fact, it has absolutely no way to cut anything. And more importantly, the command state is not actually changing (nothing is selected; we just focused the window), and yet we're still firing the event...

(In reply to comment #82) > Uh.... we're firing these events when the document gets focus? That seems > _very_ weird. Very very weird. > If Bug 144000 is backed out, those events don't fire. http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsPresShell.cpp&rev=3.1078&mark=4276#4267

Uh.. but a collapsed selection is not copyable. Clearly the event-firing code is overeager. Would it fire on every caret move during editing or caret browsing?

(In reply to comment #87) > Uh.. but a collapsed selection is not copyable. But perhaps still pasteable > Would it fire on every caret move during editing or caret browsing? Doesn't seem so

Why would it break pages to ask "should the Copy command be enabled now?" more often than IE? Firing on every caret move sounds perfectly reasonable for onbeforepaste: a page might want you to be able to paste in some places but not in others.

(In reply to comment #89) > Firing on every caret move sounds perfectly reasonable for onbeforepaste: a > page might want you to be able to paste in some places but not in others. That's not what onbeforepaste is for. onbeforepaste Event Fires on the target object before the selection is pasted from the system clipboard to the document. http://msdn2.microsoft.com/en-us/library/ms536905.aspx This is supposed to be called when the user actually selected paste. It happens before paste and based on the return from the call, the paste happens or doesn't happen. It's supposed to be designed so that on paste you can modify the data about to be pasted.

The MSDN documentation is misleading. It is fired "before the selection is pasted", but not immediately before, and not always. The before[...] events in IE are fired when a user right-clicks to view a context menu, or opens the "Edit" menu. These firings are to check whether the relevant menu items should enabled / visible. If I remember correctly from my testing, before[...] isn't fired *at all* during the actual operation, if you use keyboard shortcuts. And even if disabled by beforepaste, you can still use Ctrl-V to paste. Mozilla's implementation fires before[...] events at different times (very different times), but to the same effect as IE. The events being fired on document focus is new, as Smaug points out.

Hmm. When I loaded the testcase from comment 81, I got about 20-30 alerts that I had to dismiss. Two came up every time I moused over the window. That's what comment 82 was based on. Sadly, I haven't been able to reproduce that since... I just get the two alerts now.

(In reply to comment #91) > The MSDN documentation is misleading. It is fired "before the selection is > pasted", but not immediately before, and not always. > > The before[...] events in IE are fired when a user right-clicks to view a > context menu, or opens the "Edit" menu. These firings are to check whether the > relevant menu items should enabled / visible. True. Testing on IE7 confirms this. > If I remember correctly from my testing, before[...] isn't fired *at all* > during the actual operation, if you use keyboard shortcuts. And even if > disabled by beforepaste, you can still use Ctrl-V to paste. False. The event fire when using keyboard shortcuts too. > Mozilla's implementation fires before[...] events at different times (very > different times), but to the same effect as IE. The events being fired on > document focus is new, as Smaug points out. Mozilla events fire way too soon for my taste. Firing on selection of text is too soon and too often. Not firing on keyboard shortcut or menu use is not right either. We will also hurt performance on websites that use these events.

The events have to fire on selection change when the cut/copy/paste toolbar buttons are visible, at least.

(In reply to comment #94) > The events have to fire on selection change when the cut/copy/paste toolbar > buttons are visible, at least. > You would think - [onbeforeXxxx events can cancel behavior so we should disable buttons] - but after putting the cut/copy/paste buttons on the IE7 toolbar, I get no onbeforeXxx alerts when selecting text.

We don't have to match IE bug for bug :)

So the question is, is onbefore* in its _current_ form similar enough to the IE behavior that it sites will JustWork? If not, we have two options: 1. Fix the implementation so that sites will JustWork 2. Remove support for the onbefore* events in gecko 1.9 At this point, I'd say we should only do 1 if it's relatively easy. The onbefore* events are pretty scary as is actually.

(In reply to comment #96) > We don't have to match IE bug for bug :) Didn't microsoft invent these messages? So there is no spec for them other than microsoft? So if we want them to work as people expect them to work, we should match IE bug for bug. Because that's how the messages work...

(In reply to comment #98) > (In reply to comment #96) > > We don't have to match IE bug for bug :) > > Didn't microsoft invent these messages? So there is no spec for them other than > microsoft? So if we want them to work as people expect them to work, we should > match IE bug for bug. Because that's how the messages work... I believe that Jesse was referring to the fact that the cut/copy/paste status isn't updated properly when the buttons are on the toolbar. That definitely sounds like an IE bug, since you won't be able to disable the buttons based upon the selection context. (In reply to comment #97) > So the question is, is onbefore* in its _current_ form similar enough to the IE > behavior that it sites will JustWork? Yes, I believe so. The before events are intended to determine whether the action is available to the user. They are badly named. For evidence to support this statement, I point out that the events are fired whenever a user opens the "Edit" menu or right-clicks, regardless of whether they intend to perform an action related to these events. Developers currently using these events in IE should have noticed that, in IE: * They have to be fast, because they occur when opening a dropdown menu. You don't want to delay the "Edit" menu for the user. * They will be called even when the following event doesn't occur. (ie. beforepaste will be fired even if a paste never occurs). The same rules apply to the Mozilla implementation. The event handler has to be fast, and it will be called even when the following event doesn't occur. It just happens to be called at a different arbitrary time. I would be surprised to find a page that works nicely in IE using these events, but does not work in Mozilla. I think it would require using these events for some purpose beyond my imagination.

(In reply to comment #97) > So the question is, is onbefore* in its _current_ form similar enough to the IE > behavior that it sites will JustWork? I'd lean towards "no". (though I don't really know how current sites use onbefore*, so I'm not 100% sure how the FF3 onbefore* quirks would affect them.) See https://bugzilla.mozilla.org/show_bug.cgi?id=410858#c1 for a list of various onbefore* experiments & their results in FF3 and IE7. (the experiments all refer to the first testcase posted on that bug, attachment 295447 [details]). Those experiments show that FF3 treats onbefore* pretty differently from IE7... In particular, for experiments A-G, IE7 does absolutely nothing whereas FF3 fires one or more sets of events. And in the final two experiments (H and I), both IE7 and FF3 do something, but they still do somewhat different things.

(In reply to comment #100) > See https://bugzilla.mozilla.org/show_bug.cgi?id=410858#c1 for a list of > various onbefore* experiments & their results in FF3 and IE7. (the experiments > all refer to the first testcase posted on that bug, attachment 295447 [details]). > > Those experiments show that FF3 treats onbefore* pretty differently from IE7... See also https://bugzilla.mozilla.org/show_bug.cgi?id=410858#c4 for two more experiments... e.g. experiment I -- I don't see why FF3 would needs to fire 6 onbeforeCut, 3 onbeforePaste, and 3 onbeforeCopy events when the user just single-clicks inside a textbox, especially given that IE7 fires no onbeforeXXX events at all in that situation (In reply to comment #98) > (In reply to comment #96) > > We don't have to match IE bug for bug :) > > Didn't microsoft invent these messages? So there is no spec for them other than > microsoft? So if we want them to work as people expect them to work, we should > match IE bug for bug. Because that's how the messages work... AFAICT, this isn't just a case of trying to match them on edge cases where they're buggy. In playing with the testcases I posted on bug 410858, I've had a hard time finding *any* situations in which we fire the exact same set of onbeforeXXX events as IE7. (though maybe the differences are irrelevant in real-world situations?)

So what is the usecase for onbefore*? I.e. what are people using it for currently? It's been mentioned the ability to disable cut/copy/paste from context menus. It's also been mentioned the ability to change what is actually cut/copy/pasted. Is there anything else? I'm a little surprised by the second example, I thought that that was what oncut/oncopy/onpaste was for, not the 'before' events. Can people provide example snippets of code that they are currently using? This way we can determine if that will work in mozillas implementation too.

Btw, we need to make a decision pretty soon on this. We probably want to have this all taken care of by beta 3 which means that if someone needs to come up with a patch to fix onbefore*, that needs to happen really soon. Without a patch we're likely to back out onbefore*, but still keep oncopy/cut/paste

I don't see what's wrong with leaving it as-is. It's more correct (because it allows for giving toolbar buttons the correct state) but puts the same requirements on web pages (onbefore* must be fast and should not change anything). It also avoids giving web pages a way to screw with all context menus, which IE's implementation allows.

(In reply to comment #104) > I don't see what's wrong with leaving it as-is I'm just a little skeptical about the correctness of the current implementation because we fire so many onBeforeXXX events in quick succession, many of which seem to be duplicates. As demonstrated in the experiments on bug 410858, very simple operations on very simple testcases can trigger 3 successive sets of {onBefCut, onBefCopy, onBefPaste}, when 0 or 1 set of those events would be sufficient. Having said that... 1) Maybe there's a good reason for each of those seemingly duplicate events to be fired; I'm just not sure what it is. 2) Maybe the duplicates don't matter in real-world uses of onBeforeXXX

I'd really like for someone that is actually planning on using this to speak up. Kaply; do you have any js code you could share?

(In reply to comment #105) I must agree with Daniel Holbert. To Mathieu Fenniak: Your implementation uses "selection change" to trigger onbeforecopy and onbeforecut, which results in triggering these events at inappropriate times: that is, when selection changes (see attachment 295447 [details]). Instead, these events should be related to the moment when the user manifests the intention of performing a clipboard operation, either by using the keyboard or the contextual menu or the application menu: CTRL+X and CTRL+C. This is why they are called "onBeforePaste" and not "onAfterSelectionChange". They are not badly named as you claimed in comment #99. In the IE implementation (Safari too): the onbefore events are NOT intended to determine whether the action is available to the user or not (as you said it), but instead, all these onBeforeXXX and onXXX events should give developers a chance to CANCEL the default action associated with the corresponding clipboard operation and implement a custom clipboard behavior. Check http://msdn2.microsoft.com/en-us/library/ms537658(VS.85).aspx or http://developer.apple.com/documentation/AppleApplications/Conceptual/SafariJSProgTopics/Tasks/CopyAndPaste.html for details. Your actual code could be the starting point for a so much needed onselectionchange event, though.

Opening a context menu does not "manifest the intention of performing a clipboard operation". You haven't explained what Mozilla's behavior breaks, while it's clear what IE's behavior breaks.

Fwiw, this is an IE invention, so it would seem best to me to be compatible with IE.

(In reply to comment #106) > I'd really like for someone that is actually planning on using this to speak > up. > > Kaply; do you have any js code you could share? > Unfortunately not. But I have to agree with the other sentiments here. These features should be about IE compatibility. Not inventing our own way to do this.

(In reply to comment #110) > These features should be about IE compatibility. "IE compatibility" is a matter of debate. Yes, the events are fired at different times. But a reasonable implementation of the event handlers will result in the same end-user experience. (In reply to comment #107) > ... these onBeforeXXX and onXXX events should give developers > a chance to CANCEL the default action associated with the corresponding > clipboard operation and implement a custom clipboard behavior. This may be what the documentation indicates, but it does not make logical sense when compared with the actual implementation. How is the event supposed to change the default action? The only function it can perform is to disable the menu option, which is the same functionality that Mozilla's implementation has. (In reply to comment #107) > They are not badly named as you claimed in comment #99. "beforecut" is fired (even in IE, Safari) when the user has made no attempt to cut text. Doesn't that seem like the event name is wrong? (In reply to comment #102) > ... It's also been mentioned the ability to change what is actually > cut/copy/pasted. Is there anything else? I'm a little surprised by the second > example, I thought that that was what oncut/oncopy/onpaste was for, not the > 'before' events. This is not something that the before* events can do, even in IE. As long as cut/copy/paste events remain, I don't care if the before events are removed. I think they're pretty much pointless, but also pretty much harmless.

(In reply to comment #111) > As long as cut/copy/paste events remain, I don't care if the before events are > removed. I think they're pretty much pointless, but also pretty much harmless. Bug 394818 is actually a crash that's partly dependent on "onbeforecopy" firing (and causing frame destruction) after a focus change in the middle of frame destruction. That's partly symptomatic of a deeper focus-related bug, but if onbeforecopy were removed (or were made to behave like IE and fired less), that bug would be fixed.

(In reply to comment #109) > Fwiw, this is an IE invention, so it would seem best to me to be compatible > with IE. I think we all agree with that. The question is what it means to be "compatible with IE". It doesn't necessarily mean that we're firing the events at exactly the same time. It does mean that code written for IE will work in Firefox.

I filed now bug 415938 for the test failure, as this bug gets a little big.

I filed bug 420985 for the tests, this code landed, and keeping this bug open for the tests is just confusing. (Note that support for the onbefore* events was backed out in bug 418457.)

These are now documented.

Is there an actual security issue with implementing the beforeXXX events, or is it just confusion over the intent of these events? Some of the assessment above is incorrect (admittedly the documentation is mostly misleading or incorrect as well). The beforeXXX events are mainly intended to update the enabled/disabled state of the UI. The IE documentation for onbeforecut/onbeforecopy/onbeforepaste states this and all of the examples use the events this way (and only in that way) See, for example, http://msdn.microsoft.com/en-us/library/ms536902%28VS.85%29.aspx The problems found by Daniel's testing done in bug 410858 isn't as noticable in current builds, possibly due to focus handling improvements. The tests are somewhat bogus since they display alerts; this can cause focus changes which can affect what happens (moreso on Linux).

> Is there an actual security issue with implementing the beforeXXX events Depends on when they're fired and whether they give the page the ability to fully control what ends up in the clipboard. > The beforeXXX events are mainly intended to update the enabled/disabled state > of the UI. This is mentioned above, yes. As well as the fact that IE is inconsistent in firing them, that they don't work well for keyboard copy/paste, etc, right?

(In reply to comment #117) > Is there an actual security issue with implementing the beforeXXX events Yes. My initial investigation here was due to bug 394818, a double-free that depended on onbeforecopy. See in particular bug 394818 comment 7 -- basically, the issue there was that a style-change can cause frame destruction, which can implicitly change our focus (in the middle of the destruction), which can cause "onbeforecopy" to fire, which can trigger more frame destruction (while we're already in the middle of frame destruction). If we can guarantee that won't happen anymore, then I'm less concerned about this.

(In reply to comment #118) > > Is there an actual security issue with implementing the beforeXXX events > > Depends on when they're fired and whether they give the page the ability to > fully control what ends up in the clipboard. I wouldn't think we'd provide clipboard access during those events. > This is mentioned above, yes. As well as the fact that IE is inconsistent in > firing them, that they don't work well for keyboard copy/paste, etc, right? Not sure what comment you're referring to above, but IE (and Safari and Chrome) always fire the beforeXXX event before the cut/copy/paste event when using the keyboard shortcut. > If we can guarantee that won't happen anymore, then I'm less concerned about > this. Command updating that occurs from selection or focus changes are now run using AddScriptRunner.