415921 - Onbeforecut/onbeforecopy/onbeforepaste events don't fire anymore when context menu isn't used

Status: RESOLVED WORKSFORME

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

Description

[Martijn Wargers (dead)]

Note that the decision to fire onbeforecut/onbeforecopy/onbeforepaste events when something is focused was made in bug 390238. That may be a debatable issue, see the comments in that bug.
But the onbefore.. events also don't fire anymore when you actually cut/copy/paste anything using the keyboard. That is definitely a bug.

This is the regression range (at 2008-01-25, between 20:00 and 23:00, appr):
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1201323900&maxdate=1201328279
So I guess this is a regression from bug 404232.

What I find strange here is that the behavior of the onbeforecut/onbeforecopy/onbeforepaste events apparently depends on the UI of the browser, that seems really bad to me.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Looking...

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Still looking for the cause of this...

Given that bug 404232's fix didn't have the Tp impact I expected (although it may have had other beneficial performance impacts that aren't measured by our current tests), I wonder whether the best answer here is simply to back that patch out.

Comment 3

[Mike Schroepfer]

Probably worth considering

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: backs out fix for bug 404232

The onbefore(cut|copy|paste) events fire whenever we call the IsCommandEnabled method of the controllers for those commands (f.e. nsPasteCommand::IsCommandEnabled calls nsPlaintextEditor::CanPaste, which calls nsPlaintextEditor::FireClipboardEvent).

We used to call IsCommandEnabled whenever focus changed, but since the fix for bug 404232 we only call it on focus changes when some edit UI is visible (the Edit menu, the context menu, the places context menu, or the Cut/Copy/Paste toolbar buttons).

That should be ok, as invoking one of those commands via the keyboard shortcuts is supposed to call globalOverlay.js's goDoCommand, which then calls IsCommandEnabled before actually executing the command.  But for some reason the keyboard shortcuts aren't calling goDoCommand.

I haven't been able to figure out why that is yet.  I thought I tested this case when working on bug 404232, but perhaps I missed this variant, or maybe another recent checkin regressed it.

I could dig in deeper, but at this point I think it's probably best to simply back out the fix for bug 404232, taking the performance hit (and regressing the inadvertent fix for bug 390238) in exchange for reverting to a known "good" state.

Here's a patch that does that.

Comment 5

[Martijn Wargers (dead)]

Well, the fact that changes in the brower/UI code can cause this bug is what baffles me. It seems to me that should not be happening at all.
I mean, those onbefore.. events should be working in the same way, whether it's run inside a browser or not, no?

Comment 6

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #5)
> Well, the fact that changes in the brower/UI code can cause this bug is what
> baffles me. It seems to me that should not be happening at all.
> I mean, those onbefore.. events should be working in the same way, whether it's
> run inside a browser or not, no?

I'm not sure they should work in exactly the same way, but whatever executes those commands should certainly be checking IsCommandEnabled first.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Aren't keyboard shortcuts done via XBL command keys?  And yes, perhaps those should be checking whether the command is enabled.  nsXBLPrototypeHandler::DispatchXBLCommand doesn't seem to do that, as far as I can tell.  Someone familiar with command controllers and such want to take a look at that code?

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Jonas, are you familiar with nsXBLPrototypeHandler::DispatchXBLCommand and know what might be going wrong here?

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Not enough no. But more importantly I think we're going to back out support for the onbeforecut/onbeforecopy/onbeforepaste events. They aren't working very well and the use cases are dubious.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

bug 418457 is now fixed, so I guess this could be marked as WFM or something.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Bug 418457 removed support for onbeforecut/copy/paste, so this should be WFM, feel free to reopen if you think there are remaining issues.

Comment 12

[Stuart Parmenter]

jonas says WFM