583638 - Copying doesn't update the system clipboard on Firefox trunk

Status: RESOLVED FIXED

(Skywriter Graveyard :: Editor, defect, P1)

Description

[Nickolay_Ponomarev]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.0b3pre) Gecko/20100729 Minefield/4.0b3pre

With https://bespin.mozillalabs.com/ (this also causes problems for mozmill - bug 583631 and jetpack's addons builder - no bug filed, afaik):

1. Select text
2. Cmd+C
3. Cmd+V

Actual results: the selected text is replaced with whatever was in the system clipboard before copying

Expected results: pasted text is exactly the same as copied.

I currently have this regression range for Firefox:
2010-01-01-03-mozilla-central - works
2010-04-26-03-mozilla-central - fails
...but I'm not up to minimizing it further today or minimizing bespin into a testcase that could be filed as a Core bug.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Does that mean that also older (stable) versions of Bespin are affected by this bug?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Btw, the same can be seen with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.9pre) Gecko/20100730 Namoroka/3.6.9pre

Comment 3

[Nickolay_Ponomarev]

I don't know. It's interesting that you say that 3.6.9pre is also affected, since 3.6.8 works fine for me.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Please scratch my last comment. Forgot the -b switch on the command line and run with Minefield. Namoroka is working fine, so it's really a regression on trunk only.

Should we move this over to Core?

Comment 5

[Nickolay_Ponomarev]

Feel free. I was wary of filing a bug using something as complicated as bespin as a testcase and was hoping to get help with minimizing it from bespin devs.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That sounds reasonable. So lets wait. Having a definitive regression data would be helpful too. I don't really have time in the next days to work on that. The only thing I have noticed that in the range you have already specified, Bespin doesn't work very well. I only get a gray box nothing more. That will make it a bit harder for me to find the regression range.

Comment 7

[Julian Viereck]

(In reply to comment #6)
> That sounds reasonable. So lets wait. Having a definitive regression data would
> be helpful too. I don't really have time in the next days to work on that. The
> only thing I have noticed that in the range you have already specified, Bespin
> doesn't work very well. I only get a gray box nothing more. That will make it a
> bit harder for me to find the regression range.

The key handling code is here: http://hg.mozilla.org/labs/bespinclient/file/c13149ad4794/plugins/supported/text_editor/views/textinput.js#l159

It shouldn't be hard to put together a small html page that is based on the textinput.js file. Let me know if that would be helpful.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

If you have the time a simple test would really be helpful. As sooner as we know what has been regressed here, our chances are better to get this hopefully fixed for the final version of Firefox 4.0.

Comment 10

[Julian Viereck]

Attachment: Testcase

The HTML file attached contains a reduction of the Bespin key handling code. This code only handles copy, cut and paste (there is an introduction in the HTML file how to use it).

Paste works on the latest Minefield. Copy and cut works in Firefox 3.6.8 but not in Minefield.

As far as I can see, the "copy" and "cut" events are not fired that are bound to the textarea. See line 219 in the HTML file. The MDC says (see [1] and [2]) that theses events should be working since Firefox 3.

[1] https://developer.mozilla.org/en/DOM/element.oncopy
[2] https://developer.mozilla.org/en/DOM/element.oncut

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

For me the alert gets fired but nothing is copied to the clipboard. This bug is x-platform. Julian, would you have time to get a more granular regression range, as what we have right now from comment 0?

Comment 12

[Julian Viereck]

(In reply to comment #11)
> For me the alert gets fired but nothing is copied to the clipboard. 

What platform/Minefield version is that?

> Julian, would you have time to get a more granular regression
> range, as what we have right now from comment 0?

I've got some other stuff on my plate (new developer tools). Maybe next week.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #12)
> (In reply to comment #11)
> > For me the alert gets fired but nothing is copied to the clipboard. 
> 
> What platform/Minefield version is that?

Each platform and the Minefield build from yesterday.

Comment 15

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

This is pretty painful for addon developers using Add-on Builder, so it's a high priority for the Jetpack project anyway.  I filed bug 585746 to track the issue on the Add-on Builder side of things (that bug depends on this one) and narrowed the regression range to the time between the March 19 and 20 nightly builds, i.e. between these two user agents:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a4pre) Gecko/20100319 Minefield/3.7a4pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a4pre) Gecko/20100320 Minefield/3.7a4pre

I can also confirm that this is cross-platform, as I see it on both Linux and Mac; and bug 582026, which seems like a dupe, reports the problem on Windows.

Comment 16

[Nickolay_Ponomarev]

Myk, could you get the revision from about:buildconfig of both builds, post the http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=xxxxxxxxxxxx&tochange=xxxxxxxxxxxx URL here?

Comment 17

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

(In reply to comment #16)
> Myk, could you get the revision from about:buildconfig of both builds, post the
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=xxxxxxxxxxxx&tochange=xxxxxxxxxxxx
> URL here?

Ah, of course, here it is:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5108c4c2c043&tochange=45338ed951a7

Of those, http://hg.mozilla.org/mozilla-central/rev/27259a0fcbe6 (bug 501154) seems the obvious candidate, so cc:ing Neil and smaug.

Comment 18

[Neil Deakin]

I'm not sure what you're expecting, but the testcase seems to imply that the Copy command should work even when it is disabled. The copy command is only enabled when the focused element or document has a non-collapsed selection, and I can't seem to create one for the 'Hello World!' text in the testcase. I'm guessing from the behviour that you're trying to fake a copy by having a focused textbox. You'll need to have selected text in it though if you expect a copy to work.

Comment 19

[Julian Viereck]

(In reply to comment #18)
> I'm not sure what you're expecting, but the testcase seems to imply that the
> Copy command should work even when it is disabled. The copy command is only
> enabled when the focused element or document has a non-collapsed selection, and
> I can't seem to create one for the 'Hello World!' text in the testcase. I'm
> guessing from the behviour that you're trying to fake a copy by having a
> focused textbox. You'll need to have selected text in it though if you expect a
> copy to work.

Neil, thanks for pointing this out. Yes, the intention of this code is to "fake" the copy command. Whenever "copy" is detected, a internal function is called to get the text that should be copied.

I was able to set the value of the textbox and select the value before the "copy" command is executed using this workaround:

    textField.addEventListener('keydown', function(evt) {
        if (evt.keyCode == 67 && (evt.metaKey || evt.ctrlKey)) {
            self._setValueAndSelect("foobar");
        }
    }, false);

When the the copy-key-combo is detected, it sets the value of the textbox to "foobar". Although this works, I'm not pleased with this solution.

WebKit has a "beforecopy"|"beforecut" event. This event is fired before the "copy"|"cut" event is fired. On WebKit browsers, I listen to this event, set the content of the textbox, select it and when the "copy"|"cut" command is executed, it copies the content of the textbox to the clipboard.

Neil, do you think such an event could be added to Gecko as well? If you have another idea to solve this issue, I'm more then happy to hear it.

Comment 20

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

Yes, beforecopy/cut could be added. In fact those used to be supported for a short
while but were backed out because of some problems, see Bug 418457.

Comment 21

[Neil Deakin]

(In reply to comment #19)
> When the the copy-key-combo is detected, it sets the value of the textbox to
> "foobar". Although this works, I'm not pleased with this solution.

It also isn't very portable since it will fire when the wrong key is pressed (and not at all if a user has configured the Alt key for shortcuts or something).

> Neil, do you think such an event could be added to Gecko as well? If you have
> another idea to solve this issue, I'm more then happy to hear it.

Yes, we could add the 'before' events but see http://enndeakin.wordpress.com/2010/03/04/clipboard-data/ for my thoughts on this.

The main issue is lots of extra events firing as we need to fire all three events on every focus and selection change.

Also, IE only implements the beforeXXX events for <input> and/or when a selection is present and you can only disable the command. There is no means to enable the command when it would not normally be enabled. That may be suitable for this case, but it isn't a good general solution.

Comment 22

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

Julian: is your workaround in comment 19 sufficient to solve this problem in Bespin for now, or do we need a Gecko fix (like the onbeforecopy/cut/paste events)?  Either way, I want to make sure we get a fix for Firefox 4/Gecko 2, since it's important for Add-on Builder.

Comment 23

[Julian Viereck]

(In reply to comment #22)
> Julian: is your workaround in comment 19 sufficient to solve this problem in
> Bespin for now, or do we need a Gecko fix (like the onbeforecopy/cut/paste
> events)?  Either way, I want to make sure we get a fix for Firefox 4/Gecko 2,
> since it's important for Add-on Builder.

The problem with the workaround as described in comment 19 is, that you have to ensure there is always a selection. This is doable for most characters + arrow movements (if the user hits a key combo that moves the selection or cursor, then there won't be any more a selection but that should be able to work around).

However, in the case of "combined" characters, this doesn't work out ("combined" characters are characters combined of two key strokes, e.g. ¨ + u -> ü). The cursor have to stay after the first typed character to get combined with the second character.

To solve this problem the key detection code and the Bespin editor have to work a little bit hand in hand. When there is no selection in Bespin there is also no need to select something in the hidden textarea because copy or cut can't take place. If there is a selection in Bespin, then something have to be selected in the textarea as well. In the case of the "combined" characters this means, when the user types ¨ there is no selection in the editor anymore (if there was a selection before, then it's overridden by the ¨ character) so the textarea position can stay where it is. Hitting the "u" key results in "ü" then.

As far as I can think of it this should work out and shouldn't be to hard to implement as we have a selection event in Bespin already.

Myk, what's the timeframe till this have to be done? There are some Firefox b5 blockers on my plate that have to be done till end of this month. Is that okay if I go for this afterwards?

Comment 24

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

(In reply to comment #21)
> Yes, we could add the 'before' events but see
> http://enndeakin.wordpress.com/2010/03/04/clipboard-data/ for my thoughts on
> this.

This blog post makes it sound like implementing a platform fix here, especially in the Fx4 timeframe, will be hard.


(In reply to comment #23)
> As far as I can think of it this should work out and shouldn't be to hard to
> implement as we have a selection event in Bespin already.

That sounds like the better approach for the short run, given the apparent difficulty of deciding on and implementing a platform fix.


> Myk, what's the timeframe till this have to be done? There are some Firefox b5
> blockers on my plate that have to be done till end of this month. Is that okay
> if I go for this afterwards?

Yes, working on this after the end of the month is fine.  The sooner the better, but it becomes really important once we start talking about Add-on Builder more broadly in conjunction with a push to get addon developers to start updating their addons for Firefox 4.

That push is currently tentatively scheduled for the end of September, so if we can get a fix for this in the first half of September that we can then deploy in the second half, it'll be in good time.

Comment 25

[Julian Viereck]

Pushed changeset

  http://hg.mozilla.org/labs/bespinclient/rev/8ba0e6ff5592

that should fix this problem. If there is selection in the editor then the value of the hidden textarea is set to "z" and selected. Otherwise the value of the textarea is set to "". This also makes the copy/cut buttons in the FF toolbar work (they are disabled if there is nothing to copy/cut and activated if there is something to copy/cut).

Also, the "input" event is used now to determ if the user has typed something.

Comment 26

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

Piotr: can you confirm that the Bespin fix described in comment 25 addresses the bug in FlightDeck that you can't copy code from the addon code editor into the clipboard on Firefox 4 beta and nightly builds?

Comment 27

[Pushpender]

While I pasting the result..I got another type of error.
Alert box with title GetLastError
and text Not enough storage is available to process the command.

Comment 28

[Piotr Zalewa [:zalun]]

We will wait for the editor change and see what will happen next - this should happen very soon