search

w3c / aria-at-app

Test runner and results reporter for ARIA-AT
http://aria-at.w3.org/
Other
35 stars 15 forks source link

Mockups for Candidate Test Plan Review #437

Open isaacdurazo opened 2 years ago

isaacdurazo commented 2 years ago

Candidate Test Plan Review Process - AT Developer User Journey

Candidate Test Plans Queue

This is the page where an AT Developer begins a Test Plan review process. It has a table listing all Test Plans in the Candidate phase as well as the percentage of passing tests for each AT and Browser combination the Test Plans were run with.

 UI Characteristics and details

ARIA-AT - Candidate Test Plans Queue

Single Candidate Test Plan

Once the AT Developer selects a Candidate Test Plan they would land on this page. Here we have a header containing UI elements to display different statuses and actions the user can utilize for reviewing. The main content of this page has a table listing every Assistive Technology used for every Candidate Test Plan and a column for each Browser used for each AT. 

 UI Characteristics and details

ARIA-AT - Single Candidate Test Plan page

Single Assistive Technology with multiple browsers

After selecting an Assistive Technology, the user would land on this page, where they can look at a dedicated results table for each AT, which lists each of the tests within the test plan and a column for required and optional assertions, and unexpected behaviors 

UI Characteristics and details

ARIA-AT - Single Assistive Technology page

Single Assistive Technology and Browser combination

Lastly in the review process, on this page, the AT Developer can look at a detailed reports table for each Test for a given AT and Browser.

ARIA-AT - Single Assistive Technology and Browser combination

jscholes commented 2 years ago

@isaacdurazo I like it! Some thoughts:

Candidate Test Plans column: the header cell of this column proposes a tabbed navigation

Is the proposal that a table column header cell will contain a tab list? That is extremely unusual from a semantic perspective, and raises concerns for me about cognitive load. In addition and most critically, the names of the two tabs will bubble up to become the accessible name of the cell itself, meaning that as a screen reader user navigates across columns, they will hear both tab names announced.

If it is intended that a user will be able to switch between two table views, the tab list should be before the table and not inside it.

X number of open issues

Is this the number of issues for the plan in total, or is it scoped to the vendor user who is signed in?

Ok: once an AT developer approves a Candidate test plan an "Ok" label is displayed next to its name

I think this should be more explicit. For me, "Ok" without additional context doesn't get across the intended meaning, i.e. that the vendor has approved it. How about just using the word "Approved", as is the case on the single test plan page?

Note: this does relate to my previous question. If, say, Vispero have approved a plan, but issues raised by Apple are still open, will the Vispero representative see the approved status on its own? Or will it be accompanied by the number of open issues too?

The cell contents of these columns could either be the word "None" if there are no test results

Alternatively, is there a downside to only showing columns for the combinations that were tested?

Add your Review: Clicking this opens a Dropdown with three checkboxes to choose from depending on the type of review and text area. The three checkbox options are: Approve, Provide Feedback, and Request Changes.

These seem like mutually exclusive options (e.g. approving while requesting changes doesn't make sense as a use case). So should they be radios?

Actions: There are four actions in this row, "Open Test", "Raise an Issue about this Test", "Leave Feedback" and "File an AT bug".

I feel like maybe this has been discussed, but I'm not sure the distinction between raising an issue and leaving feedback will be clear to the vendors. I'm not sure I remember that distinction (if one was determined).

isaacdurazo commented 2 years ago

@jscholes thanks a lot for the feedback! My response to your comments:

Is the proposal that a table column header cell will contain a tab list?

Yes, that's my proposal, but after reviewing your commend and concern I realize that yes, we should move away from this. I think having the tabbing outside the table should work.

Is this the number of issues for the plan in total, or is it scoped to the vendor user who is signed in?

It is the number of issues for the plan in total. These issues will be properly labeled on Github, which is not defined yet what labels we will be using.

think this should be more explicit. For me, "Ok" without additional context doesn't get across the intended meaning, i.e. that the vendor has approved it. How about just using the word "Approved", as is the case on the single test plan page?

I see your point, let's propose "Approved".

Note: this does relate to my previous question. If, say, Vispero have approved a plan, but issues raised by Apple are still open, will the Vispero representative see the approved status on its own? Or will it be accompanied by the number of open issues too?

Great question! The case you have stated did come up to me while I was working on these mockups and was hoping we all can come up with the best solution. My thinking right now is that if let's say, Vispero approved a Test Plan and then someone else raises an issue, the "Approval" label should be removed and reinstated once those issues have been resolved.

Alternatively, is there a downside to only showing columns for the combinations that were tested?

Not at all, but we have all Candidate Test Plans in the same table and there might be a case where maybe one of the Test Plans didn't get tested with a particular AT and Browser combination. Since they all share the same table, we wouldn't be able to display the progress bar for that Test Plan that didn't get tested with said AT and Browser. Does that make sense?

These seem like mutually exclusive options (e.g. approving while requesting changes doesn't make sense as a use case). So should they be radios?

Whoops, that was an oversight on my end. You are 100% right. These should be radio buttons.

I feel like maybe this has been discussed, but I'm not sure the distinction between raising an issue and leaving feedback will be clear to the vendors. I'm not sure I remember that distinction (if one was determined).

The distinction is that raising an issue blocks approval, while leaving feedback doesn't. It's good that you are bringing this up. We should figure out how to add more clarity to it.

mcking65 commented 2 years ago

I don't quite understand the intent of the top-level table. At this point, we have 3 screen readers, later there will be more screen readers. At the highest level, we are concerned about tracking the plan review status for each screen reader. Browsers do not matter. The table describes the status as being in the first column with the plan name, but we actually need a status for each screen reader. For disclosure nav menu, we need to know for each screen reader: how many issues raised, is approved, etc.

jscholes commented 2 years ago

@isaacdurazo

My thinking right now is that if let's say, Vispero approved a Test Plan and then someone else raises an issue, the "Approval" label should be removed and reinstated once those issues have been resolved.

Makes sense to me. However, we should consider the case of a vendor not being actively involved in the process, either at all or to the degree that we'd like. At some point, if they don't approve a given test plan, it will still need to continue to the next phase regardless. And, we don't want to suggest that one vendor approving a plan implies approval from another who is actively invested.

we have all Candidate Test Plans in the same table and there might be a case where maybe one of the Test Plans didn't get tested with a particular AT and Browser combination. Since they all share the same table, ...

Great point. That completely hadn't occured to me.

s3ththompson commented 2 years ago

After consideration of feedback and discussion at the last two ARIA-AT CG meetings, @isaacdurazo is proposing the following changes to the above mockups. The mockups will be updated to reflect these changes soon.

We're looking forward to discussing these changes in more detail at the upcoming CG meeting. We're happy to provide additional context and considerations for these choices or to consider other options for these design challenges.

isaacdurazo commented 2 years ago

Thank you @s3ththompson for explaining the last changes we would like to propose. Here is the updated version of both visual and text-based mockups.

Candidate Test Plans Queue

This is the page where an AT Developer begins a Test Plan review process. It has a table for every AT that lists all Test Plans in the Candidate phase.

 UI Characteristics and details

ARIA-AT - Candidate Test Plans Queue v1 1

Candidate Test Plan Review page

Once the AT Developer selects a Candidate Test Plan they would land on this page. Here we have a header containing UI elements to display different statuses. The main area resembles the Test Run page with a left-hand navigation section with an ordered list of all tests and full test material on the right

UI Characteristics and details

ARIA-AT - Candidate Test Plan Run

Finish your review modal

When the reviewer reaches the last test in a Candidate Test Plan and clicks "Finish Review", they are prompted with a Modal with the following content.

ARIA-AT - Candidate Test Plan Review

ARIA-AT - Candidate Test Plan Success Page

mcking65 commented 2 years ago
Matt's feedback on Candidate Test Plans Queue page

Overall, sounds like a simple, clear design with straightforward calls for action.

I like that there is space for an introductory description.

I like that at a high level, it is organized by AT so the rep for each AT has a clear place to focus. This structure can work for 3 AT but also scales well up to 6 or 12 AT.

I have several suggestions:

  1. Include an H2 heading for each AT that makes the CTA for the AT even more clear, e.g., <h2>JAWS Test Plan Reviews by Vispero</h2>.
  2. Make the headings collapsable (use ARIA accordion pattern) and show them in the collapsed state by default, so multiple AT can be visible above the fold. This will make it easier for everyone to see how many AT vendors are in scope for the current review work.
  3. Change the wording for the status based on feedback below.
  4. In each AT table add columns for:
    1. Candidate Phase Start Date
    2. Target Completion Date
    3. total number of tests
    4. Number of tests with failures. Alternatively, a "Results Summary" column could contain something like X assertions failed across Y tests run with Z browsers
  5. At the end of the page, add a summary section with a table so test admins can easily see the overall review status for each plan and include an action for moving the plan to recommended phase. This table could have columns for:
    • test plan name
    • Candidate Phase Start Date
    • Target Completion Date
    • Approvals (list of AT vendors who have marked approved)
    • Number of open feedback issues (which would be a link that opens an issues list in GitHub filtered to those issues)
    • Actions: Menu button that opens actions menu for the plan (Publish as recommended, Change target date)

With respect to plan status, I am concerned about the word "issue". It is likely that AT vendors could confuse "issues" with "test failures". Let's work on ways of labeling information that mitigates the risk of such confusion.

Perhaps we could refer to the GitHub issues raised by AT developers as "Test Feedback". If we can distinguish between "general feedback" and "changes requested" that would be even better.

If an AT developer has read some or all of a test plan but not filed any feedback, would we still show the status of "ready for review"? Or, would we show a status like "Partially Read" or "Read" or "Review In Progress"?

Here are suggestions for allowed plan review status values:

Matt's feedback on Candidate Test Plan Review page

The description of main content says:

The main content of this page has a table listing every Assistive Technology used for every Candidate Test Plan and a column for each Browser used for each AT. 

Is that a copy/paste error? I don't understand it. This page is about one test in one plan with results for that test in one AT. It should not show data for multiple AT nor for multiple plans.

That AT may have candidate test results from more than one version of the AT and the test could have been run with that AT in more than one browser. For example, for JAWS, the disclosure menu test plan could have results for the test of navigating forward to the menu for two versions of JAWS in both Chrome and Firefox. This has consequences discussed below.

The description of the header says:

Heading: Comprised of a <h1> with the name of the Test Plan, e.g. "Disclosure Navigation Menu Example" followed by a label with the number of open issues

I don't think we need the plan name in the H1 text. The H1 text needs to indicate which test is being reviewed just like in the test runner, e.g.:

Testing task:1. Navigate to the first unchecked radio button in a group in reading mode

Although, I'd prefer different wording from the runner. I recommend:

Review Test 1 of 26: Navigate to the first unchecked radio button in a group in reading mode

I don't think that we should put an issue count in the H1 text. I am concerned that people will not understand what that means. Further, it is unlikely that a vendor will open multiple GitHub issues for a single test in a plan. In fact, we might not want more than one issue from a given vendor for a given test in a given plan.

Nevertheless, in the H1, I think it would be helpful to also indicate both which AT the plan is for and the current read/feedback status for the test. Note that the status for a test is not a "Review Status" because the review status is for the plan, not the test. We should not imply that we are asking for "approval" on every test.

The status values we could use for the test in the H1 as well as in the left navigation could be:

  1. Not Previously Viewed
  2. Previously Viewed
  3. Feedback Filed
  4. Change Requested
  5. Changed Since Last Viewed

The header description says:

Status row: There are 3 different "status" indicators in this row. One for the phase in which the Test Plan is (Candidate in this case), another one for the number of open issues, and lastly one for the kind of review an AT Developer might have left. For the latter, there are 3 different kinds:

  • Approved: When an AT Developer approves a Candidate Test Plan, we display a green check mark icon followed by the text  [username] Approved this Test Plan.
  • Requested Changes: When an AT Developer requests changes on a Candidate Test Plan, we display a red x icon followed by the text: [username] requested changes.
  • Left Comments: When an AT Developer leaves feedback on a Candidate Test Plan, we display a blue bubble text icon followed by the text: [username] Left comments.

I think we should have a slightly more verbose header to avoid confusion. Clarity trumps brevity here.

Here is a suggestion for header content:

<h1>Review Test I of N: TEST_NAME using AT_NAME (READ_STATUS)</h1>
<ul>
  <li>Test I in Candidate Test Plan: TEST_PLAN_NAME</li>
  <li>Status of candidate review by VENDOR_NAME: PLAN_REVIEW_STATUS</li>
  <li>Target candidate review phase completion: TARGET_DATE</li>
  <li>Feedback from VENDOR_NAME:
    <ul>
      <li>STATE_OF_FEEDBACK_FOR_TEST</li>
      <li> STATE_OF_FEEDBACK_FOR_PLAN for the tests in this plan</li>
    </ul>
  </li>
</ul>

For example:

<h1>Review Test 1 of 26: Navigate to the first unchecked radio button in a group in reading mode using JAWS (Previously Viewed)</h1>
<ul>
  <li>Test 1 in Candidate Test Plan: Radio Group Example Using aria-activedescendant</li>
  <li>Status of candidate review by Vispero: In Progress</li>
  <li>Target candidate review phase completion: December 31, 2022</li>
  <li>Feedback from Vispero:
    <ul>
      <li><a href="link-to-issue">Roxana Fischer requested changes to this test</a><li></li>
      <li>Feedback filed for 3 tests in this plan</li>
    </ul>
  </li>
</ul>

The description of the main area says:

Test Material: This area contains the same materials from the Test Run page: instructions, test page link, commands, and assertions. All the output and results are visible as read-only.

Please do not show a read-only form. That is crazy hard to read with a screen reader because you have to read all the inputs to figure out the results. Please use the static table results format that is shown in the runner when reviewing completed tests. Of course, you need to also include the instructions and button to open the test case.

BTW, in the test runner when reviewing completed tests, we really, really need the instructions and button to open the test case. Having to go into edit mode to get to those elements is a serious drag. I ran into this problem constantly when working on conflicts. I needed to run the test to assess a plan of action, and the only way to get access to the test was to go into edit mode.

The description says:

Next to the main heading for every test two buttons have been added: "Raise and issue" and "File an AT bug"

I think you mean for each command, not each test? The entire page is for a single test that has results for one or more commands.

I assume there are also navigation buttons for previous and next?

s3ththompson commented 2 years ago

Candidate Test Queue Page

Nomenclature & copy changes

This makes sense. @isaacdurazo will rework the copy around “feedback” (and/or “changes requested”) instead of “issues”.

Review Status

You suggested the following values:

Some of these options are not mutually exclusive. For example, “X tests changed since last review” could apply independently of the review status.

I suggest that we separate these options into three discrete indicators:

Visibility in Test Queue

For now, we’ll keep the test plan report visible in the test queue while it’s in candidate review. Going forward, we need to map out the lifecycle of a report and exactly when and how it moves between different pages and different roles. I believe we have some time scheduled for a deeper discussion there.

Additional columns

We have two followup questions for the additional columns on the AT tables:

Additional summary table across ATs

@isaacdurazo will update the mockups to reflect this addition. The use case here makes sense.

Markup changes

We’ll reference this markup during implementation.

Disclosure Pattern for AT tables

Noted, good idea.

Candidate Test Review Page

Read-only form

Thanks for highlighting the accessibility issues with using a read-only form. We can render the submitted result values as plaintext instead of form inputs. Given that, would you prefer:

Both options would include the same information. Is the table easier to navigate? There was some concern that the cells contained too much information for longform readability.

(Also, agreed on adding instructions + open test page button to results summary in test runner)

Action Buttons

The two action buttons “raise an issue” (now “raise feedback”) and “file an AT bug” are to the right of the test content. The previous / next buttons are below the test content.

This layout mirrors the layout of action buttons in the test runner view. Your comment suggests creating separate action buttons per-command? I don’t think we have a UX pattern for this. I would suggest starting with the buttons in their usual location (per-test) and re-visiting after we have some feedback from vendors.

Markup

We’ll reference this markup during implementation.

Thanks for your thorough review Matt!