1279771 - Issue with execCommand('copy') in change events in e10s

Status: NEW

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

Description

[ulrichb]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160604131506

Steps to reproduce:

See the following reproduction sample.

https://jsfiddle.net/Ulrich/8qd5x23h/


Actual results:

In Firefox 49a2 (2016-06-11) *with enabled e10s* the "execCommand('copy')" in the "change" event handler returns "false" and doesn't copy the value to the clipboard.

With disabled e10s it works.

Note that the "execCommand('copy')" during an "input" event works with e10s.

Tested on Windows 8.1.


Expected results:

"execCommand('copy')" should work with enabled e10s in an user induced "change" event.

Comment 1

[Alice0775 White]

Bug 1012662 fixed the "execCommand('copy')" in non-e10s mode, but not fixed in e10s mode.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

The problem in this case is that the execCommand('copy') event requires EventStateManager::StartHandlingUserInput(); to have been called higher in the stack.

This isn't happening with the select element because the dropdown is displayed in the parent process, and thus the input event is only occurring in the parent process. The child process isn't recorded as handling input when the callback is being run.

To fix this this handler: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/SelectContentHelper.jsm#114 needs to be wrapped in calls to StartHandlingUserInput and StopHandlingUserInput. Unfortunately currently those functions are only visible to C++.

Ideally we'd expose a `withHandlingUserInput(cb)` function which would call the cb closure surrounding it with calls to StartHandlingUserInput and StopHandlingUserInput to chrome JS. I'm not sure what the best way to do this would be right now, as we'd need to expose it on some object, and I'm not sure which one to choose.

Comment 3

[Jim Mathies [:jimm]]

Mike, how serious is this in your opinion?

Comment 4

[Mike Taylor [:miketaylr]]

Here's what I'm seeing on my machine:
              

                   | change | input |
-------------------------------------
Mac Fx 49 e10s     | false  | true  |
Mac Fx 49 non-e10s | true   | true  |
Win Fx 49 e10s     | false  | true  |
Win Fx 49 non-e10s | true   | true  |
Mac Chrome 53      | false  | true  |
Mac Safari Te.Prev | false  | true  |
Win Chrome 53      | true   | true  |
Edge 38            | false  | false |


So on Mac, e10s aligns with what Chrome and Safari do today. 

On Windows, e10s diverges from what Chrome does. I'm guessing Edge just doesn't support execCommand('copy') yet.

Given the divergence between platforms, I'd be surprised if this breaks web-facing content (unless that relied heavily on UA sniffing).

ulrichb, are you aware of any sites that this change breaks?

Comment 5

[ulrichb]

@Mike 
I mentioned this issue during testing an internal application in Fx Dev Edition with enabled e10s. The application uses the same pattern with a `select`-element and the `change`-event as in my repro sample.

Please note that on the Windows platform the `execCommand('copy')` inside of a `change`-event is supported in Chrome, Fx w/o e10s, and IE (11). => In the vast majority of used browsers, and therefor I would expect more users facing this issue as soon a e10s gets into production. 

BTW: The fact, that Microsoft dropped support for `execCommand('copy')` in Edge at all is now a second issue in our internal application :)

Comment 6

[Mike Taylor [:miketaylr]]

Thanks Ulrich. I'm curious how you all are working around Chrome and Safari on Mac, since they don't support this using a change event?

Comment 7

[Jim Mathies [:jimm]]

>                    | change | input |
> -------------------------------------
> Mac Fx 49 e10s     | false  | true  |
> Mac Fx 49 non-e10s | true   | true  |
> Win Fx 49 e10s     | false  | true  |
> Win Fx 49 non-e10s | true   | true  |
> Mac Chrome 53      | false  | true  |
> Mac Safari Te.Prev | false  | true  |
> Win Chrome 53      | true   | true  |
> Edge 38            | false  | false |

We need to fix the difference between e10s and non-e10s, but due to the mixed results not blocking.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

I should have a patch for this up shortly, just need to write some tests etc.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Treat change events triggered by changing <select> element selections as user-initiated in e10s

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe73f3d7e160
Treat change events triggered by changing <select> element selections as user-initiated in e10s, r=enndeakin

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c676d55b6b00
Backout due to linux test failures

Comment 12

[Neil Deakin]

I notice that nsIDOMWindowUtils already has a setHandlingUserInput method. This patch should be changed to just use the existing method rather than add a new one.

Comment 13

[Nika Layzell [:nika] (ni? for response)]

(In reply to Neil Deakin from comment #12)
> I notice that nsIDOMWindowUtils already has a setHandlingUserInput method.
> This patch should be changed to just use the existing method rather than add
> a new one.

Thank you for noticing that! I can't believe I missed it when I was writing the initial patch.
I'll have a new patch up in a minute.

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Attachment: Treat change events triggered by changing <select> element selections as user-initiated in e10s

This new patch should theoretically fix both the timeout issues on infra, as well as remove the unnecessary new idl functions on nsDOMWindowUtils

Comment 15

[Nika Layzell [:nika] (ni? for response)]

(In reply to Michael Layzell [:mystor] [:mrl] from comment #14)
> Created attachment 8768954 [details] [diff] [review]
> Treat change events triggered by changing <select> element selections as
> user-initiated in e10s
> 
> This new patch should theoretically fix both the timeout issues on infra, as
> well as remove the unnecessary new idl functions on nsDOMWindowUtils

I'm coming back to this patch for the first time in a while. IIRC I stopped working on this patch because the test consistently failed on infra, but not in my local builds, and I had no idea what to do. The problem was that when the test was running the parent process would try to discover the context menu to click on it, but the context menu would not be there. 

Neil, do you have any ideas about how we could potentially redesign the test such that it doesn't depend on unreliable timing of things such as context menus appearing in the chrome process?

Comment 16

[Neil Deakin]

I'm unclear how the patch here relates to context menus. Do you mean the popup for the <select>? Do you want to wait for the popup to open? Perhaps load the chrome script before clicking and have it wait for the popupshown event.

Alternatively, maybe it would be easier to make this a browser test (part of browser_selectpopup.js)

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 18

[Nika Layzell [:nika] (ni? for response)]

Unassigning as I don't have time to work on this.