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

Tie accesskit root nodes to winit's WindowId instead of window entity #12799

Closed
wants to merge 2 commits into from

Conversation

UkoeHB
Copy link
Contributor

@UkoeHB UkoeHB commented Mar 30, 2024

Objective

  • Allow accesskit root nodes to work even if window entities change. Motivated by bevy_worldswap.
  • Improve code robustness by attaching accesskit root nodes directly to windows, instead of using window entities as a proxy.

Explanation

In order for accesskit to connect to a window, an Adapter is required. Adapters can only be created at the moment a winit window is initialized. Moreover, Adapters internally track a NodeId tied to a specific window. This means the NodeId in window adapters must be defined when windows are initialized.

Currently window adapter NodeIds are defined from the Bevy entities that are used to track each window. The entity bits are essentially used as a proxy identifier for the underlying winit window. This works just fine when there is only one Bevy world that uses a specific winit window. However, bevy_worldswap is a new, experimental, crate that allows you to swap the world that renders at runtime.

When swapping worlds, the new foreground world (i.e. the world that will render) needs to be hooked up to existing windows. This means moving window adapters between worlds. Transferring adapters is easy enough, but the problem comes when accessibility updates in the update_accessibility_nodes system. New accessibility node trees are attached to the current PrimaryWindow via the Bevy entity id. But since we swapped to a different world, windows now live on different entities, so accessibility will not work. This is a consequence of using proxy ids for window adapters instead of directly tying adapters to windows.

Solution

  • Use winit's WindowId for accesskit root nodes instead of the Bevy entity that tracks a window.

@NthTensor NthTensor added A-Windowing Platform-agnostic interface layer to run your app in A-Accessibility A problem that prevents users with disabilities from using Bevy C-Usability A simple quality-of-life change that makes Bevy easier to use C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Mar 30, 2024
Copy link
Contributor

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@NthTensor
Copy link
Contributor

I'd appreciate a more detailed justification for this change, or some discussion of the pros and cons of each approach.

@UkoeHB
Copy link
Contributor Author

UkoeHB commented Mar 30, 2024

@NthTensor added an explanation.

As far as I know this is not a breaking change, because NodeIds are internal implementation details. I could be misunderstanding it though.

@NthTensor NthTensor removed the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 30, 2024
@NthTensor
Copy link
Contributor

Fixed

@UkoeHB UkoeHB mentioned this pull request Apr 3, 2024
6 tasks
@UkoeHB
Copy link
Contributor Author

UkoeHB commented Apr 16, 2024

Closing due to won't fix conclusion from #12860.

@UkoeHB UkoeHB closed this Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Accessibility A problem that prevents users with disabilities from using Bevy A-Windowing Platform-agnostic interface layer to run your app in C-Usability A simple quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants