Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'keyup' and 'keypress' to 'activation triggering input event' #6818

Closed
wants to merge 1 commit into from

Conversation

marcoscaceres
Copy link
Member

@marcoscaceres marcoscaceres commented Jul 1, 2021

Please see w3c/pointerlock#70 over on Pointerlock for details.

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
  • Implementation bugs are filed:
    • Chrome: …
    • Firefox: …
    • Safari: …

(See WHATWG Working Mode: Changes for more details.)

cc @mustaqahmed


/interaction.html ( diff )

@domenic
Copy link
Member

domenic commented Jul 1, 2021

This at the very least needs tests. Can you restore the PR template?

@mustaqahmed
Copy link
Contributor

This at the very least needs tests.

WPTs with user activation are tricky because they have to rely on a gated API like popup or fullscreen. I know fullscreen has interop problems with activation consumption, so popup seems to be the only good choice here.

(Wish we had some movement in #4008, at least for sake of better WPTs.)

@marcoscaceres
Copy link
Member Author

I don't know all the details here, but we can easily send keys via web driver (and detect the events).

Quick search finds the following, which maybe we can use?:
https://github.com/web-platform-tests/wpt/blob/master/editing/include/editor-test-utils.js
(cc @masayuki-nakano, who put that together)

@mustaqahmed, if you can describe what you need, we can try to cook something up.

If we hit issues, that's actually good... as it's incentive to fix things in Web Driver or WPT!

@mustaqahmed
Copy link
Contributor

Web-driver key dispatch works (i.e. activates the page correctly), I meant asserting this activation state is the main hurdle in WPTs. You have to rely on a user-activation gated API like popup, or rely on a tentative API. See the WPTs in html/user-activation), I think basic.html is only good example of a non-tentative test for us here.

@domenic domenic added needs tests Moving the issue forward requires someone to write tests normative change topic: user activation labels Jul 14, 2021
@mustaqahmed
Copy link
Contributor

@marcoscaceres Please let me know if writing a WPT like this one makes sense to you.

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Sep 7, 2021

@mustaqahmed yes, but that could be written in a way that is a bit more linear using promises/async/await. Like, can get a little bit more fancy with them (easier to read, debug, and maintain):

promise_test(async t => {
  const clickPromise = new Promise(resolve => {
    window.addEventListener("click",resolve);
  });
  test_driver.click(document.body);
  await clickPromise;
  const win = window.open();
  assert_true(Boolean(win));
}, "Popup with click");

For your case with the keys, I suggest doing something similar to the follow (I've not tested this code, just pseudo code):

function keyPress(element, keys) {
  test_driver.send_keys(element, keys);
  const pressPromise = new Promise((resolve) => {
    element.addEventListener("keypress", resolve, { once: true });
  });
  return pressPromise; // Returns the Event
}

promise_test(async t => {
  const input = document.createElement("input");
  input.type = "text";
  document.body.appendChild(input);
  input.focus();

  await keyPress(input, "a");
  await keyPress(input, "b");
  await keyPress(input, "c");

  assert_equals(input.value, "abc");

  const event = keyPress(input, "d");
  // assert_true() something to do with the event?
}, "Keypress events must be sent blah blah make sure this is really descriptive and matches the spec");

@marcoscaceres
Copy link
Member Author

marcoscaceres commented Sep 7, 2021

Just as an aside, .send_keys() let's you send multiple keys, so you could get a bit fancier.
https://web-platform-tests.org/writing-tests/testdriver.html#send-keys

Anyway, feel feel to ping me over on the WTP side once you have a test up and I can help review it - or we can bounce ideas there how to best test things.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 14, 2021
These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 15, 2021
These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
@mustaqahmed
Copy link
Contributor

Testing with popup seems tricky because popup blocker is disabled in WPT setup! web-platform-tests/wpt#30777

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 15, 2021
These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
@marcoscaceres
Copy link
Member Author

I don't recall having issues with popups, to be honest? I think that as long as you "bless()" them first, they should work (at least on desktop)... on mobile, the tests will probably fail spectacularly, but that's probably ok.

Dogit grep "window.open" in the wtp repo to see examples of tests using popups. I'm seeing about 600 files making use of them.

@mustaqahmed
Copy link
Contributor

All existing tests I saw want popups to open. For our case, the test needs to block a popup without user activation, so that we can determine the availability of user activation.

Fortunately, all of Chrome, Firefox and Safari seem to consume user activation on fullscreen, so I am working now on a fullscreen-based tests instead.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 22, 2021
These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 27, 2021
These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 28, 2021
These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 30, 2021
These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
@mustaqahmed
Copy link
Contributor

Just landed a WPT for keypress/keyup event activation triggering.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 30, 2021
These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3159551
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Cr-Commit-Position: refs/heads/main@{#926812}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Sep 30, 2021
These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3159551
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Cr-Commit-Position: refs/heads/main@{#926812}
pull bot pushed a commit to jamlee-t/chromium that referenced this pull request Sep 30, 2021
These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3159551
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Cr-Commit-Position: refs/heads/main@{#926812}
@annevk
Copy link
Member

annevk commented Oct 1, 2021

As per #3849 (comment) I think we need to mention the Esc key here somehow. And perhaps also test it?

@mustaqahmed
Copy link
Contributor

@annevk I agree, thanks. The challenge with Esc is that it is not an activation trigger, so perhaps it would be wiser/easier if we fix this along with the keys that are triggers (this link was originally posted in #3849).

May I suggest doing that bigger change separately? It would also unblock @marcoscaceres's PointerLock PR mentioned above.

@domenic
Copy link
Member

domenic commented Oct 1, 2021

I think it would be unfortunate for the spec to start mandating that Esc keyup and Esc keypress were activation-triggering, as they would if we accepted this PR as-is. So I think the issue needs to be taken care of holistically.

@mustaqahmed
Copy link
Contributor

That's fair, thanks. Let's discuss about an incremental-yet-acceptable path in the issue. I will dump some ideas there today.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Oct 5, 2021
…ress and keyup events., a=testonly

Automatic update from web-platform-tests
Added WPTs for user-activation from keypress and keyup events.

These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3159551
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Cr-Commit-Position: refs/heads/main@{#926812}

--

wpt-commits: aa4a6c240131c34425819ecd981423332f9028f0
wpt-pr: 30777
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Oct 6, 2021
…ress and keyup events., a=testonly

Automatic update from web-platform-tests
Added WPTs for user-activation from keypress and keyup events.

These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3159551
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Cr-Commit-Position: refs/heads/main@{#926812}

--

wpt-commits: aa4a6c240131c34425819ecd981423332f9028f0
wpt-pr: 30777
Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this pull request Nov 18, 2021
These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3159551
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Cr-Commit-Position: refs/heads/main@{#926812}
@mustaqahmed
Copy link
Contributor

We can close this PR now: we recently landed a clearer definition of "activation triggering input events" (through #7248) which covers the changes proposed here.

@annevk annevk closed this Nov 18, 2021
@marcoscaceres marcoscaceres deleted the input_exports branch December 8, 2021 06:32
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
These tests are for the following HTML PR:
whatwg/html#6818

Also renamed the existing click event test for consistency.

Change-Id: If77750f4159828b68bd91a4b48f46606421b7df6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3159551
Reviewed-by: Robert Flack <flackr@chromium.org>
Commit-Queue: Mustaq Ahmed <mustaq@chromium.org>
Cr-Commit-Position: refs/heads/main@{#926812}
NOKEYCHECK=True
GitOrigin-RevId: d0d9a5b51f501f0f901dfe3a097b028b06c4b907
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Moving the issue forward requires someone to write tests normative change topic: user activation
Development

Successfully merging this pull request may close these issues.

4 participants