fix: Add default aria role to svg and group elements#9697
fix: Add default aria role to svg and group elements#9697lizschwab merged 2 commits intoRaspberryPiFoundation:v13from
Conversation
gonfunko
left a comment
gonfunko
left a comment
There was a problem hiding this comment.
Just a couple minor nits but looks good, thanks!
packages/blockly/core/utils/dom.ts
Outdated
| * For svg and group (g) elements, we set the role to generic so that they are ignored by assistive technologies. | ||
| */ | ||
| if ( | ||
| name === 'svg' || |
There was a problem hiding this comment.
Rather than string literals, it might be preferable to use Svg.G.tagName and Svg.SVG.tagName (from utils/svg.ts)
| }); | ||
| test('svg elements of type svg have the generic role by default', function () { | ||
| const svgSvg = Blockly.utils.dom.createSvgElement( | ||
| Blockly.utils.Svg.G, |
There was a problem hiding this comment.
This should be Blockly.utils.Svg.SVG I think?
There was a problem hiding this comment.
Yep, thanks for catching that!
|
|
||
| import type {Svg} from './svg.js'; | ||
| import * as aria from './aria.js'; | ||
| import {Svg} from './svg.js'; |
There was a problem hiding this comment.
In order to use Svg as a value below I needed to change the import statement. This does not appear to affect anything, but I wanted to highlight it as I'm unsure of the reasons we were previously importing it as a type.
There was a problem hiding this comment.
Generally we just import the type alone if it's being used strictly for TS typings, e.g. never instantiated/having any fields accessed. It's fine to import the full thing when we are actually using it though.
| * For svg and group (g) elements, we set the role to generic so that they are ignored by assistive technologies. | ||
| */ | ||
| if ( | ||
| name === Svg.SVG.toString() || |
There was a problem hiding this comment.
Svg.SVG.tagName is private, but toString() returns the tagName.
There was a problem hiding this comment.
Ah good point, but yes this is fine, thanks!
|
|
||
| import type {Svg} from './svg.js'; | ||
| import * as aria from './aria.js'; | ||
| import {Svg} from './svg.js'; |
There was a problem hiding this comment.
Generally we just import the type alone if it's being used strictly for TS typings, e.g. never instantiated/having any fields accessed. It's fine to import the full thing when we are actually using it though.
| * For svg and group (g) elements, we set the role to generic so that they are ignored by assistive technologies. | ||
| */ | ||
| if ( | ||
| name === Svg.SVG.toString() || |
There was a problem hiding this comment.
Ah good point, but yes this is fine, thanks!
2 participants
The basics
The details
Resolves
Fixes #9664
Proposed Changes
Adds
role=genericto svg elements of type g and type svgReason for Changes
This change ensures that browsers and screen readers do not attempt to associate additional semantic information with these elements.
Test Coverage
Added unit tests which cover the following scenarios: