Closed Bug 1476612 Opened 2 years ago Closed 2 years ago

key events should be considered user interactions when received by editable HTML elements

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(1 file, 1 obsolete file)

https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/dom/events/EventStateManager.cpp#887-894

I don't see why this is a valid statement. Here a scenario:

1. site.com does a window.open(page.html)
2. page.html shows a a form and it appears in a popup window.
3. the user clicks on a <input type=text> element.

The document should be marked as user-interacted.
Summary: key events should be consider user-interaction when received by editable HTML elements → key events should be considered user interactions when received by editable HTML elements
Attached patch editable.patch (obsolete) — Splinter Review
Attachment #8992960 - Flags: review?(cpearce)
It's important to share why I found this issue:

for the anti-tracking project, we grant first party storage access to tracking resources after user interactions. In my testing, I saw that typing something into a <input type=text> was not enough. Basically I'm using the user-interaction monitoring for something that is different than media autoplaying. It behaves perfectly, except for editable HTML elements.
Priority: -- → P3
baku, does this have anything to do with bug 1359867? For that change, we've been debating what counts as a user interaction.
Flags: needinfo?(amarchesini)
Not at the moment. The concept of user-interaction here is taken from media autoplay. See bug 1470346.
We can use part of this approach for allow-top-navigation-by-user-activation iframe sandbox. I'll follow that bug.
Flags: needinfo?(amarchesini)
Comment on attachment 8992960 [details] [diff] [review]
editable.patch

Review of attachment 8992960 [details] [diff] [review]:
-----------------------------------------------------------------

When block autoplay is enabled, we don't want to activate for events sent to editable elements. It would be confusing if video started playing when you were typing something. Ideally we only want to gesture activate on a "play" button click/submit. So we don't want this change for our use case.

Maybe what you want for your use case is to use nsIDocument::UserHasInteracted() [1], instead of nsIDocument::HasBeenUserGestureActivated()?

UserHasInteracted() doesn't filter out events targeted at editable elements [2]. Also note that when one document in a doctree is gesture activated (for HasBeenUserGestureActivated()), every document the same tab is (effectively) gesture activated, including descendant document, which may not be what you want here. UserHasInteracted() only activates the current document, and ancestor documents. Also "gesture activation" only activates for presses of printable keys, so key events which are likely to be keyboard shortcuts don't activate. Whereas the "user has interacted" feature doesn't filter those out.

So I suggest you either:
1. Use nsIDocument::UserHasInteracted() instead of nsIDocument::HasBeenUserGestureActivated(), or,
2. Refactor the HasBeenUserGestureActivated() feature to instead store some kind of bitmask so we can both get the behaviour we want out of the one feature. Maybe we could merge both UserHasInteracted() and HasBeenUserGestureActivated() into one, as it's confusing having two.


[1] https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/dom/base/nsIDocument.h#3373
[2] https://searchfox.org/mozilla-central/rev/c296d5b2391c8b37374b118180b64cca66c0aa16/dom/events/EventStateManager.cpp#508
Attachment #8992960 - Flags: review?(cpearce) → review-
Attached patch editable.patchSplinter Review
Thanks for your comment!
Attachment #8992960 - Attachment is obsolete: true
Attachment #8993136 - Flags: review?(cpearce)
Comment on attachment 8993136 [details] [diff] [review]
editable.patch

Review of attachment 8993136 [details] [diff] [review]:
-----------------------------------------------------------------

Easy as! :)

Commit message needs updating.
Attachment #8993136 - Flags: review?(cpearce) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/694089774911
AntiTracking should use nsIDocument::Get/SetUserHasInteracted instead of UserGestureActivation, r=cpearce
https://hg.mozilla.org/mozilla-central/rev/694089774911
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.