1452536 - Don't gesture activate documents upon interaction with the browser/OS

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement, P2)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Currently we gesture activate a document whenever there's a key up event:
https://searchfox.org/mozilla-central/rev/7ccb618f45a1398e31a086a009f87c8fd3a790b6/dom/events/EventStateManager.cpp#826

This results in an unpleasantly surprising user experience, as keyboard shortcuts that the user intended to direct at the browser or operating system gesture activate the document.

For example, keyboard shortcuts like Ctrl+Tab, or Alt+Tab, Ctrl+R, etc and scrolling keys like PageUp/Down, and cursor Up/Down, should not be considered interaction with the page, and so shouldn't gesture activate the document.

So I think we need to restrict gesture activation via key events to key press events for printable characters.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1452536 - Don't gesture activate documents on non-printable key events.

We plan to only allow autoplay audible video in documents which have had user
interaction. So when a document has a mouse or key event dispatched to it, we
want to mark the document as gesture-activated. However we don't want
interactions which are probably not interaction with the web page to gesture
activate the document. So keypresses like scrolling with keys, or keyboard
shortcuts, should not gesture activate.

So add a blacklist of keys to EventStateManager.cpp of keys we think should not
gesture activate a document, and don't activate on key events for those keys.

Review commit: https://reviewboard.mozilla.org/r/235958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235958/

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1452536 - Test that key events for non-printable keys and interaction with editable elements don't unblock autoplay.

Review commit: https://reviewboard.mozilla.org/r/235960/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235960/

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c049210090e55b06297278ffbb346ae019f3a3cd

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Did you know actual problem which caused by your explanation?

Anyway, I don't understand why we need to mark the document active with keyup events. keydown/keypress/keyup events are fired on focused document *when* PresShell dispatches them into a DOM tree. So, if focus is changed while dispatching keydown/keypress events, following keyup event may not be intended by the user.

If we need to activate document at keyup rather than keydown, how about to check if last keydown event which is not marked as auto-repeated is fired in the document? It's possible to check easily with storing last keydown event fired ESM with a static variable.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #4)
> Did you know actual problem which caused by your explanation?

I tried to explain in the first patch's commit message. The current block-autoplay strategy of unblocking autoplay upon keyboard interaction with the document is not good, because if the user uses a keyboard shortcut, they'll unexpectedly unblock autoplay. For example, of you press CTRL+Tab to switch tab, you activate the document as you leave. If you scroll with ArrowUp/Down, you activate the document. This is annoying, and so we want to restrict gesture activation to keypresses that are not likely to be intended as interaction with the browser or OS.


> Anyway, I don't understand why we need to mark the document active with
> keyup events. keydown/keypress/keyup events are fired on focused document
> *when* PresShell dispatches them into a DOM tree. So, if focus is changed
> while dispatching keydown/keypress events, following keyup event may not be
> intended by the user.
> 
> If we need to activate document at keyup rather than keydown[...]

I took over this work from alwu after the Taipei layoffs. I don't know why alwu chose keyup over keydown. Activating on keydown seems fine to me. What about activating on keypress?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Chris Pearce (:cpearce) from comment #5)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #4)
> > Did you know actual problem which caused by your explanation?
> 
> I tried to explain in the first patch's commit message. The current
> block-autoplay strategy of unblocking autoplay upon keyboard interaction
> with the document is not good, because if the user uses a keyboard shortcut,
> they'll unexpectedly unblock autoplay. For example, of you press CTRL+Tab to
> switch tab, you activate the document as you leave. If you scroll with
> ArrowUp/Down, you activate the document. This is annoying, and so we want to
> restrict gesture activation to keypresses that are not likely to be intended
> as interaction with the browser or OS.

Okay, I got it.

> > Anyway, I don't understand why we need to mark the document active with
> > keyup events. keydown/keypress/keyup events are fired on focused document
> > *when* PresShell dispatches them into a DOM tree. So, if focus is changed
> > while dispatching keydown/keypress events, following keyup event may not be
> > intended by the user.
> > 
> > If we need to activate document at keyup rather than keydown[...]
> 
> I took over this work from alwu after the Taipei layoffs. I don't know why
> alwu chose keyup over keydown.

I see... It *might* be that if keydown activates the document, shortcut key or something which is handled before keyup event might be broken.

> Activating on keydown seems fine to me. What
> about activating on keypress?

I don't think that we should active document with keypress since if keydown changes focus, keypress may also be fired on unexpected document.

However, if you want to minimize risk of this change, I recommend to active the document at keyup but add check if preceding keydown event is fired on same document. Then, we can avoid the whitelist from our code.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> However, if you want to minimize risk of this change, I recommend to active
> the document at keyup but add check if preceding keydown event is fired on
> same document.

Ok. That makes sense. I will add a changeset to the push to check that the preceding keydown was fired on the same document. Thanks!

> Then, we can avoid the whitelist from our code.

I want to clarify this statement. Are you saying we can remove the blacklist? Irrespective of what key event we activate documents on, I think we still need a blacklist, as we don't want keys intended to be interaction with the browser to activate the document.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Chris Pearce (:cpearce) from comment #7)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> > Then, we can avoid the whitelist from our code.
> 
> I want to clarify this statement. Are you saying we can remove the
> blacklist? Irrespective of what key event we activate documents on, I think
> we still need a blacklist, as we don't want keys intended to be interaction
> with the browser to activate the document.

Hmm, I don't think that we should check specific key combinations for such purpose since key bindings depend on *current* implementation of both Firefox and OSes. Although, it's okay to ban a couple of key combinations with blacklist, but current your patch is too big and banning Shift + something which is used a lot, for example, typing upper case letters, scrolling with Shift + Space, etc.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #8)
> (In reply to Chris Pearce (:cpearce) from comment #7)
> > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> > > Then, we can avoid the whitelist from our code.
> > 
> > I want to clarify this statement. Are you saying we can remove the
> > blacklist? Irrespective of what key event we activate documents on, I think
> > we still need a blacklist, as we don't want keys intended to be interaction
> > with the browser to activate the document.
> 
> Hmm, I don't think that we should check specific key combinations for such
> purpose since key bindings depend on *current* implementation of both
> Firefox and OSes.

The Block Autoplay UX spec that has been developed by the team working on this calls for keypresses that are intended as interaction with the browser or OS to not unblock autoplay:
https://mozilla.invisionapp.com/share/N8F2K0I93#/screens/287176969

If we don't blacklist key combinations, then how do you propose we achieve the goal of preventing key combinations that are intended as interaction with the browser and OS from activating a document?

Is your complaint here that we're embedding Firefox behaviour inside Gecko? Would you prefer us to store the list of blacklisted key combinations inside some sort of bundled file which is part of the Firefox chrome code?

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> > I took over this work from alwu after the Taipei layoffs. I don't know why
> > alwu chose keyup over keydown.
> 
> I see... It *might* be that if keydown activates the document, shortcut key
> or something which is handled before keyup event might be broken.
> 
> > Activating on keydown seems fine to me. What
> > about activating on keypress?
> 
> I don't think that we should active document with keypress since if keydown
> changes focus, keypress may also be fired on unexpected document.
> 
> However, if you want to minimize risk of this change, I recommend to active
> the document at keyup but add check if preceding keydown event is fired on
> same document. Then, we can avoid the whitelist from our code.

Having looked into this bit further, by the time the keydown event handler runs in Chrome, the document is activated. So to be compatible with Chrome we need to have activated the document before the keydown handler runs, we can't wait until the keyup event.

For example, the following JS code:
(live: http://pearce.org.nz/video/autoplay-play-in-key-events.html )

  ["keypress", "keydown", "keyup"].forEach(
      function(e) {
        window.addEventListener(e, async function(event){
          let played;
          try {
            let x = await video.play();
            played = true;
          } catch (e) {
            played = false;
          }
          logEvent(event.type + " " + event.key + " played=" + played);
        }, false);
      });

When run in Chrome Unstable (which has autoplay blocked by default), if you press the 'p' key, this logs:

keydown p played=true
keypress p played=true
keyup p played=true

So Chrome is activating before their keydown handler runs.

Whereas because we're activating in the keyup event, we'll refuse to play in the keydown handler, so we'll log:

keydown p played=false
keypress p played=false
keyup p played=true


So I think we should change the behaviour here to activate on keydown, and not on keyup, otherwise we'll not be compatible with Chrome.

Masayuki: does that make sense to you? Thanks!

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

A similar test for mouse events shows that Chrome is also activating before dispatching mousedown, not mouseup like we do. So we should also change our gesture activation from mouseup to mousedown.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Chris Pearce (:cpearce) from comment #9)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #8)
> > (In reply to Chris Pearce (:cpearce) from comment #7)
> > > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> > > > Then, we can avoid the whitelist from our code.
> > > 
> > > I want to clarify this statement. Are you saying we can remove the
> > > blacklist? Irrespective of what key event we activate documents on, I think
> > > we still need a blacklist, as we don't want keys intended to be interaction
> > > with the browser to activate the document.
> > 
> > Hmm, I don't think that we should check specific key combinations for such
> > purpose since key bindings depend on *current* implementation of both
> > Firefox and OSes.
> 
> The Block Autoplay UX spec that has been developed by the team working on
> this calls for keypresses that are intended as interaction with the browser
> or OS to not unblock autoplay:
> https://mozilla.invisionapp.com/share/N8F2K0I93#/screens/287176969
> 
> If we don't blacklist key combinations, then how do you propose we achieve
> the goal of preventing key combinations that are intended as interaction
> with the browser and OS from activating a document?

Hmm, okay, for conforming to the document, it doesn't make sense to listen keyup events since we cannot distinguish if the key operation is for inputting some characters or not. Currently, we can know that only with keypress event.  If keypress event's all of ctrlKey, altKey and metaKey are false and charCode is not 0, the key operation is for inputting characters.

Note that following key events could occur, in this case, your patch doesn't work as expected:

1. Pressing Ctrl key:  keydown for Control.
2. Pressing R key:     keydown for R and keypress for R whose ctrlKey is true.
3. Releasing Ctrl key: keyup for Control.
4. Releasing R key:    keyup for R whose ctrlKey is false.

> Is your complaint here that we're embedding Firefox behaviour inside Gecko?
> Would you prefer us to store the list of blacklisted key combinations inside
> some sort of bundled file which is part of the Firefox chrome code?

Not so. I just want to avoid including blacklist/whitelist which need to maintain.

(In reply to Chris Pearce (:cpearce) from comment #10)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #6)
> > > I took over this work from alwu after the Taipei layoffs. I don't know why
> > > alwu chose keyup over keydown.
> > 
> > I see... It *might* be that if keydown activates the document, shortcut key
> > or something which is handled before keyup event might be broken.
> > 
> > > Activating on keydown seems fine to me. What
> > > about activating on keypress?
> > 
> > I don't think that we should active document with keypress since if keydown
> > changes focus, keypress may also be fired on unexpected document.
> > 
> > However, if you want to minimize risk of this change, I recommend to active
> > the document at keyup but add check if preceding keydown event is fired on
> > same document. Then, we can avoid the whitelist from our code.
> 
> Having looked into this bit further, by the time the keydown event handler
> runs in Chrome, the document is activated. So to be compatible with Chrome
> we need to have activated the document before the keydown handler runs, we
> can't wait until the keyup event.
>
> ...
> 
> So I think we should change the behaviour here to activate on keydown, and
> not on keyup, otherwise we'll not be compatible with Chrome.
> 
> Masayuki: does that make sense to you? Thanks!

Yeah, it must be better to behave exactly same as Chrome. On the other hand, we don't have any flag whether keydown event is followed by keypress event which will cause inputting text. However, perhaps, ignoring only a few special cases, e.g., cases to input a character with Ctrl + foo, we can test as following simple code:

if (!keyEvent ||
    !keyEvent->PseudoCharCode() ||
    (keyEvent->IsControl() && !keyEvent->IsAltGraph()) ||
    (keyEvent->IsAlt() && !keyEvent->IsAltGraph()) ||
    keyEvent->IsMeta() ||
    keyEvent->IsOS()) {
  return;
}
// do something.

If you need to allow to keydown event which will cause compositionstart, you need to allow |key->mKayNameIndex == KEY_NAME_INDEX_Process| even if any other members are true. "Process" key means that the key combination is consumed by IME.

(In reply to Chris Pearce (:cpearce) from comment #11)
> A similar test for mouse events shows that Chrome is also activating before
> dispatching mousedown, not mouseup like we do. So we should also change our
> gesture activation from mouseup to mousedown.

Yeah, we should change it in another bug.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #12)
> we can test
> as following simple code:
> 
> if (!keyEvent ||
>     !keyEvent->PseudoCharCode() ||
>     (keyEvent->IsControl() && !keyEvent->IsAltGraph()) ||
>     (keyEvent->IsAlt() && !keyEvent->IsAltGraph()) ||
>     keyEvent->IsMeta() ||
>     keyEvent->IsOS()) {
>   return;
> }
> // do something.

This seems to give the behaviour we want, and is much simpler! Thank you!


> If you need to allow to keydown event which will cause compositionstart, you
> need to allow |key->mKayNameIndex == KEY_NAME_INDEX_Process| even if any
> other members are true. "Process" key means that the key combination is
> consumed by IME.

Just checking my understanding, we need to make the above test:

  WidgetKeyboardEvent* keyEvent = aEvent->AsKeyboardEvent();
  if (keyEvent && (!keyEvent->PseudoCharCode() ||
                   (keyEvent->IsControl() && !keyEvent->IsAltGraph()) ||
                   (keyEvent->IsAlt() && !keyEvent->IsAltGraph()) ||
                   keyEvent->IsMeta() || keyEvent->IsOS() ||
                   keyEvent->mKeyNameIndex == KEY_NAME_INDEX_Process)) {
    return;
  }



> (In reply to Chris Pearce (:cpearce) from comment #11)
> > A similar test for mouse events shows that Chrome is also activating before
> > dispatching mousedown, not mouseup like we do. So we should also change our
> > gesture activation from mouseup to mousedown.
> 
> Yeah, we should change it in another bug.

OK, thanks!

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Chris Pearce (:cpearce) from comment #13)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #12)
> > If you need to allow to keydown event which will cause compositionstart, you
> > need to allow |key->mKayNameIndex == KEY_NAME_INDEX_Process| even if any
> > other members are true. "Process" key means that the key combination is
> > consumed by IME.
> 
> Just checking my understanding, we need to make the above test:
> 
>   WidgetKeyboardEvent* keyEvent = aEvent->AsKeyboardEvent();
>   if (keyEvent && (!keyEvent->PseudoCharCode() ||
>                    (keyEvent->IsControl() && !keyEvent->IsAltGraph()) ||
>                    (keyEvent->IsAlt() && !keyEvent->IsAltGraph()) ||
>                    keyEvent->IsMeta() || keyEvent->IsOS() ||
>                    keyEvent->mKeyNameIndex == KEY_NAME_INDEX_Process)) {
>     return;
>   }

Ah, no. If we need to allow to activate document with first key press even though it's consumed by IME, it should be:

if (!keyEvent ||
    (keyEvent->mKeyNameIndex != KEY_NAME_INDEX_Process &&
     (!keyEvent->PseudoCharChode() ||
      ...))) {
  return; // not activate.
}

I don't know whether Google Chrome allows such keydown events too since such keydown event is dispatched only in <input type="text">, <textarea> or <foo contenteditable>. It might be better to active document with compositionstart event instead if web apps won't try to start to play video at preceding keydown event of compositionstart.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #12)
> (In reply to Chris Pearce (:cpearce) from comment #11)
> > A similar test for mouse events shows that Chrome is also activating before
> > dispatching mousedown, not mouseup like we do. So we should also change our
> > gesture activation from mouseup to mousedown.
> 
> Yeah, we should change it in another bug.

Will change to gesture activating on key/mouse down in bug 1456037.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #14)
> (In reply to Chris Pearce (:cpearce) from comment #13)
> > (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #12)
> > > If you need to allow to keydown event which will cause compositionstart, you
> > > need to allow |key->mKayNameIndex == KEY_NAME_INDEX_Process| even if any
> > > other members are true. "Process" key means that the key combination is
> > > consumed by IME.
> > 
> > Just checking my understanding, we need to make the above test:
> > 
> >   WidgetKeyboardEvent* keyEvent = aEvent->AsKeyboardEvent();
> >   if (keyEvent && (!keyEvent->PseudoCharCode() ||
> >                    (keyEvent->IsControl() && !keyEvent->IsAltGraph()) ||
> >                    (keyEvent->IsAlt() && !keyEvent->IsAltGraph()) ||
> >                    keyEvent->IsMeta() || keyEvent->IsOS() ||
> >                    keyEvent->mKeyNameIndex == KEY_NAME_INDEX_Process)) {
> >     return;
> >   }
> 
> Ah, no. If we need to allow to activate document with first key press even
> though it's consumed by IME, it should be:
> 
> if (!keyEvent ||
>     (keyEvent->mKeyNameIndex != KEY_NAME_INDEX_Process &&
>      (!keyEvent->PseudoCharChode() ||
>       ...))) {
>   return; // not activate.
> }
> 
> I don't know whether Google Chrome allows such keydown events too since such
> keydown event is dispatched only in <input type="text">, <textarea> or <foo
> contenteditable>. It might be better to active document with
> compositionstart event instead if web apps won't try to start to play video
> at preceding keydown event of compositionstart.

Based discussions elsewhere, we want to not gesture activate for input events directed at editable elements. This would prevent typing using IME or in Latin/English character sets in editable inputs from gesture activating documents. This would also mean we don't need to check for KEY_NAME_INDEX_Process, as it should only be dispatched to editable target content, and we will be refusing to gesture activate on these anyway.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8967271 [details]
Bug 1452536 - Don't gesture activate documents on non-printable key events.

I think that you're updating the patches for this bug.

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Bug 1452536 - Don't gesture activate for events targetting editable elements.

We don't want to gesture activate for key and mouse/pointer events sent to
editable elements, as that would mean user interaction intended as text input
could start media playback, which would not likely be the user's intention.

Review commit: https://reviewboard.mozilla.org/r/240208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240208/

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8967271 [details]
Bug 1452536 - Don't gesture activate documents on non-printable key events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235958/diff/1-2/

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8967272 [details]
Bug 1452536 - Test that key events for non-printable keys and interaction with editable elements don't unblock autoplay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235960/diff/1-2/

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8967271 [details]
Bug 1452536 - Don't gesture activate documents on non-printable key events.

https://reviewboard.mozilla.org/r/235958/#review246030

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8971457 [details]
Bug 1452536 - Don't gesture activate for events targetting editable elements.

https://reviewboard.mozilla.org/r/240208/#review246038

Giving r+ since Japan starts holiday week and anyway the pointed issues are trivial.

::: dom/events/EventStateManager.cpp:911
(Diff revision 1)
> +IsTextInput(nsIContent* aContent)
> +{
> +  MOZ_ASSERT(aContent);
> +  nsCOMPtr<nsITextControlElement> textCtrl = do_QueryInterface(aContent);
> +  return textCtrl != nullptr;

I think that this returns true for any types of <input> element. Perhaps, you want to do:

if (!aContent->IsElement()) {
  return false;
}
return !!aContent->AsElement()->GetTextEditorInternal();

Although, this could return true for readonly <input>/<textarea> elements. If you want to ban readonly editors, then, 

TextEditor* textEditor =
  aContent->AsElement()->GetTextEditorInternal();
return textEditor && !textEditor()->IsReadOnly();

::: dom/events/EventStateManager.cpp:945
(Diff revision 1)
>  
> +  // Don't activate if the target content of the event is contentEditable or
> +  // is inside an editable document, or is a text input control. Activating
> +  // due to typing/clicking on a text input would be surprising user experience.
> +  if (aTargetContent->IsEditable() ||
> +      doc->IsEditable() ||

Must not be necessary to check this since nsINode::IsEditable() checks it:
https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/dom/base/nsINode.cpp#208,215,218

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8967272 [details]
Bug 1452536 - Test that key events for non-printable keys and interaction with editable elements don't unblock autoplay.

https://reviewboard.mozilla.org/r/235960/#review246040

::: dom/media/test/file_autoplay_policy_key_blacklist.html:62
(Diff revision 2)
> +          "Shift",
> +          "Escape",
> +        ];
> +
> +        let modifiedKeys = [
> +          { key: "C", modifiers: { ctrlKey: true } },

nit: "c" since shiftKey is not true.

::: dom/media/test/file_autoplay_policy_key_blacklist.html:64
(Diff revision 2)
> +        ];
> +
> +        let modifiedKeys = [
> +          { key: "C", modifiers: { ctrlKey: true } },
> +          { key: "V", modifiers: { ctrlKey: true, shiftKey: true } },
> +          { key: "A", modifiers: { altKey: true } },

nit: "a" since shiftKey is not true.

::: dom/media/test/file_autoplay_policy_key_blacklist.html:74
(Diff revision 2)
> +          synthesizeComposition({ type: "compositioncommit", data: "\u30E9\u30FC\u30E1\u30F3", key: { key: "KEY_Enter" } });
> +          played = await element.play().then(() => true, () => false);
> +          ok(!played, "Entering text to " + name + " via IME should not activate document and should not unblock play");

I think that you should check synthesizeCompositionChange() before "compositioncommit".

  synthesizeCompositionChange({
    composition: {
      string: "\u30E9\u30FC\u30E1\u30F3",
      clauses: [
        { length: 4, attr: COMPOSITION_ATTR_RAW_CLAUSE }
      ]
    },
    caret: { start: 4, length: 0 }
  });

Then, you tested usual IME sequence (i.e., keydown -> compositionstart -> compositionupdate (-> text) -> keyup -> keydown (-> text) -> compositionend -> keyup

::: dom/media/test/file_autoplay_policy_key_blacklist.html:78
(Diff revision 2)
> +          for (let ch of "some ascii text for " + name) {
> +            synthesizeKey(ch);
> +          }
> +          played = await element.play().then(() => true, () => false);
> +          ok(!played, "Entering ASCII text into " + name + " should not activate document and should not unblock play");

Why don't you check if usual character input in editor won't cause allowing autoplay?

E.g., sendString("a");

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8967271 [details]
Bug 1452536 - Don't gesture activate documents on non-printable key events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235958/diff/2-3/

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8971457 [details]
Bug 1452536 - Don't gesture activate for events targetting editable elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240208/diff/1-2/

Comment 26

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8967272 [details]
Bug 1452536 - Test that key events for non-printable keys and interaction with editable elements don't unblock autoplay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235960/diff/2-3/

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8971457 [details]
Bug 1452536 - Don't gesture activate for events targetting editable elements.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240208/diff/2-3/

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8967272 [details]
Bug 1452536 - Test that key events for non-printable keys and interaction with editable elements don't unblock autoplay.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235960/diff/3-4/

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=53dcc095fe03c56b7aabb1cd7c7824e6e46cc650

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks for the review and your thoughtful comments Masayuki!

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/006f976d7963
Don't gesture activate documents on non-printable key events. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/c454505cc025
Don't gesture activate for events targetting editable elements. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/1259c5bc20a7
Test that key events for non-printable keys and interaction with editable elements don't unblock autoplay. r=masayuki

Comment 32

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/006f976d7963
https://hg.mozilla.org/mozilla-central/rev/c454505cc025
https://hg.mozilla.org/mozilla-central/rev/1259c5bc20a7

Comment 33

[Cosmin Sabou [:CosminS]]

As per this comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1458166#c4

Backed out for causing crashes bug 1458166.

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=176224212&repo=autoland&lineNumber=6356
Backout link: https://hg.mozilla.org/mozilla-central/rev/a91ca6e5ca820b53fad9bb98b256934353f5217a

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

As per bug 1458166 comment 14 this is not the cause of the failure reported in bug 1458166. So I'm going to re-land this.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46471bf78ee8
Don't gesture activate documents on non-printable key events. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/2f2a7a27a90f
Don't gesture activate for events targetting editable elements. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/9c2deb188bdf
Test that key events for non-printable keys and interaction with editable elements don't unblock autoplay. r=masayuki

Comment 36

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/46471bf78ee8
https://hg.mozilla.org/mozilla-central/rev/2f2a7a27a90f
https://hg.mozilla.org/mozilla-central/rev/9c2deb188bdf

Comment 37

[Nickolay_Ponomarev]

Based discussions elsewhere, we want to not gesture activate for input events directed at editable elements.

Chris, could you point me to those discussions? This is rather counter-intuitive, as the act of typing into an input seems to qualify as "interacting with the web page", and the current rules motivate the developer to avoid native inputs in order to get the page gesture-activated, see the use-case described here: https://stackoverflow.com/questions/57504122/browser-denying-javascript-play

Comment 38

[Alastor Wu [:alwu][PTO until 12/16]]

I tested the examlple in comment37 and another one reported in bug1572939, I found that Chrome and Safari didn't block autoplay when haiving user input on editable content. I can imagine some scenario like simulating a typing machine by playing a short sound while user is typing [1].

It seems that we don't really need to restrict that user inputs, which can activate document, should only occur on non-editable content. Even if they occur on non-editable content, it might still have a chance to annoy user, it's totally depending on websites' design.

I will go to remove this constraint in bug1572939.

[1] https://alastor0325.github.io/htmltests/autoplay_tests/playSoundOnEditableInput.html