-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Build Accessibility Tree from scene #12074
Conversation
Woot! I love it!! cc @PirateJC and @PatrickRyanMS for awareness Only thing to consider is that we are in code lock for 5.0 but this is totally 6.0 material |
+1 for AWESOME FEATURE! I'll love to see it in next release 😄 |
I love it a lot !!!! the main issue I am seeing is that it requires the full inspector which will turn out to force loading all of Babylon creating a huge dependency. I bet @RaananW can help with this part as this component should be easily embeddable without pulling all of the framework. |
Sorry, didn't find time for review today, I'll go over this tomorrow. |
No rush, we will not merge until after the release |
I guess we can think about continuing this thread |
…ects, and 2) (partially completed) make DOM nodes interactive for actionable nodes.
…hange (node add/remove, enable/disable, control add/remove, visible/invisible)
009c674
to
d3c406d
Compare
Please make sure to tag your PR with "bug", "new feature" or "breaking change" tags. |
Reviewer - this PR has made changes to one or more package.json files. |
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12074/merge/index.html#WGZLGJ#4600 To test the snapshot in the playground itself use (for example): https://playground.babylonjs.com/?snapshot=refs/pull/12074/merge#BCU1XR#0 |
I updated it so it's now in its own package, and it support GUI (TextBlock and Button only for now). It can now also update when scene updates (add/remove node and GUI control, enable/disable node, visible/invisible GUI control). I think it's in a good shape to be reviewed :) @RaananW, @sebavan, @deltakosh |
Please bear with us as @RaananW is OOF and I'm expecting him to give us the green light :) |
(Answering David's question about why changing core and then have another package for accessibility)
Good question! The idea is that the core can allow tagging accessibility metadata to nodes, that developers can assign this metadata and later use this metadata for customized accessibility. And then there's an accessibility package, specifically providing methods to render accessibility DOM for a scene. Right now, it just renders every accessibility object in the scene to DOM, as if we are flattening a 3D scene into a readable 2D document, even some objects might be behind the camera. This will work great for more static style applications like gallery, information display, but won't work that much for applications where the user have to move around, like multi-user meeting app, because it loses the sense of spatial. Later we can extend the package so it can provide another rendering method that represents the user's view when the user moves, e.g. it can render DOM for objects that's inside the current camera view. But with the accessibility tag (change in the core), the developer should be able to customize their own renderer already. Does that make sense? I wonder whether we should change the package to AccessibilityHelper or AccessibilityRenderer to reduce confusion?
…On Thu, 21 Apr 2022 at 16:19, David Catuhe ***@***.***> wrote:
Some questions on my side:
- Why using GUI? I believe the goal is to generate DOM elements right?
- DO you see this as part of the framework ? (I see it is decorrelated
here). THen it will be an independent NPM package?
—
Reply to this email directly, view it on GitHub
<#12074 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABJXMNVW5F26UAMSRJP6RB3VGHO6LANCNFSM5PV2POOA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
…ad of 'onControlAddedOrRemovedObservable'
Please make sure to tag your PR with "bug", "new feature" or "breaking change" tags. |
Reviewer - this PR has made changes to one or more package.json files. |
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12074/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/12074/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/12074/merge#BCU1XR#0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is awesome!!! Can't wait to have it as part of the framework :-)
I have a few general questions/suggestions (and will be happy to discuss them all)
- I think the better place for this would be the
tools
directory and not thedev
directory, as this is a tool using the core libs. - The role definition of the component holding the data can be a bit confusing. Both Control and Node should have the accessibility tag added to them, allowing the dev to extend the corresponding (HTML) elements. Those tags can be cached and updated in the React implementation when changed. I wrote in one of the comments that I think the cache and "changed" functionality should be a part of the node/control (or maybe even its own "accessibility class" that will take care of that for both classes as part of core?). This way the dev could change the a11y data when the app is already running and will always see the changes propagate to the react implementation (or any other implementation that will use this API).
- You are using a lot of instanceof, which can be a little unreliable (and a bit slow, though the differences are not that extreme). If possible, try using feature-detection or use the getClassName function (if possible, of course).
A bit philosophical, but I think that, in general, screen readers don't know what 3D content is. They understand 2D very well, and this is a kind of 3D-to-2D converter. My question here is whether the order (and "level") should be controlled by something other than the tree-structure? it makes sense in GUI (which is "2D" in its nature), but might not fit the node implementation. TBH - I don't have a good answer as to what is better here, but will be happy to talk about it :-)
And in general - I believe the React implementation should use the provided observables in a more dynamic way instead of reading the entire scene structure on every change. So when a node (or control) is added, a new component should be created, and this component should attach to the corresponding node/control's observables to decide whether or not its a11y tag should be rendered. In this case it would also make sense to have a "onA11YTagUpdatedObservable` (and maybe added/removed as well? though "changed" might be enough) and attach to it. This way changes in the scene graph will automatically propagate to the react implementation in a more performant way (instead of checking all nodes for changes on each frame).
Reviewer - this PR has made changes to one or more package.json files. |
1 similar comment
Reviewer - this PR has made changes to one or more package.json files. |
Reviewer - this PR has made changes to one or more package.json files. |
1 similar comment
Reviewer - this PR has made changes to one or more package.json files. |
Visualization tests for webgl1 have failed. If some tests failed because the snapshots do not match, the report can be found at If tests were successful afterwards, this report might not be available anymore. |
Is it now ready for final review and merge? |
# Conflicts: # package.json
Reviewer - this PR has made changes to one or more package.json files. |
Reviewer - this PR has made changes to one or more package.json files. |
Visualization tests for webgl1 have failed. If some tests failed because the snapshots do not match, the report can be found at If tests were successful afterwards, this report might not be available anymore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
❤️🔥 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Readme and examples look great!
Visualization tests for webgl1 have failed. If some tests failed because the snapshots do not match, the report can be found at If tests were successful afterwards, this report might not be available anymore. |
1 similar comment
Visualization tests for webgl1 have failed. If some tests failed because the snapshots do not match, the report can be found at If tests were successful afterwards, this report might not be available anymore. |
Reviewer - this PR has made changes to one or more package.json files. |
Reviewer - this PR has made changes to one or more package.json files. |
Reviewer - this PR has made changes to one or more package.json files. |
* Build Accessibility Tree from scene: 1) build DOM tree for static objects, and 2) (partially completed) make DOM nodes interactive for actionable nodes. * Rename classes. (Remake) * Enable HTML Twin building for GUI; enable dynamic update when scene change (node add/remove, enable/disable, control add/remove, visible/invisible) * Create package for accessibility. * Fix accessibility tree update for parent enable/disable. * Use 'onControlRemovedObservable' and 'onControlAddedObservable' instead of 'onControlAddedOrRemovedObservable' * Move AccessibilityTag to it's own file, and add onAccessibilityTagChangedObservable . * Make accessibilityTag nullable. * Move package from dev to tools. * Remove isSalient in IAccessibilityTag. * Add aria to IAccessibilityTag to allow override rendering accessibility behavior. * Add README.md * Add types restriction! * Adjust renderer to take customized aria and event handler. * Add ES6 and UMD packages. # Conflicts: # package-lock.json * Fix build error. * Fix build error: remove redundant blank space. * Fix build error: edit gitignore. * Fix formatting. * Fix lint error. * Refine eventHandler interface. * Refine from comment: click and right click -> triggerEvent(). * Fix comment and formating. * Add example scenes to README. * Fix comment: add pointer position to onPointerClick observer; dispose tree when scene is disposed. * Fix comments. * Fix comment * Revert package-lock.json * Revert package-lock.json again * Fix some comments. Rename packages and classes. * Fix formatting * Fix README typos and fix formatting. * Render html twin inside canvas. * Some README typo fix that I missed two commits ago. * [trivial] revert change. * Revert package-lock.json, remove redundant comments. * Make package-lock.json same as master branch. * Update package-lock.json and format fix. Co-authored-by: Sunny Zhang <[email protected]> Former-commit-id: dd2a04ac2372bb960af597627202b6a5ef8d6d95
(A draft PR hoping to get early feedback.)
@RaananW @Exolun
Testing:
Testing code 1:
Given a scene that has objects that cover conditions of:
Testing code 2: