Skip to content

Pull Request Review Process

Matt King edited this page Dec 18, 2023 · 39 revisions

Overview

The types of reviews and approvals required by a request depend on the types of additions or revisions included in the pull request. Following is a description of what is covered by each review.

Editors can create a review tracker in a pull request by using the pull request review checklist.

Editorial review

Review all prose to ensure they are:

  • Clear and understandable.
  • Phrasing is consistent throughout the guide.
  • Accurately represent the meaning of the ARIA specification.
  • Consistent with W3C and APG editorial guidelines.

Functional review

Exercise the example with keyboard, mouse, and touch to ensure:

  • Keyboard behaviors match the pattern documentation.
  • Mouse and touch behaviors match expected norms.

Test using:

  • Chrome on Windows, macOS, and Android
  • Safari on macOS and iOS
  • Firefox on Windows

Visual design review

Review the visual presentation to assess whether look and feel is similar to typical modern implementations of the widget. Allow for adjustments to accommodate accessibility requirements in cases where most modern implementations are not sufficiently accessible.

Accessibility review

Review for compliance with applicable WCAG requirements as well as Assitive Technologies.

  1. Test with Assistive Technologies
  • Windows JAWS/NVDA/Narrator in several browsers (Chrome, Firefox, Edge)
  • Mac VO in several browsers (Safari, Chrome, Firefox)
  • iOS VO (Safari, Chrome), Android TalkBack (Chrome)
  • Note: Allow for assistive technology interoperability failures where the failure is due to lack of adequate support in the browser or assistive technology.
  1. Test in Windows High Contrast Mode (HCM):
  • Turn on HCM in Windows Settings - Ease of Access - High Contrast.
  • Open the example in Edge and Firefox (note that Firefox must be started after HCM is on).
  • Test in both High Contast Black and High Contrast White themes.
  • Make sure everything is visible and has suitable contrast in both themes. Pay particular attention to Non-text contrast, i.e. check that UI components and their states (e.g. focus/hover), as well as any graphical objects, have enough contrast with any adjacent colors.
  • You can use Color Contrast Analyzer to test contrast.
  1. Test high contrast support with "forced-colors"

    3-1. Testing forced color using Google Chrome Developer Tool

  • Open Google Chrome Developer Tools, click the three dots in Google Developer Tools, choose "More tools," and select "Rendering."

  • In the Rendering pane:

    • Emulate CSS media feature prefers-color scheme (Force CSS prefers-color scheme media feature): Select "prefers-color-scheme: dark" from the dropdown menu.
    • Emulate CSS media feature forced-colors (Force CSS forced-colors media feature): Select "forced-colors: active" from the dropdown menu.
    • Check if any active element is hidden due to a low color contrast issue

    3-2. "Styling for Windows high contrast with new standards for forced colors"

Code review

  1. Review clarity of JavaScript and CSS source based on expectations defined in the APG Code Guide.
  2. If this change is a wide sweeping change that will require contributors to update their branches (for example, the introduction of "use strict"; or a change in an frequently used regression test utility function), then this PR should contain in its description step by step instructions for contributors to quickly update their local PRs after the new change is merged into main. Code reviewers should make sure these instructions are there and review these instructions for clarity.
  3. If the code is for an example, then make sure the "Open in CodePen" button has been added and the code in the Codepen works as expected. You can read about how to add the "Open in Codepen" button on this wikipage.

Test review

  1. Check out the appropriate working branch locally and run npm run regression-report. The regression report will check the following:

    • whether data-test-ids are missing
    • whether tests are missing
    • whether test files are name correctly
  2. Confirm the following for the new test file:

    • There is a data-test-id attributes on every row of the keyboard and attribute documentation tables in the example's HTML file.
      • if the the attribute is testable (for example, if it is implicit), then it should have test-not-required
    • There is a test for every data-test-id.
    • The test files are name correctly (prefixed by the example directory and an underscore).
  3. Read through each test case and confirm the following:

    • The test is easy to read (if the test fails in the future, the action of the test should be manually repeatable by a future programmer trying to address the bug).
    • The test appropriate tests the behavior referenced in the example page's attributes or keyboard interaction tables.
    • The test covers all boundary conditions of the describe behavior, instead of only one example of the behavoir.
    • There are two code paths for most possible interactions with the example widget. One is using a mouse (clicking on parts of the widget, such as a menu) and the other is via the keyboard (sending key events instead of clicking). Both code paths should be reasonably covered.
    • As the tester and reviewer's discretion, tests of widget behavior not describe in the example page keyboard interaction and attribute tables can be written. These tests should have data-test-id set to "test-additional-behavior" to indicate they test behavior not tracked in the tables. These intention of these test must be particularly clear as there is no additional documentation for them.
  4. Finally, run the tests and confirm they pass.

Clone this wiki locally