feat: mouse and keyboard indicators #3526

JoelEinbinder · 2020-08-19T07:04:55Z

Adds an overlay that shows the current mouse position and typed text. I have a test but its waiting on some fixture refactorings I'm working on.

The overlays are added into the dom. While their guts are hidden with a shadow root, they still can be detected by JavaScript on the page. In practice I don't expect this to be an issue.

I'd like future patches to animate the mouse cursor and show "to-be-typed" text for a smoother experience.

aslushnikov

lgtm!

Let's make sure everyone is happy with API names (I am)

Also, this'll close #1374

src/input.ts

dgozman · 2020-08-22T03:44:51Z

docs/api.md

@@ -4135,6 +4135,8 @@ const { chromium } = require('playwright');  // Or 'firefox' or 'webkit'.
 - `options` <[Object]>
  - `wsEndpoint` <[string]> A browser websocket endpoint to connect to. **required**
  - `slowMo` <[number]> Slows down Playwright operations by the specified amount of milliseconds. Useful so that you can see what is going on. Defaults to 0.
+  - `showMouseIndicator` <[boolean]> Show Playwright's mouse cursor.


Did you consider doing this under PWDEBUG instead? How do you see these options being used?

Let's land this behind env instead.

JoelEinbinder · 2020-08-28T09:46:35Z

@dgozman and I talked offline about this.

PWDEBUG doesn't replace javascript apis.
It would be nice to have a simple option debug: true which does headless: false, slowMo: 250, showKeyboardIndicator: true, etc.
Does the single debug option replace the individual options? No. The individual options make our documentation story much nicer, and also lets us add new features to debug: true without it being a breaking change.
We proposed having debug: true as well as debug: {mouse: true, sourcemaps: true}. This avoids cluttering the launch api with many options.

By myself I had a change of heart about debug: {mouse: true}. It gets tricky because we want debug to set headless: false, but headless is not always a debugging feature. It also is already in the launch api, so we would have to mirror it and document what happens when you specify both debug: {headless: true} and headless: false.

Instead I was inspired by typescript's "strict": true, which has related options "strictBindCallApply", "strictNullChecks", etc. I've renamed showMouseIndicator into debugMouse and debugKeyboard. Then when we add debug, it will be clear that it turns on all options prefixed with debug. And when im launching my browser, I can safely ignore all options that being with debug.

pavelfeldman · 2020-08-29T18:43:03Z

src/server/input.ts

+      function createKeyboardIndicator() {
+        const element = document.createElement('playwright-keyboard-indicator');
+        const shadow = element.attachShadow({mode: 'closed'});
+        shadow.innerHTML = `


our injected script already has html library for debug overlays, let's align those implementations.

pavelfeldman · 2020-08-29T18:44:41Z

src/server/input.ts

@@ -50,14 +51,98 @@ export class Keyboard {
    this._page = page;
  }

+  async _updateKeyboardIndicator(description?: KeyDescription) {


Let's get some code modularity going. This is a debugging code, it needs to be strictly separate from the code functionality. See what Dmitry did for tracing - make sure your auxiliary feature is using apis / dependency injection where necessary.

pavelfeldman

Requesting changes for modularity, we can't pollute core.

JoelEinbinder · 2020-11-11T19:58:30Z

Requesting changes for modularity, we can't pollute core.

This has been updated. ptal.

pavelfeldman · 2020-12-11T16:53:41Z

Actually, still needs work. It is disconnected from the existing debugging features and cli, we should get some consistency going there.

Aaron-Pool · 2020-12-30T15:17:45Z

Just chiming in to say that I would love to see this feature get integrated 👍

JoelEinbinder · 2021-01-15T06:08:03Z

Merged all of the options into showUserInput. Refactored to use a context listener instead of wrapping the touch events. @dgozman ptal thanks!

dgozman · 2021-01-19T19:31:44Z

docs/src/api/class-browsertype.md

@@ -157,6 +157,11 @@ headless`] option will be set `false`.

 Slows down Playwright operations by the specified amount of milliseconds. Useful so that you can see what is going on.

+### option: BrowserType.launch.showUiserInput


typo in the option name

I don't like that we lost linting for these

docs/src/api/class-browsertype.md

src/server/browserContext.ts

dgozman · 2021-01-19T19:35:55Z

src/server/browserContext.ts

 }

+export type InputEvent = {


I feel like instrumentation has grown large enough to justify a separate file.

Ok, I made a contextListeners file.

test/indicators.spec.ts

src/debug/showUserInput.ts

dgozman · 2021-01-19T19:43:37Z

src/debug/showUserInput.ts

+          shadow.appendChild(div);
+        }
+        if (timeout)
+          clearInterval(timeout);


This uses setTimeout + clearInterval. Let's decide whether it's a timeout or an interval 😄

https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/frame/window_or_worker_global_scope.cc;l=213?q=::clearInterval&ss=chromium

dgozman · 2021-01-19T23:13:38Z

src/debug/showUserInput.ts

+        timeout = setTimeout(() => {
+          div.remove();
+          timeout = null;
+        }, 200);


Played with this, 200ms is too short for me. WDYT about reusing the slowMo value is present? Same for tap.

Sure. (slowMo || 150) + 50

For tap I think always 500ms is fine. We treat tap as an atomic action and dont slowMo inbetween touchstart and touchend.

dgozman · 2021-01-19T23:21:43Z

src/debug/showUserInput.ts

+    text = '';
+  } else if (!modifiers.size && key.trim().length === 1) {
+    const currentText = (textForPage.get(page) || '') + key;
+    textForPage.set(page, currentText);


I think we should reset this somewhere, otherwise multiple texts will coalesce. Perhaps on other forms of input? Navigations? Timeout?

More generally, I think the text should increase while the tooltip is shown, and be reset once the tooltip is hidden. wdyt?

await page.fill('username', 'John Doe'); await page.check('remember be'); await page.fill('password', 'foo');

Yeah I think theres a lot we can do by taking into account the specific method that called into the keyboard. I was looking at a karaoke style https://youtu.be/OU3699R53rs?t=23 for page.type. And we can remove the indicator on page.fill if its filling uninterrupted into a textarea. It's just redundant there.

I'd rather tackle these in a follow up patch because they involve a lot of api changes and bikeshedding.

docs/src/api/class-browsertype.md

src/debug/showUserInput.ts

src/server/contextListeners.ts

dgozman · 2021-02-02T18:30:20Z

docs/src/api/class-browsertype.md

@@ -197,6 +197,11 @@ Specify environment variables that will be visible to the browser. Defaults to `

 Slows down Playwright operations by the specified amount of milliseconds. Useful so that you can see what is going on.

+### option: BrowserType.launch.showUserInput


I am hesitant with a public option. We are growing a list of options nobody will ever know about. Let's think of "one to rule them all" approach?

dgozman · 2021-02-02T18:31:21Z

docs/src/api/class-browsertype.md

@@ -290,6 +295,14 @@ Specify environment variables that will be visible to the browser. Defaults to `
 Slows down Playwright operations by the specified amount of milliseconds. Useful so that you can see what is going on.
 Defaults to 0.

+### option: BrowserType.launchPersistentContext.showUserInput


Will I see these indicators on the screenshots? Does that mean all my screenshot tests will just fail? Let's hide them when screenshotting?

@dgozman as a counter argument to this, what if I want indicators in my screenshots? To make it more obvious what the screenshot is demonstrating?

yeah, I would prefer to have them as well

dgozman · 2021-02-06T00:39:38Z

src/server/supplements/showUserInput.ts

+export class ShowUserInput implements ContextListener {
+  async onContextCreated(context: BrowserContext) {
+    context._inputListeners.add(async (page, event) => {
+      if (!page.context()._browser.options.showUserInput)


Let's just not add the input listener in the first place.

pavelfeldman · 2021-02-06T01:18:07Z

src/protocol/channels.ts

@@ -332,7 +334,7 @@ export type BrowserTypeLaunchPersistentContextOptions = {
  downloadsPath?: string,
  chromiumSandbox?: boolean,
  slowMo?: number,
-  noDefaultViewport?: boolean,
+  showUserInput?: boolean,


what happened to the noDefaultViewport?

I would not want to add another API like headless: false that requires modifications of source for every debugging session.

We are missing some doc checks these days.

pavelfeldman · 2021-02-06T01:18:32Z

src/server/browserContext.ts

@@ -322,7 +308,7 @@ export abstract class BrowserContext extends EventEmitter {

      // Bookkeeping.
      for (const listener of this._browser.options.contextListeners)
-        await listener.onContextDidDestroy(this);
+        await listener.onContextDidDestroy?.(this);


Do we do this ?. already?

As of this patch yes. See previous comments about tsconfig.json

pavelfeldman · 2021-02-06T01:19:27Z

src/server/input.ts

@@ -57,7 +57,15 @@ export class Keyboard {
    if (kModifiers.includes(description.key as types.KeyboardModifier))
      this._pressedModifiers.add(description.key as types.KeyboardModifier);
    const text = description.text;
-    await this._raw.keydown(this._pressedModifiers, description.code, description.keyCode, description.keyCodeWithoutLocation, description.key, description.location, autoRepeat, text);
+    await Promise.all([
+      this._page.context().notifyInputListeners(this._page, {


This now conflicts with both pw:api and trace viewer. I wish we could have it all aligned.

pavelfeldman · 2021-02-16T18:25:02Z

Closing this in favor of the new PWDEBUG=1 behavior that has already landed.

aslushnikov approved these changes Aug 21, 2020

View reviewed changes

src/input.ts Outdated Show resolved Hide resolved

This was referenced Aug 21, 2020

[Feature] Mouse helper (tracking mouse position, clicks, drag & drops, selections) #1374

Open

[Feature] Simulate mouse movements so you can visually see what is being clicked during a test run #2718

Closed

JoelEinbinder force-pushed the mouse_keyboard_indicator branch from 06e5d25 to 4f5f533 Compare August 21, 2020 17:32

dgozman reviewed Aug 22, 2020

View reviewed changes

JoelEinbinder force-pushed the mouse_keyboard_indicator branch 2 times, most recently from cb88ddb to b4815fa Compare August 28, 2020 09:35

pavelfeldman reviewed Aug 29, 2020

View reviewed changes

pavelfeldman requested changes Aug 29, 2020

View reviewed changes

JoelEinbinder force-pushed the mouse_keyboard_indicator branch 2 times, most recently from e16de46 to c937ef0 Compare November 10, 2020 08:00

JoelEinbinder force-pushed the mouse_keyboard_indicator branch from c937ef0 to dc0305d Compare December 2, 2020 15:58

JoelEinbinder force-pushed the mouse_keyboard_indicator branch from dc0305d to aa97293 Compare January 14, 2021 00:46

dgozman reviewed Jan 19, 2021

View reviewed changes

JoelEinbinder force-pushed the mouse_keyboard_indicator branch from c401499 to 6d87cf7 Compare January 21, 2021 07:37

aslushnikov mentioned this pull request Jan 27, 2021

[Question] Way to do mouse.move(selector, steps) type functionality #5160

Closed

JoelEinbinder force-pushed the mouse_keyboard_indicator branch from 6d87cf7 to c28b9c1 Compare January 28, 2021 16:41

dgozman reviewed Feb 2, 2021

View reviewed changes

JoelEinbinder force-pushed the mouse_keyboard_indicator branch from c28b9c1 to dd3556c Compare February 4, 2021 17:04

dgozman reviewed Feb 6, 2021

View reviewed changes

JoelEinbinder force-pushed the mouse_keyboard_indicator branch from dd3556c to 58a5f6b Compare February 6, 2021 00:52

pavelfeldman reviewed Feb 6, 2021

View reviewed changes

feat: mouse and keyboard indicators

05ba4a4

JoelEinbinder force-pushed the mouse_keyboard_indicator branch from 58a5f6b to 05ba4a4 Compare February 8, 2021 17:20

pavelfeldman closed this Feb 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: mouse and keyboard indicators #3526

feat: mouse and keyboard indicators #3526

JoelEinbinder commented Aug 19, 2020

aslushnikov left a comment

dgozman Aug 22, 2020

pavelfeldman Aug 29, 2020

JoelEinbinder commented Aug 28, 2020

pavelfeldman Aug 29, 2020

pavelfeldman Aug 29, 2020

pavelfeldman left a comment

JoelEinbinder commented Nov 11, 2020

pavelfeldman commented Dec 11, 2020

Aaron-Pool commented Dec 30, 2020

JoelEinbinder commented Jan 15, 2021

dgozman Jan 19, 2021

JoelEinbinder Jan 21, 2021

dgozman Jan 19, 2021

JoelEinbinder Jan 28, 2021

dgozman Jan 19, 2021

JoelEinbinder Jan 21, 2021

dgozman Jan 19, 2021

JoelEinbinder Jan 21, 2021

JoelEinbinder Jan 21, 2021

dgozman Jan 19, 2021

JoelEinbinder Jan 21, 2021

dgozman Feb 2, 2021

dgozman Feb 2, 2021

Aaron-Pool Feb 4, 2021

LanderBeeuwsaert Feb 4, 2021

dgozman Feb 6, 2021

pavelfeldman Feb 6, 2021

JoelEinbinder Feb 8, 2021

pavelfeldman Feb 6, 2021

JoelEinbinder Feb 8, 2021

pavelfeldman Feb 6, 2021

pavelfeldman commented Feb 16, 2021

		@@ -157,6 +157,11 @@ headless`] option will be set `false`.

		Slows down Playwright operations by the specified amount of milliseconds. Useful so that you can see what is going on.

		### option: BrowserType.launch.showUiserInput

		@@ -197,6 +197,11 @@ Specify environment variables that will be visible to the browser. Defaults to `

		Slows down Playwright operations by the specified amount of milliseconds. Useful so that you can see what is going on.

		### option: BrowserType.launch.showUserInput

feat: mouse and keyboard indicators #3526

feat: mouse and keyboard indicators #3526

Conversation

JoelEinbinder commented Aug 19, 2020

aslushnikov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelEinbinder commented Aug 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelfeldman left a comment

Choose a reason for hiding this comment

JoelEinbinder commented Nov 11, 2020

pavelfeldman commented Dec 11, 2020

Aaron-Pool commented Dec 30, 2020

JoelEinbinder commented Jan 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavelfeldman commented Feb 16, 2021