Open Bug 1185052 Opened 9 years ago Updated 2 years ago

Consider extending IsHandlingUserInput to also return true in event callbacks triggered by a user event

Categories

(Core :: DOM: UI Events & Focus Handling, enhancement, P5)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
enhancement

Tracking

()

People

(Reporter: nika, Unassigned)

References

Details

(Keywords: parity-chrome)

Attachments

(3 files)

Bug 1185052 - Part 1: Move user-activation code from EventStateManager to UserActivation;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1185052 - Part 2: Add UserActivationData;
47 bytes, text/x-phabricator-request
Details | Review
Bug 1185052 - Part 3: Propagate user-interaction information through async callbacks;
47 bytes, text/x-phabricator-request
Details | Review 
Right now we only allow calling functions which require user input (such as fullscreen or document.execCommand('cut'/'copy')) when the user event callback is on the stack, and a timeout of 1 second has not elapsed. We may want to extend that functionality to also support calling these functions within event callbacks which are triggered by the user event (such as websocket or XHR events). The timeout would still be in effect, meaning that such copy or cut events would have to occur within a reasonable timeframe of a user initiated event.

bug 1012662 comment 53 includes some justification as to why this might be useful. According to bug 1012662 comment 51 this is supported in chrome (although I haven't tested this myself).
We'll need to add some hack to the JS engine part to make it work.
Well, probably no. And that probably won't work.

I played a little with Chrome on that, and also investigated its code. It seems to me that, Chrome allowing that kind of behavior is not intentional. It seems to be some kind of side effect of their impl which I don't fully understand. I really suspect it is a feature from bug, and doesn't always work properly as expected.

See the following code, in which, text is <input type="text" value="some text">
> var text = document.getElementById("text");
> function tryCut() {
>   text.focus();
>   text.setSelectionRange(0, text.value.length);
>   document.execCommand("cut");
> }

If I use code:
> text.onclick = () => setTimeout(tryCut, 1000);
it works. However, if I add one more line:
> setInterval(tryCut, 500);
then the code above doesn't work anymore.

Also, if I use the following code instead:
> text.onclick = () => setTimeout(() => setTimeout(tryCut, 100), 100);
it doesn't work either.

This result indicates that, Chrome doesn't do exact stack check or callback track.
It seems to me that tracking callbacks from user input event precisely is non-trivial. If the usecases are really important, I'd rather suggest we remove the stack check, and just allow any request within 1s from the last user input.

It would allow code like "setInterval(doSomethingEvil, 500);" to work. But it doesn't seem to me it would do anything more harmful than what content can do currently. An attacker who is able to set interval on a page is also able to register listeners on all user input event and call "doSomethingEvil" there.

smaug, dveditz, does this change sound reasonable to you?

FYI, the stack check of user input was initially implemented in bug 252326, and the time limit was added in bug 684627.
Flags: needinfo?(dveditz)
Flags: needinfo?(bugs)
A use case described in bug 1012662 [1] is fetching content meant for the local clipboard with an XHR request. So user triggered gesture => XHR request => event handler => write to clipboard. I don't think a general 1 second timeout is a good solution to that use case - it just creates a race condition where the command will sometimes fail for no apparent reason if the network request is a little slow.

Now, based on 

data:text/html, <body onclick="setTimeout(function(){window.open('http://example.com');}, 500)">hi</body>

I wonder if setTimeout() tracking already works OK. I vaguely recall Opera (Presto) had to rewrite the popup blocker implementation to support setTimeout().

Based on http://jsfiddle.net/bwun8fqk/1/ event listeners on an XHR request started from a user-triggered thread are not considered user triggered. (Not surprising).

IMHO domain whitelist with extended permissions is a better solution - at least regarding the clipboard stuff.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1012662#c53
(In reply to Hallvord R. M. Steen [:hallvors] from comment #4)
> A use case described in bug 1012662 [1] is fetching content meant for the
> local clipboard with an XHR request. So user triggered gesture => XHR
> request => event handler => write to clipboard. I don't think a general 1
> second timeout is a good solution to that use case - it just creates a race
> condition where the command will sometimes fail for no apparent reason if
> the network request is a little slow.

They will meet this everywhere. Chrome also blocks clipboard access after 1s. And I think remote desktop products should generally use WebSocket or something similar, instead of per-request XHR, hence the delay should normally be low enough.

Also, if it takes more than 1s, the experience itself has been broken enough. Making it fail silently isn't a real issue. Users usually use content immediately after they copy it. Can you imagine that, when you copy something, you cannot paste it until 2 or 3s later? Or think about, when you cannot paste it, and you turn to copy something else, but the old content suddently kicks in after several seconds?

> Now, based on 
> 
> data:text/html, <body
> onclick="setTimeout(function(){window.open('http://example.com');},
> 500)">hi</body>
> 
> I wonder if setTimeout() tracking already works OK. I vaguely recall Opera
> (Presto) had to rewrite the popup blocker implementation to support
> setTimeout().

It seems yes, we explicitly tracks popup for timeout and interval. That happens in nsGlobalWindow::SetTimeoutOrInterval() and nsGlobalWindow::RunTimeoutHandler(). The code there is very specific, and is probably hard to generalize.
Want to apologize for taking so long on the ni?  I'm on vacation and I'll continue to let this roll around in the back of my head and try to give you and answer when I get back next week.
Ping :dveditz. Any thoughts here?
Some additional justifications:

1) WebExtensions browser_action clicks, page_action clicks, context menu clicks, and keyboard shortcut commands

Currently, calling execCommand('copy') directly from these respective handlers fails due to this bug.

2) Using setTimeout() to rate-limit clicks or detect double/triple click

Example use case: TabCopy (https://chrome.google.com/webstore/detail/tabcopy/micdllihgoppmejpecmkilggmaagfdmb), an extension I'm currently attempting to port to Firefox as a WebExtension. It distinguishes between browser_action single/double/triple click to determine the scope of the copy.

3) Using a WebExtensions API, many of which are async, as the source of text to copy

Eg, browser.tabs.query()


FWIW, I created the following tool to test support for sync/async use of execCommand('copy'):
Clipboard Copy Test:  http://hansifer.com/clipboardCopyTest.html
Whiteboard: [parity-blink]
Blocks: 1382574
https://github.com/whatwg/html/issues/1903 seems relevant here as far as specifying this behavior in standards goes and coordinating with other browsers about what it should be.
Priority: -- → P2
No longer blocks: 1382574
Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)
Keywords: parity-chrome
Whiteboard: [parity-blink]
See Also: → 1441881
See Also: → 1478423
Blocks: 1487542
GeckoView has run into some more use cases impacted by this bug.  Focus uses EventStateManager to determine when to clear its search bar when navigating away from a search results page, and this is broken on DuckDuckGo because their link click handler uses a timeout (bug 1487542).  Also, some sites that use Facebook or Google login trigger the popup blocker when they shouldn't, possibly because of this bug.
Blocks: 1494123
Blocks: 1509933
Type: defect → enhancement
Component: DOM: Events → User events and focus handling

We should also consider having some mechanism to consume the user interaction due to some bug reported and also spec discussion. The Chrome support this, but it has only one token, so it doesn't work for the page that for example want to enter the fullscreen and also play video in a single user interaction. So maybe we should have separated token for different operation, which probably gives better user experience?

Assignee: nobody → echen
Depends on: 1558776

Propagate UserActivaionData through following async callbacks,

  • setTimeout
  • setInterval
  • requestIdleCallback
  • requestAnimationFrame
See Also: 1441881 →
No longer blocks: 1129227
No longer blocks: 1557301
No longer blocks: 1509933
Flags: needinfo?(dveditz)
Flags: needinfo?(bugs)
Assignee: echen → nobody
Priority: P2 → P5
No longer blocks: 1494123
See Also: → 1494123
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: