w3c / at-driver

AT Driver defines a protocol for introspection and remote control of assistive technology software, using a bidirectional communication channel.
https://w3c.github.io/at-driver
Other
31 stars 4 forks source link

Keypresses #26

Closed zcorpan closed 1 year ago

zcorpan commented 2 years ago

API to simulate keypresses. Milestone 3 in #15


Preview | Diff

zcorpan commented 2 years ago

From today's AT Automation Group Meeting (minutes)

We should have these commands:

If we need keydown/keyup in the future, it can be added at that time.

sendKeys (which is "type text") doesn't need to be supported since it's a convenience command that is more relevant in WebDriver (for typing text into e.g. an <input> element).

The level of keyboard simulation should be such that the AT will respond to them, but there may be security risks with too low-level simulation.

For keys that the AT doesn't itself respond to, those keys should be forwarded to the OS/focused application (as normal if the user were to press the same keys).

jkva commented 2 years ago

In order to understand the extraction script better, I've separated some code out into functions. Feel free to incorporate what seems useful to you.

import { Lexer, Parser } from 'cddl';
import * as fs from 'node:fs/promises';

const parseCddl = input => new Parser(new Lexer(input)).parse();

const cddlToJSONSchema = input => {
  const cddlAst = parseCddl(input);
  const jsonSchemaAst = {
    "$schema": "https://json-schema.org/draft/2020-12/schema",
    "$id": "TODO",
    "$defs": {},
  };
  for (const obj of cddlAst) {
    console.log(JSON.stringify(obj, null, 2)); // TODO
    // For SessionNewCommand, need to not set additionalProperties: false
    // since Command allows additional properties, all schemas in Command's allOf
    // need to also allow additional properties (otherwise 'id' will be invalid).
  }
}

const removeIndentation = (indentation, cddl) => {
  if (indentation.length === 0) {
    return cddl;
  }

  let indentationRegexp = new RegExp(`^${indentation}`);
  return cddl
      .split('\n')
      .map(line => line.replace(indentationRegexp, ''))
      .join('\n');
}

const formatCddl = cddl => cddl.join('\n\n').trim() + '\n';

const extractCddlFromSpec = async () => {
  const source = await fs.readFile('index.bs', { encoding: 'utf8' });
  const matches = [...source.matchAll(/^([ \t]*)<pre class=['"]cddl((?: [a-zA-Z0-9_-]+)+)['"]>([\s\S]*?)<\/pre>/gm)];

  const [local, remote] = matches.reduce(([local, remote], match) => {
    const [_, indentation, cssClass, content] = match;

    let isLocal = cssClass.indexOf(' local-cddl') > -1;
    let isRemote = cssClass.indexOf(' remote-cddl') > -1;

    if (!isLocal && !isRemote) {
      return [local, remote];
    }

    let cddl = removeIndentation(indentation, content.trim());

    if (isLocal) {
      local.push(cddl);
    } else {
      remote.push(cddl);
    }

    return [local, remote];
  }, [[], []])

  return [
      formatCddl(local),
      formatCddl(remote)
  ]
}

try {
  await fs.mkdir('schemas');
} catch(ex) {
  if (ex.code !== 'EEXIST') {
    throw ex;
  }
}

const [localCddl, remoteCddl] = await extractCddlFromSpec();

await fs.writeFile('schemas/at-driver-local.cddl', localCddl);
await fs.writeFile('schemas/at-driver-remote.cddl', remoteCddl);

// Convert CDDL to JSON Schema
cddlToJSONSchema(localCddl);
cddlToJSONSchema(remoteCddl);
jkva commented 2 years ago

@zcorpan I noticed you also ran into a problem with the module entry script being incorrect; I've submitted a PR which addresses this. Yet if this is not incorporated by the maintainer, we should consider forking the module.

jugglinmike commented 1 year ago

@zcorpan I noticed you also ran into a problem with the module entry script being incorrect; I've submitted a PR which addresses this. Yet if this is not incorporated by the maintainer, we should consider forking the module.

Much appreciated. I'm including a corrected version of the link so that GitHub recognizes the relationship between this pull request and that one:

https://github.com/saucelabs/cddl/pull/5

And in the mean time, the following change will allow the script in this patch to run (though I don't recommend relying on it indefinitely):

diff --git a/scripts/extract-schemas.js b/scripts/extract-schemas.js
index 0bd627d..582e0fc 100644
--- a/scripts/extract-schemas.js
+++ b/scripts/extract-schemas.js
@@ -1,4 +1,4 @@
-import { Lexer, Parser } from 'cddl';
+import { Lexer, Parser } from 'cddl/build/index.js';
 import * as fs from 'node:fs/promises';
zcorpan commented 1 year ago

Thank you for your review @jkva ! I've incorporated your script refactoring in #19 and I think all comments are now addressed.

I would like to merge each open PR in order so that the history is clear both in the commit log and in GitHub's UI. We've set up this repo to require an approved review in GitHub before merging PRs. Since you have reviewed this, it means #19 and #25 are also reviewed. Can you mark this PR and the other 2 as reviewed, please?

The Settings PR is not included here: #22. If you have reviewed that one, can you also approve it?

Thanks!

s3ththompson commented 1 year ago

@jkva could you take another look and mark as reviewed if this looks good to merge? thanks!

zcorpan commented 1 year ago

Not per se related to this PR, but I notice that lines 221 and 222 of index.bs refer to the key platform while it seems that ought to be platformName.

Good catch! Fix: https://github.com/w3c/aria-at-automation/pull/29

There are several lines where there is referral to a definition having been defined in CDDL. This seems redundant and perhaps ought to be written as a definition existing in as CDDL.

Hmm, yeah. Filed a new issue: https://github.com/w3c/aria-at-automation/issues/30

zcorpan commented 1 year ago

I've now updated the PR to specify interaction.pressKeys as discussed in https://github.com/w3c/aria-at-automation/pull/26#issuecomment-1178106223

Include a special modifier for the screen-reader specific modifier keys.

This is not done yet. How do we want to support this? Should there be a special string for each vendor in place of the raw key string, e.g. "nvda", "macOS VoiceOver", etc?

{
  "method": "interaction.pressKeys",
  "params": {
    "keys": ["nvda", "a"]
  }
}

Or a boolean property in InteractionPressKeysParameters to indicate that the screen reader specific modifier keys should be pressed?

{
  "method": "interaction.pressKeys",
  "params": {
    "keys": ["a"],
    "vendorModifier": true
  }
}
zcorpan commented 1 year ago

I filed a new issue #34 to track the above question.

As discussed in our call today, we could merge this PR without special modifier support and address that later.

jugglinmike commented 1 year ago

Here's that patch: https://github.com/w3c/aria-at-automation/pull/41