Closed
Bug 1185052
Opened 10 years ago
Closed 5 months 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)
Core
DOM: UI Events & Focus Handling
Tracking
()
RESOLVED
INVALID
People
(Reporter: nika, Unassigned)
References
Details
(Keywords: parity-chrome)
Attachments
(3 files)
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).
|
||
Comment 1•10 years ago
|
||
We'll need to add some hack to the JS engine part to make it work.
|
|
||
Comment 2•10 years ago
|
||
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.
|
|
||
Comment 3•10 years ago
|
||
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)
|
||
Comment 4•10 years ago
|
||
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
|
|
||
Comment 5•10 years ago
|
||
(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.
|
||
Comment 6•10 years ago
|
||
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.
|
|
||
Comment 7•10 years ago
|
||
Ping :dveditz. Any thoughts here?
|
||
Comment 8•9 years ago
|
||
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
|
|
||
Updated•9 years ago
|
Whiteboard: [parity-blink]
|
||
Comment 9•8 years ago
|
||
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.
|
||
Updated•7 years ago
|
Priority: -- → P2
|
||
Comment 10•7 years ago
|
||
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]
|
||
Comment 11•6 years ago
|
||
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.
|
|
||
Updated•6 years ago
|
Type: defect → enhancement
|
||
Comment 12•6 years ago
|
||
|
|
||
Updated•6 years ago
|
Component: DOM: Events → User events and focus handling
|
|
||
Comment 13•6 years ago
|
||
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?
|
|
||
Comment 14•6 years ago
|
||
|
|
||
Updated•6 years ago
|
Assignee: nobody → echen
|
|
||
Comment 15•6 years ago
|
||
|
|
||
Comment 16•6 years ago
|
||
|
|
||
Comment 17•6 years ago
|
||
Propagate UserActivaionData through following async callbacks,
- setTimeout
- setInterval
- requestIdleCallback
- requestAnimationFrame
|
|
||
Updated•5 years ago
|
Flags: needinfo?(dveditz)
|
|
||
Updated•5 years ago
|
Flags: needinfo?(bugs)
|
|
||
Updated•5 years ago
|
Assignee: echen → nobody
Priority: P2 → P5
|
|
||
Updated•5 years ago
|
|
||
Updated•2 years ago
|
Severity: normal → S3
|
|
||
Comment 18•5 months ago
|
||
We've implemented the activation propagation to match the current spec'ed behavior.
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•