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

feat: use 'fill' for inputs #727

Merged
merged 16 commits into from
Jul 11, 2020
Merged

feat: use 'fill' for inputs #727

merged 16 commits into from
Jul 11, 2020

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Jul 6, 2020

Resolves #702

Changes

To better handle input typing edge cases, the create command now generates fill actions for typing that happens in any input or textarea with a few exceptions.

Details

  • page.type is no longer generated
  • page.press actions are generated in a different way. Modifier keys are no longer tracked for now.

Implementing this as proposed in the issue didn't work. Checking whether the next event is the same target as the previous doesn't handle a bunch of cases where various other events that should be ignored are firing between, such as scrolling and clicking. I tried the input event instead, but it was difficult to figure out when the input changing was "done". Then I tried the "change" event, which is almost perfect because it lets the browser decide when the input change is committed. The only problem with "change" is that it always fires after the Enter, Tab, or click that caused it, which is the reverse of what we want to generate. To resolve this, I use "change" to figure out when editing an input's value is done, but then I use the most recent "input" event for that target as the actual event. In my tests this seems to work well.

Special Cases

I had to add special handling for a number of cases.

  • A "change" event happens for inputs of type "checkbox" and "radio", but the value is always "on" and page.fill isn't appropriate for those. I skip these fills and let the associated "click" event do the work instead.
  • A "change" event happens after you select a file for an input of type "file", but we can't use page.fill on these. I skip these fills. File selection will need to be handled elsewhere.
  • Many of the special input types have input masks. For example, date, datetime-local, time, month, week, etc. There are two special considerations for these.
    • Pressing Tab on a mask moves between the mask sections rather than moving to the next input. So I have special handling that does not generate a page.press for Tab key if it doesn't result in the target focus changing.
    • The "change" event fires multiple times for these inputs while tabbing around within the mask. I solved this with a very general check that ensures we never have two page.fill commands generated for the same selector in a row. Only the last is kept.
  • The Enter key does a line break when editing in a textarea, so I do not track Enter press when the target is a text area. The generated page.fill for a textarea selector includes all line breaks.

Testing

I haven't yet added Jest tests. For manual testing, I created the following HTML file, which I serve on localhost:

<html>
  <head>
    <title>Inputs Testing</title>
  </head>
  <body>
    <!--
      Not tested
      hidden
      button
      image
      reset
      submit
    -->
    <div><textarea rows="5"></textarea></div>
    <div><textarea rows="5">Test editing existing value</textarea></div>
    <div><input type="checkbox" /></div>
    <div><input type="color" /></div>
    <div><input type="date" /></div>
    <div><input type="datetime-local" /></div>
    <div><input type="email" /></div>
    <div><input type="file" /></div>
    <div><input type="month" /></div>
    <div><input type="number" /></div>
    <div><input type="password" /></div>
    <div><input type="radio" /></div>
    <div><input type="range" /></div>
    <div><input type="search" /></div>
    <div><input type="tel" /></div>
    <div><input type="text" /></div>
    <div><input type="time" /></div>
    <div><input type="url" /></div>
    <div><input type="week" /></div>
  </body>
</html>

@jperl jperl self-requested a review July 6, 2020 21:27
@jperl
Copy link
Member

jperl commented Jul 6, 2020

Awesome, thank you! I am reviewing this now.

We have a sandbox project we use for test pages. It should have all the inputs you need. https://github.com/qawolf/qawolf/tree/master/packages/sandbox

You can run it locally with npm start or if you use it as is you can just do npx @qawolf/sandbox

This is a script for generating fixtures.

@jperl jperl force-pushed the feat-support-input-fill branch 3 times, most recently from 5933874 to 879e981 Compare July 7, 2020 20:59
@aldeed
Copy link
Contributor Author

aldeed commented Jul 7, 2020

Great, thanks @jperl. I'll add a bunch of tests to verify and codify the special cases.

@jperl
Copy link
Member

jperl commented Jul 7, 2020

I ran into a few issues while reviewing this PR that I fixed on master, and rebased this PR onto:

  1. Fixed npm run watch not picking up web changes due to a typescript issue.
  2. Selector will not longer target the value attribute when the target is a non-checkbox/radio input or text area. This was causing issues because the selector was constantly changing.

The main issue I have with the PR is the experience is slightly worse, since changes only show up after the change event instead of while the user is typing.

@aldeed
Copy link
Contributor Author

aldeed commented Jul 7, 2020

@jperl I encountered 1 and was just restarting. Glad to know there's a fix. Selector fix makes sense.

I agree the lack of generated code while typing is a worse experience than before. I'll take a look at that because it might not be too hard to generate a temporary fill while typing and replace with the final value once it's known.

and generate fills while typing, updating them as you type
@aldeed
Copy link
Contributor Author

aldeed commented Jul 9, 2020

@jperl Fills are generated while you type now, and I added support for filling contenteditables. Haven't added the tests yet.

@jperl
Copy link
Member

jperl commented Jul 9, 2020

@aldeed Just tested it out. Works very well! Let me know if you want help with the tests.

@aldeed
Copy link
Contributor Author

aldeed commented Jul 9, 2020

@jperl I generated a bunch of fixtures and added many tests for buildFillSteps. No problems there. I also fixed some of the other failures in ed4da67. These seemed relatively straightforward side effects of my changes (type -> fill, "change" event now tracked, etc.). One of the things I did was re-record the "login" fixture and update snapshots related to that.

But there are still a couple more tests failing locally for me, and I'm not as sure about those. Would you mind taking a look at those and see if it's OK to update snapshots or if I'm unintentionally breaking something?

I also still need to add tests for buildPressSteps, but it should be just as easy the others were now that I know the secret event recorder command. 😄

@jperl
Copy link
Member

jperl commented Jul 9, 2020

I'll fix any broken tests now that were pre-existing before the PR.

@aldeed
Copy link
Contributor Author

aldeed commented Jul 9, 2020

@jperl One more thing: ts-node isn't listed as a dev dependency so I had to install it globally to get the saveFixture command to work. Is that intentional, or should I add it as dep?

@jperl
Copy link
Member

jperl commented Jul 9, 2020

@aldeed I went ahead and added it as a dev dependency.

Copy link
Member

@jperl jperl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After you feel like it has adequate test coverage, merge it 👍 🔥 🙏

@jperl
Copy link
Member

jperl commented Jul 9, 2020

Actually I found one issue. We do not want to include clicks due to pressing enter on a form. If you press enter in the password, it will also generate the click (even though there was no click). Investigating this now. Update: pushed a fix to this branch 1121d33

  await page.goto("localhost:5000");
  await page.click("text=Log in");
  await page.click("#username");
  await page.fill("#username", "tomsmith");
  await page.press("#username", "Tab");
  await page.fill("#password", "SuperSecretPassword!");
  await page.press("#password", "Enter");
  // don't want this click generated
  await page.click("button");
  await page.click("text=Log out");

@aldeed aldeed merged commit 3afe743 into master Jul 11, 2020
@aldeed aldeed deleted the feat-support-input-fill branch July 11, 2020 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants