Debug failing a11y test on tabbed dialog variant in ShareDialog

[lyzadanger]

There is a skipped a11y test in ShareDialog's tests that will fail with the following error.

-[
      -  {
      -    "description": "Ensures all ARIA attributes have valid values"
      -    "help": "ARIA attributes must conform to valid values"
      -    "helpUrl": "https://dequeuniversity.com/rules/axe/4.7/aria-valid-attr-value?application=axeAPI"
      -    "id": "aria-valid-attr-value"
      -    "impact": "critical"
      -    "nodes": [
      -      {
      -        "all": [
      -          {
      -            "data": [
      -              "aria-controls=\"share-panel\""
      -            ]
      -            "id": "aria-valid-attr-value"
      -            "impact": "critical"
      -            "message": "Invalid ARIA attribute value: aria-controls=\"share-panel\""
      -            "relatedNodes": []
      -          }
      -        ]
      -        "any": []
      -        "failureSummary": "Fix all of the following:\n  Invalid ARIA attribute value: aria-controls=\"share-panel\""
      -        "html": "<button role=\"tab\" data-component=\"Tab\" type=\"button\" aria-selected=\"true\" id=\"share-panel-tab\" aria-controls=\"share-panel\" class=\"focus-visible-ring transition-colors whitespace-nowrap flex items-center gap-x-1.5 px-4 py-2 font-semibold text-grey-7 rounded-t border border-transparent border-b-0 aria-selected:text-color-text aria-selected:bg-white aria-selected:border aria-selected:border-grey-3 aria-selected:border-b-0 enabled:hover:text-color-text disabled:text-grey-7/50\">"
      -        "impact": "critical"
      -        "none": []
      -        "target": [
      -          "#share-panel-tab"
      -        ]
      -      }
      -    ]
      -    "tags": [
      -      "cat.aria"
      -      "wcag2a"
      -      "wcag412"
      -    ]
      -  }
      -]
      +[]

    at Context.<anonymous> (/Users/lyza/Projects/client/src/test-util/accessibility.js:87:14)

I believe I have a fair grip on how aria-controls is supposed to work and have verified that:

• We have a button role="tab" here inside of a div role="tablist"
• The contentious aria-controls attribute value of share-panel points at an extant div role="tabpanel" in the same document

Seems like that should be valid, but something is up here. It could be an artifact of our testing environment, say, a duplicated element id or something.

It's important that we get a11y right, so I don't want to lose track of this, but I also want to put it aside for a moment to get a couple of other things squared away.

[acelaya]

I have been debugging this test, and it passes if the TabHeader and Card that are rendered by ShareDialog when the feature flag is enabled, are wrapped into some kind of container other than a fragment.

diff --git a/src/sidebar/components/ShareDialog/ShareDialog.tsx b/src/sidebar/components/ShareDialog/ShareDialog.tsx
index 7f567133e..c1ae2ebc6 100644
--- a/src/sidebar/components/ShareDialog/ShareDialog.tsx
+++ b/src/sidebar/components/ShareDialog/ShareDialog.tsx
@@ -30,7 +30,7 @@ export default function ShareDialog() {
       variant={tabbedDialog ? 'custom' : 'panel'}
     >
       {tabbedDialog && (
-        <>
+        <div>
           <TabHeader>
             <Tab
               id="share-panel-tab"
@@ -71,7 +71,7 @@ export default function ShareDialog() {
               <ExportAnnotations />
             </TabPanel>
           </Card>
-        </>
+        </div>
       )}
       {!tabbedDialog && <ShareAnnotations />}
     </SidebarPanel>

I have been trying to find-out why this happens, and I have checked that, inside the accessibility test, a call to wrapper.html() returns something in these lines (simplified. I have removed classes and other non-relevant attributes to make it more readable):

<div data-testid="tab-header">
    <button
        role="button"
        aria-label="Close"
        type="button"
        title="Close"
    >
        ...
    </button>
    <div role="tablist" aria-orientation="horizontal">
        <button role="tab" type="button" aria-selected="true" id="share-panel-tab" aria-controls="share-panel">
            <span data-content="Share" >Share</span>
        </button>
        <button role="tab" type="button" aria-selected="false" id="export-panel-tab" aria-controls="export-panel">
            <span data-content="Export">Export</span>
        </button>
    </div>
</div>
<div data-component="Card">
    <div role="tabpanel" id="share-panel" aria-labelledby="share-panel-tab">
        <div data-component="CardTitle">
            <h1>Share Annotations in Test Private Group</h1>
        </div>
        <div class="space-y-3 pt-2"></div>
    </div>
    <div role="tabpanel" id="export-panel" aria-labelledby="export-panel-tab" hidden="">
        <div data-component="CardTitle">
            <h1>Export from Test Private Group</h1>
        </div>
        <div class="space-y-3 pt-2"></div>
    </div>
</div>

Since the tab header (which contains the tabs) and the card (which contain the tabpanels) do not have a common parent, I'm guessing it could be the test/accessibility helper/something is just completely ignoring the card as if it did not exist at all, resulting in the error about aria-controls="share-panel" being invalid.

If the above is wrapped in a div (or probably anything which is semantically correct), then the test passes.

I haven't checked if adding that extra div wrapper has other implications, but a workaround would be to wrap the ShareDialog just for this specific test, as in real use cases, there will always be a wrapper coming from some parent component.

[robertknight]

I find it very plausible that there could be an issue within either our checkAccessibility code or Enzyme with components that have a fragment as the top-level container. The fix should be implemented in the checkAccessibility code though, or at least worked around there if the problem is in a dependency.

[acelaya]

You are right @robertknight. I have checked the code of checkAccessibility, and at one point, if we detect we have passed an enzyme ReactWrapper instance, we do wrapper.getDOMNode(). This returns only the first element in the fragment.

[acelaya]

This issue seems related enzymejs/enzyme#1799

I'm trying to move the workaround to checkAccessibility, but since the wrapper is already created there, it's not as straightforward to render an extra container.

[acelaya]

I have tried another approach which implies working around it inside the accessibility test helpers:

diff --git a/src/test-util/accessibility.js b/src/test-util/accessibility.js
index 193ce2489..935288b18 100644
--- a/src/test-util/accessibility.js
+++ b/src/test-util/accessibility.js
@@ -24,8 +24,10 @@ async function testScenario(
 
   let wrapper;
   if (elementOrWrapper instanceof ReactWrapper) {
-    wrapper = elementOrWrapper;
-    container.appendChild(elementOrWrapper.getDOMNode());
+    wrapper = mount(
+      <div dangerouslySetInnerHTML={{ __html: elementOrWrapper.html() }} />
+    );
+    container.appendChild(wrapper.getDOMNode());
   } else {
     wrapper = mount(elementOrWrapper, { attachTo: container });
   }

But unfortunately, this breaks other tests.

I haven't found a reliable way to identify when the wrapper's first element is a fragment, in order to apply the logic above conditionally.

[acelaya]

This issue was fixed by https://github.com/hypothesis/client/pull/5672/files.

Tracking hypothesis/frontend-testing#7 for a long-term solution.