fix: Clean up accessibility node hierarchy (experimental)

[BenHenning]

The basics

• I validated my changes

The details

Resolves

Fixes #9304

Proposed Changes

Fixes non-visual parent roles and rendered connection navigation policy.

Reason for Changes

Other PRs have made progress on removing extraneous accessibility nodes with #9446 being essentially the last of these. Ensuring that parent/child relationships are correct is the last step in ensuring that the entirety of the accessibility node graph is correctly representing the DOM and navigational structure of Blockly.

This can have implications and (ideally) improvements for certain screen reader modes that provide higher-level summarization and sometimes navigation (bypassing Blockly's keyboard navigation) since it avoids an incorrect flat node structure and instead ensures correct hierarchy and ordering.

It was discovered during the development of the PR that setting aria-owns properties to ensure that all focusable accessibility nodes have the correct parent/child relationships (particularly for blocks) isn't actually viable per the analysis summarized in this comment: #9449 (comment). At a high level introducing these relationships seems to actually cause problems in both ChromeVox and Voiceover.

Part of the analysis discovered that nodes set with the presentation role aren't going to behave correctly due to the spec ignoring that role if any children of such elements are focusable, so this PR does change those over to generic which is more correct. They are still missing in Chrome's accessibility node viewer, and generic seems to introduce slightly better group behaviors on VoiceOver (that is, it seems to reduce some of the group announcements which VoiceOver is known for over-specifying).

Note that some tests needed to be updated to ensure that they were properly rendering blocks (in order for RenderedConnection.canBeFocused() to behave correctly) in the original implementation of the PR. Only one test actually changed in behavior because it seemed like it was incorrect before--the particular connection being tested wasn't actually navigable and the change to canBeFocused actually enforces that. These changes were kept even though the behaviors weren't needed anymore since it's still a bit more correct than before.

Overall, #9304 is closed here because the tree seems to be about as good as it can get with current knowledge (assuming no other invalid roles need to be fixed, but that can be addressed in separate issues as needed).

Test Coverage

No automated tests are needed for this since it's experimental but it has been manually tested with both ChromeVox and Voiceover.

Documentation

No documentation changes are needed for these experimental changes.

Additional Information

Note that there are some limitations with this approach: text editors and listboxes (e.g. for comboboxes) are generally outside of the hierarchy represented by the Blockly workspace. This is an existing issue that remains unaffected by these changes, and fixing it to be both ARIA compliant and consistent with the DOM may not be possible (though it doesn't seem like there's a strong requirement to maintain DOM and accessibility node tree hierarchical relationships).

The analysis linked above also considered introducing a top-level application role which might change some of the automated behaviors of certain roles but this only seemed to worsen local testing with ChromeVox so it was excluded.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[BenHenning]

Looks like there's some CI failures to address. Moreover I haven't completed in-depth testing of the hierarchy--I want to check more block and fields types before considering this ready for review.

[BenHenning]

Quick spot check on the code.

[BenHenning]

CI failures seem to be existing and unrelated to this change.

[BenHenning]

I think I'm happy with considering #9304 resolved with this PR. It's certainly possible we'll have deviations that will require future work, but as of now the tree is quite clean and I think most remaining filed work is largely on the label side and unlikely to impact the hierarchical relationships. Adding new focusable nodes to the graph might impact it, but that's not imminently planned.

PTAL @maribethb.

[maribethb]

Can you file an issue for this and link it here so we don't lose track of this?

[BenHenning]

I checked this again (with the goal of trying to figure out when it actually crashes) and I couldn't get it to crash again. I think maybe I had a weird old version or something that was set up incorrectly since my confusion here was that it shouldn't be possible for such a case to occur.

I updated canBeFocused and removed all of the hacky workarounds for this bit so I don't think an issue is needed at the moment (since there's no longer a mysterious TODO).

[maribethb]

Do you need to check this? What happens if you say aria-owns with an id that doesn't exist in the dom?

If you need to keep it, can you add the same bug ID in the todo that makes it public so that we can make it private again when it's no longer needed (I'm assuming if this bug didn't exist you wouldn't need to check this)

[BenHenning]

This has been revised to only use canBeFocused which, in turn, uses findHighlightSvg. This actually has the same effect, I think, but it's more logically correct.

The connection elements do exist in the DOM, they're just hidden. The logic is being set up to set aria-owns on them now (since the logic is no longer checking isHighlighted) and it seems like the tree is correct. It doesn't show the elements since they're hidden and thus not focusable. When they're made focusable then they correctly show up in the tree without technically needing to recompute the label. We might even be able to remove the call to recompute the block label upon highlight/unhighlight, but I didn't check that.

Edit: I am now checking removing the extra compute calls per your other comment.

[maribethb]

Please add a tsdoc with @internal so it doesn't get made part of our public API, and add same bug id from previous comment

[BenHenning]

Fortunately not needed anymore since I was able to make this private again.

[maribethb]

It seems like a lot of extra work to recompute the entire aria label which includes searching through all of the inputs, child blocks, etc. just to remove one specific ID for the connection highlight, which we already know the ID of.

Could you keep track of the childElemIds as a set and just remove or add this connection ID to the set, and then update the aria-owns attribute with the set of IDs instead of recalculating the entire set of IDs on each highlight?

[BenHenning]

I think this can actually be avoided from local experimentation, but I also noticed far more connections are being parented than necessary, and in sometimes the wrong location. I fixed the former by aligning the filtering logic to be based on the same logic used for navigation, but the latter is still an issue I haven't fully figured out yet.

[BenHenning]

I have no clue why this automatically re-requested a review from the reviewer queue--very odd. I didn't do anything on the PR to trigger that action.

[BenHenning]

Re-spot checked latest.

[BenHenning]

I've verified that connections have correct ownership in the node hierarchy now and that tests are passing (both for Core & linked against k-e).

PTAL again @maribethb.

@gonfunko it might actually be worth it for you to take a look specifically at the navigation_test changes since I did change the behavior of one test (just to spot check that my understanding is correct).

[BenHenning]

• Problem: feat: Make Flyout an ARIA list (experimental) #9528 (comment).
• Also address: feat: Make Flyout an ARIA list (experimental) #9528 (comment)

[BenHenning]

• When navigating up from a statement block inside an if block, the VoiceOver cursor doesn't follow the focused element and the if block label is not read out. Update: the addition of this line appears to be the cause of the VO cursor becoming decoupled from the focused element during workspace navigation. Removing it addresses this problem.

From @microbit-robert in #9528. The link above corrected to the final PR is: https://github.com/RaspberryPiFoundation/blockly/pull/9528/files/6660963ce8f757e44507eb105fa8fdd9104f25bb#diff-bb0de0069443e7577f62ffa96e6b51c5fafd06703f50c3eed6eb616ac87467bfR282 I believe.

I can't repro the exact issue with ChromeVox but I did notice another odd behavior that may be related: navigating between an if block and its first child results in 'Region End' always being red out despite there not actually being a region ending. This definitely suggests something odd going on for the tree structure.

[BenHenning]

Confirming that removing the ownership causes the weird 'region end' behavior to go away. Two screen readers failing for the same reason in a different strongly suggests something's wrong, but I'm not sure why it's wrong yet. The node tree looks correct, but perhaps that's misleading.

[BenHenning]

I'm starting to have a sneaking suspicion that I understand why this is breaking in odd ways. I don't understand why it's breaking in the way that it is--that would probably just be guesswork without being able to dig into the readers themselves. However, this may be a pretty good place to start.

I originally looked at the presentation role as basically "ignore this for accessibility because it's display only" (it is a synonym for none, after all). Reading the documentation closer, that's not actually true per https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles/presentation_role:

The presentation role and its synonym none remove an element's implicit ARIA semantics from being exposed to the accessibility tree.

The content of the element will still be available to assistive technologies; it is only the semantics of the container — and in some instance, required associated descendants — which will no longer expose their mappings to the accessibility API.

(emphasis mine)

I didn't make much of this originally but now I'm starting to second guess the accessibility node tree. I think the node tree is showing an incomplete view of what the reader actually has available to use for its readout in spec. While there are cases that seem outright incorrect (whenever Narrator or VoiceOver allow focusing a non-focusable element), but readers seem well within their scope to access the content of elements with disabled semantics. I actually suspect this is why we see group announcements on VoiceOver but not ChromeVox. Groups are still part of the accessibility tree even if they have disabled semantics.

From what I can tell, the only way to actually force a reader to ignore an element is to set it as aria-hidden. However, this has a substantial drawback:

Adding aria-hidden="true" to an element removes that element and all of its children from the accessibility tree. This can improve the experience for assistive technology users by hiding...

(emphasis mine again)

This puts us in a difficult position. We apparently cannot rely on presentation to ignore things and we cannot hide them because we have element parents which must be part of the tree otherwise their children (like the block paths) cannot be accessed. It seems that the current design of the accessibility standard forces a synchronization between the DOM and accessibility trees that cannot actually be avoided, and I suspect this is a huge cause of our screen reader inconsistencies. Most normal web pages won't run into these problems because they can rely on semantic HTML which ensures ARIA & DOM tree alignment outright, and readers have been optimized for those use cases.

Using some AI searching to dig a bit more into what might be happening and possible options:

• aria-hidden forcibly removes the entire affected DOM tree from the accessibility tree and nothing can bring children elements back apparently.
• VoiceOver reading out 'g' is likely a violation of the WCAG specification. This may be WAI, however, as apparently VoiceOver is designed to have special behaviors for SVGs that may not also happen if we were using divs directly.
• Gemini actually found a mistake of mine: presentation is apparently ignored if any children are focusable which might also be an explanation for the current behavior. This might mean that VoiceOver is operating within spec and ChromeVox isn't, but it's a bit difficult to be sure here. Nevertheless, it recommends using generic though I'm not a huge fan of that (since it means explicit opt into the tree), but I suspect that might fix the 'group' problem on VoiceOver. It's worth trying.
• Gemini also recommended using the application role to try and guide screen readers away from making special assumptions about SVG elements. I'm a bit skeptical this will do anything--early testing on ChromeVox seems to make things worse (it starts being treated as a landmark element like regions and that causes a mess of issues).
• Gemini suggests that aria-owns has created a context jump when switching between parent and child due to the accessibility hierarchy not matching the DOM hierarchy. This context loss requires a new parent to be computed which creates a 'boundary event' which apparently triggers the 'region end' announcement in ChromeVox. This whole thing reads as especially hallucinated, but I think there are nuggets here that sound logically correct. The conclusion is that ChromeVox is possibly making a mistake when considering both the DOM and accessibility hierarchies here.
• Gemini suggests using tree to solve these issues. :) Obviously they did solve the issues for us but introduced other ones (confusing level/tree context and poor VoiceOver support).

I am a bit stuck and need to think on this more. It feels like we're backed into a design corner:

• Flattening the hierarchy or moving away from SVGs would be the most obvious route to try and avoid these complexities, both of which are undesirable for other reasons or ludicrously out of scope.
• We can continue leveraging the ARIA spec incorrectly and hope for the best, but we must expect continued variance between screen readers that could continue to change over time with new browser and screen reader releases.
• We could try to work with the WCAG spec authors to remedy some of these pitfalls (and/or consider future WCAG draft features that might make this a lot easier), but that is a very far looking solution that doesn't solve the imminent next few years of Blockly usage.

Option (2) is the only viable approach, but I'm stuck on exactly how to go about the inconsistencies in their current form. Some more experimentation will help but we are definitely running out of ARIA features. :) At this point Gemini is also trying to convince me to implement concerningly hacky solutions to try and trick screen readers and hope for the best, so I think it's also uncertain on how to proceed here.

[BenHenning]

@microbit-robert or @gonfunko if you get a chance could you try testing this with VoiceOver? I disabled the ownership bit which should fix the lost context when navigating between C-shaped parent/children, but I also changed the default 'hidden' role to generic rather than presentation. I'm curious if this fixes the rogue 'group' pieces that seem to be included in readouts.

[microbit-robert]

@BenHenning I had a very quick test with VoiceOver. The navigation now looks good in the main workspace. I still get "group" when navigating through the flyout items. I also hacked around the HTML and the only thing that seems to make "group" go away is adding an aria-roledescription attribute with some reasonable value(?) which feels like it shouldn't be necessary and may negatively impact other screen readers but I haven't tested these yet.

[maribethb]

For what it's worth I've seen multiple comments on the internet/reddit/etc. from VoiceOver users who say that it saying "group" a lot as they navigate the internet is just a quirk of VO they've adapted to. Not that we shouldn't fix it if we can, but that this problem isn't necessarily because we're doing something obviously wrong

[BenHenning]

@maribethb PTAL again. I think this PR is basically as far as it can go to get #9304 in a good place.

[maribethb]

This feels weird to me but I can't totally articulate why. I guess it feels like we're relying on some rendering detail to make a decision about something that feels more like a business logic decision. But I'm not sure in which scenarios the highlight Svg wouldn't exist, so maybe that's okay.

[BenHenning]

I think that unease is valid, and I think I also share it. We compromised on how the highlight SVG is managed for focus which is why there's a tight coupling between the SVG and focus-related operations. This should increase the safety of interacting with RenderedConnection since getFocusableElement will throw if the SVG is missing since it's the focusable element. Ideally we would change the lifecycle management to always guarantee the SVG's presence and rely only on visibility rather than presence but, as I understand it, the highlight SVG is created separately from the class (due to it being created asynchronously in the rendering pipeline rather than part of much of the rest of the DOM).