## The basics
- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)
## 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: https://github.com/RaspberryPiFoundation/blockly/pull/9449#issuecomment-3663234767. 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.
## The basics
- [x] I [validated my changes](https://developers.google.com/blockly/guides/contribute/core#making_and_verifying_a_change)
## The details
### Resolves
Fixes part of #8207
Fixes part of #3370
### Proposed Changes
This introduces initial broad ARIA integration in order to enable at least basic screen reader support when using keyboard navigation.
Largely this involves introducing ARIA roles and labels in a bunch of places, sometimes done in a way to override normal built-in behaviors of the accessibility node tree in order to get a richer first-class output for Blockly (such as for blocks and workspaces).
### Reason for Changes
ARIA is the fundamental basis for configuring how focusable nodes in Blockly are represented to the user when using a screen reader. As such, all focusable nodes requires labels and roles in order to correctly communicate their contexts.
The specific approach taken in this PR is to simply add labels and roles to all nodes where obvious with some extra work done for `WorkspaceSvg` and `BlockSvg` in order to represent blocks as a tree (since that seems to be the best fitting ARIA role per those available: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Reference/Roles). The custom work specifically for blocks includes:
- Overriding the role description to be 'block' rather than 'tree item' (which is the default).
- Overriding the position, level, and number of sibling counts since those are normally determined based on the DOM tree and blocks are not laid out in the tree the same way they are visually or logically (so these computations were incorrect). This is also the reason for a bunch of extra computation logic being introduced.
One note on some of the labels being nonsensical (e.g. 'DoNotOverride?'): this was done intentionally to try and ensure _all_ focusable nodes (that can be focused) have labels, even when the specifics of what that label should be aren't yet clear. More components had these temporary labels until testing revealed how exactly they would behave from a screen reader perspective (at which point their roles and labels were updated as needed). The temporary labels act as an indicator when navigating through the UI, and some of the nodes can't easily be reached (for reasons) and thus may never actually need a label. More work is needed in understanding both what components need labels and what those labels should be, but that will be done beyond this PR.
### Test Coverage
No tests are added to this as it's experimental and not a final implementation.
The keyboard navigation tests are failing due to a visibility expansion of `connectionCandidate` in `BlockDragStrategy`. There's no way to avoid this breakage, unfortunately. Instead, this PR will be merged and then https://github.com/google/blockly-keyboard-experimentation/pull/684 will be finalized and merged to fix it. There's some additional work that will happen both in that branch and in a later PR in core Blockly to integrate the two experimentation branches as part of #9283 so that CI passes correctly for both branches.
### Documentation
No documentation is needed at this time.
### Additional Information
This work is experimental and is meant to serve two purposes:
- Provide a foundation for testing and iterating the core screen reader experience in Blockly.
- Provide a reference point for designing a long-term solution that accounts for all requirements collected during user testing.
This code should never be merged into `develop` as it stands. Instead, it will be redesigned with maintainability, testing, and correctness in mind at a future date (see https://github.com/google/blockly-keyboard-experimentation/discussions/673).
* Setting style property to make CSP less grumpy.
"Content-Security-Policy: The page’s settings blocked the loading of a resource at inline (“style-src”)."
The 'style' property should be set as an object, not as a string, according to CSP rules.
Back-ported from Blockly Games.
* fix(build): Restore erroneously-deleted filter function
This was deleted in PR #7406 as it was mainly being used to
filter core/ vs. test/mocha/ deps into separate deps files -
but it turns out also to be used for filtering error
messages too. Oops.
* refactor(tests): Migrate advanced compilation test to ES Modules
* refactor(build): Migrate main.js to TypeScript
This turns out to be pretty straight forward, even if it would
cause crashing if one actually tried to import this module
instead of just feeding it to Closure Compiler.
* chore(build): Remove goog.declareModuleId calls
Replace goog.declareModuleId calls with a comment recording the
former module ID for posterity (or at least until we decide
how to reformat the renamings file.
* chore(tests): Delete closure/goog/*
For the moment we still need something to serve as base.js for
the benefit of closure-make-deps, so we keep a vestigial
base.js around, containing only the @provideGoog declaration.
* refactor(build): Remove vestigial base.js
By changing slightly the command line arguments to
closure-make-deps and closure-calculate-chunks the need to have
any base.js is eliminated.
* chore: Typo fix for PR #7415
The 'compareDocumentPosition' call was inherited from Closure Library, in order to work with IE 8 and earlier. Use the more modern 'contains' call instead.
This change was originally added here:
https://github.com/google/blockly/pull/1991
Remove the DOCUMENT_POSITION_CONTAINED_BY constant which is not a NodeType and should never have been part of that enum.
This change was originally added here:
https://github.com/google/blockly/pull/2736
* Reduce usage of obsolete .keyCode property.
* Rename private properties/methods which violate eslint rules.
* Use arrays of bound events rather than individual properties.
* Improve typing info.
* Also fix O(n^2) recursive performance issue in theme's getComponentStyle function.
* And replace String(...) with '${...}' (smaller, faster).
* .toString() is considered harmful.
* chore: remove radix from parseInt
Previously any number starting with '0' would be parsed as octal if the radix was left blank. But this was changed years ago. It is no longer needed to specify a radix.
* chore: 'ID' is identification
'id' is a part of Freud's brain.
* Use Unicode characters instead of codes
This is in line with the current style guide.
* Simplify Blockly.utils.dom methods.
classList add/remove/has supports SVG elements in all browsers Blockly supports (i.e. not IE).
* refactor: Remove uses of AnyDuringMigration from workspace_comment.ts
* refactor: Remove uses of AnyDuringMigration from separator.ts.
* refactor: Remove uses of AnyDuringMigration from category.ts.
* refactor: Remove uses of AnyDuringMigration from rendered_connection.ts.
* refactor: Removed uses of AnyDuringMigration from flyout_metrics_manager.ts.
* refactor: Remove uses of AnyDuringMigration from dom.ts.
* fix: stop using dom.addClass in most cases
* chore: format
* fix: remove use of dom.addClass in toolbox
* chore: lint and format
* fix: add checks around non-constant class names
* fix: switch back to quoted access
* chore: format
* fix: remove calls to removeClass
* chore: format
* chore: remove unused deps
* fix: remove uses of hasClass
* chore: format and lint
* chore: format
* chore: add linting for tsdoc
* chore: don't require types on return
* chore: remove redundant fileoverview from ts
* chore: change return to returns and add some newlines
* chore: remove license tag
* chore: don't require params/return docs
* chore: remove spurious struct tags
* Revert "chore: change return to returns and add some newlines"
This reverts commit d6d8656a45.
* chore: don't auto-add param names
* chore: disable require-param bc it breaks on this
* return to returns and add line breaks
* chore: configure additional jsdoc rules
* chore: run format
* Revert "chore: remove license tag"
This reverts commit 173455588a.
* chore: allow license tag format
* chore: only require jsdoc on exported items
* chore: add missing jsdoc or silence where needed
* chore: run format
* chore: lint fixes
* refactor: Remove uses of AnyDuringMigration from scrollbar.ts.
* refactor: Remove almost all uses of AnyDuringMigration from workspace_svg.ts.
* refactor: Remove uses of AnyDuringMigration from contextmenu_registry.ts.
* refactor: Remove uses of AnyDuringMigration from component_manager.ts.
* refactor: Use String() rather than toString().
* fix: Fix lint and build errors.
* fix: convert files to typescript
* fix: add alias for AnyDuringMigration so that tsc will run
* chore: format
* chore: enable ts for the clang-format workflow (#6233)
* chore: Restore @fileoverview comment locations (#6237)
* chore: add declareModuleId (#6238)
* fix: Revert comment change to app_controller.js (#6241)
* fix: Add missing import goog statements (#6240)
I've added the import statement immediately before the
goog.declareModuleId calls that depend on it.
There is an argument to be made that we should put the import
statement in their normal place amongst any other imports, and
move the declareModuleId statement to below the double blank
line below the imports, but as these are so tightly coupled,
replace the previous goog.module calls, and will both be deleted
at the same time once the transition to TypeScript is fully complete
I think it's fine (and certainly much easier) to do it this way.
* chore: Fix whitespace (#6243)
* fix: Remove spurious blank lines
Remove extraneous blank lines introduced by deletion of
'use strict'; pragmas.
Also fix the location of the goog.declareModuleId call in
core/utils/array.ts.
* fix: Add missing double-blank-line before body of modules
Our convention is to have two blank lines between the imports (or
module ID, if there are no imports) and the beginning of the body
of the module. Enforce this.
* fix: one addition format error for PR #6243
* fix(build): Skip npm prepare when running in CI (#6244)
Have npm prepare do nothing when running in CI.
We don't need to do any building, because npm test will build
everything needed in the workflows in which it is run, and we
don't want to build anything in other workflows because a tsc
error would prevent those workflows from completing.
* fix: re-add `@package` annotations as `@internal` annotations (#6232)
* fix: add ~70% of internal attributes
* fix: work on manually adding more @internal annotations
* fix: add more manual internal annotations
* fix: rename package typos to internal
* fix: final manual fixes for internal annotations
* chore: format
* chore: make unnecessary multiline jsdoc a single line
* fix: fix internal tags in serialization exceptions
* fix: tsc errors picked up from develop (#6224)
* fix: relative path for deprecation utils
* fix: checking if properties exist in svg_math
* fix: set all timeout PIDs to AnyDuringMigration
* fix: make nullability errors explicity in block drag surface
* fix: make null check in events_block_change explicit
* fix: make getEventWorkspace_ internal so we can access it from CommentCreateDeleteHelper
* fix: rename DIV -> containerDiv in tooltip
* fix: ignore backwards compat check in category
* fix: set block styles to AnyDuringMigration
* fix: type typo in KeyboardShortcut
* fix: constants name in row measurables
* fix: typecast in mutator
* fix: populateProcedures type of flattened array
* fix: ignore errors related to workspace comment deserialization
* chore: format files
* fix: renaming imports missing file extensions
* fix: remove check for sound.play
* fix: temporarily remove bad requireType.
All `export type` statements are stripped when tsc is run. This means
that when we attempt to require BlockDefinition from the block files, we
get an error because it does not exist.
We decided to temporarily remove the require, because this will no
longer be a problem when we conver the blocks to typescript, and
everything gets compiled together.
* fix: bad jsdoc in array
* fix: silence missing property errors
Closure was complaining about inexistant properties, but they actually
do exist, they're just not being transpiled by tsc in a way that closure
understands.
I.E. if things are initialized in a function called by the constructor,
rather than in a class field or in the custructor itself, closure would
error.
It would also error on enums, because they are transpiled to a weird
IIFE.
* fix: context menu action handler not knowing the type of this.
this: TypeX information gets stripped when tsc is run, so closure could
not know that this was not global. Fixed this by reorganizing to use the
option object directly instead of passing it to onAction to be bound to
this.
* fix: readd getDeveloperVars checks (should not be part of migration)
This was found because ALL_DEVELOPER_VARS_WARNINGS_BY_BLOCK_TYPE was no
longer being accessed.
* fix: silence closure errors about overriding supertype props
We propertly define the overrides in typescript, but these get removed
from the compiled output, so closure doesn't know they exist.
* fix: silence globalThis errors
this: TypeX annotations get stripped from the compiled output, so
closure can't know that we're accessing the correct things. However,
typescript makes sure that this always has the correct properties, so
silencing this should be fine.
* fix: bad jsdoc name
* chore: attempt compiling with blockly.js
* fix: attempt moving the import statement above the namespace line
* chore: add todo comments to block def files
* chore: remove todo from context menu
* chore: add comments abotu disabled errors
* chore: move comments back to their correct positions (#6249)
* fix: work on fixing comments
* chore: finish moving all comments
* chore: format
* chore: move some other messed up comments
* chore: format
* fix: Correct enum formatting, use merged `namespace`s for types that are class static members (#6246)
* fix: formatting of enum KeyCodes
* fix: Use merged namespace for ContextMenuRegistry static types
- Create a namespace to be merged with the ContextMenuRegistry
class containing the types that were formerly declared as static
properties on that class.
- Use type aliases to export them individually as well, for
compatibility with the changes made by MigranTS (and/or
@gonfunko) to how other modules in core/ now import these
types.
- Update renamings.json5 to reflect the availability of the
direct exports for modules that import this module directly
(though they are not available to, and will not be used by,
code that imports only via blockly.js/blockly.ts.)
* fix: Use merged namespace for Input.Align
- Create a merged namespace for the Input.Align enum.
- Use type/const aliases to export it as Input too.
- Update renamings.json5 to reflect the availability of the
direct export.
* fix: Use merged namespace for Names.NameType
- Create a merged namespace for the Names.NameType enum.
- Use type/const aliases to export it as NameType too.
- Update renamings.json5 to reflect the availability of the
direct export. (This ought to have happened in an earlier
version as it was already available by both routes.)
* chore: Fix minor issues for PR #6246
- Use `Align` instead of `Input.Align` where possible.
* fix(build): Suppress irrelevant JSC_UNUSED_LOCAL_ASSIGNMENT errors
tsc generates code for merged namespaces that looks like:
(function (ClassName) {
let EnumName;
(function (EnumName) {
EnumName[EnumNameAlign["v1"] = 0] = "v1";
// etc.
})(EnumName = ClassName.EnumName || (ClassName.EnumName = {}));
})(ClassName || (ClassName = {}));
and Closure Compiler complains about the fact that the EnumName let
binding is initialised but never used. (It exists so that any other
code that was in the namespace could see the enum.)
Suppress this message, since it is not actionable and lint and/or tsc
should tell us if we have actual unused variables in our .ts files.
* chore(build): Suppress spurious warnings from closure-make-deps (#6253)
A little bit of an ugly hack, but it works: pipe stderr through
grep -v to suppress error output starting with "WARNING in".
* fix: remaining enums that weren't properly exported (#6251)
* fix: remaining enums that weren't properly exported
* chore: format
* fix: add enum value exports
* chore: format
* fix: properly export interfaces that were typedefs (#6250)
* fix: properly export interfaces that were typedefs
* fix: allowCollsion -> allowCollision
* fix: convert unconverted enums
* fix: enums that were/are instance properties
* fix: revert changes to property enums
* fix: renamed protected parameter properties (#6252)
* fix: bad protected parameter properties
* chore:format
* fix: gesture constructor
* fix: overridden properties that were renamed
* refactor: Migrate `blockly.js` to TypeScript (#6261)
* chore: Apply changes to blockly.js to blockly.ts
* fix: Build using core/blockly.ts instead of .js
Compiles and runs in compressed mode correctly!
* fix(build): Don't depend on execSync running bash (#6262)
For some reason on Github CI servers execSync uses /bin/sh, which
is (on Ubuntu) dash rather than bash, and does not understand
the pipefail option.
So remove the grep pipe on stderr and just discard all error output
at all.
This is not ideal as errors in test deps will go unreported AND
not even cause test failure, but it's not clear that it's worth
investing more time to fix this at the moment.
* chore: use `import type` where possible (#6279)
* chore: automatically change imports to import types
* chore: revert changes that actually need to be imports
* chore: format
* chore: add more import type statements based on importsNotUsedAsValues
* chore: fix tsconfig
* chore: add link to compiler issue
* fix: add type information to blockly options (#6283)
* fix: add type information to blockly options
* chore: format
* chore: remove erroneous comment
* fix: bugs revealed by getting the built output working (#6282)
* fix: types of compose and decompose in block
* fix: workspace naming in toolbox
* chore: add jsdoc
* chore: restore registry comments to better positions
* chore: pr comments'
* fix(variables): Revert inadvertent change to allDeveloperVariables (#6290)
It appears that a function call got modified incorrectly (probably
in an effort to fix a typing issue). This fix trivially reverts
the line in question to match the original JS version from develop.
This causes the generator tests to pass.
* fix: circular dependencies (#6281)
* chore: fix circular dependencies w/ static workspace funcs
* remove preserved imports that aren't currently necessary (probably)
* fix circular dependency with workspaces and block using stub
* fix dependency between variables and xml by moving function to utils
* add stub for trashcan as well
* fix line endings from rebase
* fix goog/base order
* add trashcan patch
* fix: types of compose and decompose in block
* fix: workspace naming in toolbox
* chore: add jsdoc
* chore: restore registry comments to better positions
* chore: remove implementations in goog.js
* chore: fix types of stubs
* chore: remove added AnyDuringMigration casts
* chore: remove modifications to xml and variables
* chore: format
* chore: remove event requirements in workspace comments
* chore: fix circular dependency with xml and workspace comments
* fixup remove ContextMenu import
* chore: fix dependency between mutator and workspace
* chore: break circular dependency between names and procedures
* chore: get tests to run?
* chore: pr comments'
* chore: fix stubbing field registry fromJson
* chore: fix spying on fire
* chore: fix stubbing parts of connection checker
* chore: fix stubbing dialog
* chore: fix stubbing style
* chore: fix spying on duplicate
* chore: fix stubbing variables
* chore: fix stubbing copy
* chore: fix stubbing in workspace
* chore: remove unnecessary stubs
* chore: fix formatting
* chore: fix other formatting
* chore: add backwards compatible static properties to workspace
* chore: move static type properties
* chore: move and comment stubs
* chore: add newlines at EOF
* chore: improve errors for monkey patched functions
* chore: update comment with a pointer to the doc
* chore: update comment with a pointer to the doc
* chore: format
* chore: revert changes to playground used for testing (#6292)
* chore: get mocha tests to pass. (#6291)
* chore: fix undo and empty code blocks
* chore: skip IE test
* chore: fix gesture test
* chore: fix replace message references test
* chore: fix string table interpolation
* chore: skip getById tests
* chore: fix field tests
* chore: fix console errors by making workspace nullable
* chore: format
* chore: fix definition overwrite warning
* chore: update metadata
* chore: temporarily modify the the advanced compile test
* chore: fix gestures by fixing test instead
Co-authored-by: Neil Fraser <fraser@google.com>
Co-authored-by: Christopher Allen <cpcallen+git@google.com>
