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

Support command sequences in at-commands.mjs #438

Merged
merged 4 commits into from
Jun 10, 2021
Merged

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented Jun 1, 2021

Preview Tests

Fixes #363


This change supports the syntax proposed in #363 (comment)

1,navigate forwards to grid,reading,JAWS,"T,DOWN","DOWN,DOWN",,,,

without changing the structure of commands.json (the strings can contain commas), and without changing the CommandsAPI used by aria-at-harness.mjs. This is to not shake the foundation for e.g. #434, while still giving test authors the option to use command sequences.

For example, given this in test-01-navigate-to-unchecked-checkbox-reading.html:

      "nvda": [
        [ "X_AND_SHIFT_X,X_AND_SHIFT_X" ],
        [ "F_AND_SHIFT_F" ],
        [ "TAB_AND_SHIFT_TAB" ],
        [ "UP_AND_DOWN" ]
      ]

the rendered test instructions are:

  1. Using the following commands, Navigate to the first checkbox. Note: it should be in the unchecked state.
    • X / Shift+X followed by X / Shift+X
    • F / Shift+F
    • Tab / Shift+Tab
    • Up Arrow / Down Arrow

@jscholes
Copy link
Contributor

jscholes commented Jun 1, 2021

@zcorpan Some questions:

  • The examples, like checkbox, that have both forward and backward direction commands in a single test (e.g. F and Shift+F) are now considered outdated and not how tests are written. It would be good to demonstrate changes to the test format with something more up to date to avoid confusion.
  • Is there a limit to the number of commands in a sequence?
  • What is the wording for a sequence with more than two commands? Will the words "followed by" just be inserted between them?

@jscholes
Copy link
Contributor

jscholes commented Jun 1, 2021

Updated my previous comment; you did include a CSV example and I just didn't see it.

@zcorpan
Copy link
Member Author

zcorpan commented Jun 2, 2021

@jscholes

  • The examples, like checkbox, that have both forward and backward direction commands in a single test (e.g. F and Shift+F) are now considered outdated and not how tests are written. It would be good to demonstrate changes to the test format with something more up to date to avoid confusion.

OK. I think we should fix those tests. Is there an issue about this?

If checkbox-tri-state is one of the not-outdated tests, an example would be the ability to press Down Arrow twice:

Current:

Using the following commands, Navigate to the first checkbox. Note: it should be in the partially checked state. You may need to press certain commands (such as Down Arrow) multiple times to hear all information relating to the checkbox.

  • X
  • F
  • Tab
  • Down Arrow

New:

Using the following commands, Navigate to the first checkbox.

  • X
  • F
  • Tab
  • Down Arrow followed by Down Arrow

@jscholes

  • Is there a limit to the number of commands in a sequence?

No.

  • What is the wording for a sequence with more than two commands? Will the words "followed by" just be inserted between them?

Yes.

@jscholes
Copy link
Contributor

jscholes commented Jun 3, 2021

@zcorpan We discussed this on today's test writing CG meeting some questions/thoughts:

  1. Will the label of the output fields for each command sequence reflect the new command sequence wording? E.g. "Output for T followed by Down Arrow".
  2. Maybe we can do a separate PR later (based on data from upcoming test plans) to increase the readability of the wording e.g. instead of "Down Arrow followed by Down Arrow followed by Down Arrow", it could say "Down Arrow 3 times".
  3. The word "then" is preferable to "followed by" as it has two less syllables.
  4. Can we place a comma after each command in a sequence (other than the last)? I.e. "command 1, then command 2 ...".

@zcorpan
Copy link
Member Author

zcorpan commented Jun 3, 2021

@zcorpan We discussed this on today's test writing CG meeting some questions/thoughts:

  1. Will the label of the output fields for each command sequence reflect the new command sequence wording? E.g. "Output for T followed by Down Arrow".

Yes.

  1. Maybe we can do a separate PR later (based on data from upcoming test plans) to increase the readability of the wording e.g. instead of "Down Arrow followed by Down Arrow followed by Down Arrow", it could say "Down Arrow 3 times".

We can do that in this PR. Should it also say "Down Arrow, twice" instead of "Down Arrow, then Down Arrow"?

  1. The word "then" is preferable to "followed by" as it has two less syllables.

OK.

  1. Can we place a comma after each command in a sequence (other than the last)? I.e. "command 1, then command 2 ...".

Sure.

@jscholes
Copy link
Contributor

jscholes commented Jun 3, 2021

@zcorpan

We can do that in this PR. Should it also say "Down Arrow, twice" instead of "Down Arrow, then Down Arrow"?

The community group didn't discuss this aspect in detail, so we're happy to kick the can down the road on it for now. I don't want to speak to something on which we didn't reach consensus.

@zcorpan
Copy link
Member Author

zcorpan commented Jun 3, 2021

OK. For now I changed " followed by " to ", then ".

@jscholes
Copy link
Contributor

jscholes commented Jun 9, 2021

@zcorpan Where are we on getting this merged?

@zcorpan
Copy link
Member Author

zcorpan commented Jun 9, 2021

It can be merged when it's reviewed, which I think @mzgoddard will do.

Copy link
Contributor

@mzgoddard mzgoddard left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I guess one non-blocker for this that kinds to mind after reading the change is, does the formatting seem good for commands with furtherInstructions like:

        [
          "LEFT_AND_RIGHT",
          "(with Smart Navigation on)"
        ]

from tests/checkbox/commands.json?

After this change might that command be changed to this?

        [
          "LEFT,RIGHT",
          "(with Smart Navigation on)"
        ]

and if so currently that'll put into the test instructions:

- Left Arrow (with Smart Navigation on), Right Arrow (with Smart Navigation on)

@jscholes
Copy link
Contributor

@mzgoddard Smart navigation won't be tested going forward, and should be removed from the older tests at some point.

@zcorpan
Copy link
Member Author

zcorpan commented Jun 10, 2021

Thanks @mzgoddard! The furtherInstructions seems OK for now. Merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for command sequences in tests
3 participants