flutter_svg
flutter_svg copied to clipboard
dnfield
Do not require new Semantics node when setting excludeFromSemantics false
Currently when excludeFromSemantics: false is set, the SvgPicture widget is unconditionally wrapped in a Semantics container that has its container property set to true, which causes it to necessarily create a new node in the semantics tree, which is not always desirable. https://github.com/dnfield/flutter_svg/blob/f1fff6b22fc0edb710ec79faef30ec8bc31449fd/lib/svg.dart#L911-L918
It would be nice if either the container property is not enforced at all, or if we could have a separate parameter to control whether the container property is true or not.
Could you explain a bit more about why?
Of course, one option that gives you full control is to just exclude the SvgPicture from semantics and wrap it in your own semantics node with whatever properties you want :)
An image with a label has no implicit need or reason to be a container. For our app, we almost never want it to be treated as a container. If the widget supports specifying a semanticLabel, IMO it would make more sense to never specify the container, but at the very least you should make this behavior configurable like Card does. Though do note that Card defaults the value to true, which makes sense because a Card is usually a logical semantic container.
Wrapping it in our own Semantics is okay, and it's what we're doing in the short term, but it's inconvenient that functionality of the widget doesn't work for our use case. I would be willing to bet that many other apps have the same issue. It's also confusing and error-prone because we need to remember not to use the SvgPicture's semantic label.
The behavior here mirrors identically that of the Image widget in the Flutter repository.
I think the reason to make it a container is so that if it has a label that label does not get subsumed into other nearby text for a screen reader. But it really depends on how your app is using images. IMHO, if it's not a container, you probably don't want it to have its own label and instead want that label to be on whatever the container is.
In other words, if your image is semantically relevant and needs a label, it likely needs to be a container as well so that assistive technology will read it as its own item. If it is not meant to be treated as a separate container, it's likely that you don't want it to have its own label either and that it's just decorative - in which case semantics label should be null and/or exclude it from semantics.
In cases where something else is needed, it's perfectly fine to tell the SvgPicture to not build the semantics node and build your own around it.
(and FWIW, changing this here would likely break 100s of tests internal to Google although I'd have to do a test run to be sure of that.)
And if this is specific to your app, it may be worth writing a MyAppSvg that does the wrapping the way you'd expect for your app's context.
Of course, one option that gives you full control is to just exclude the SvgPicture from semantics and wrap it in your own semantics node with whatever properties you want :)
Yes, indeed this is our current solution, and is functional, we were just interested in perhaps if that middle wrapper layer could be cleaned up by having the configurability built-in to the svg widget.
In case you were still interested in an example, one use case we ran into was where we need to display a 'battery icon' along with the battery percentage as text next to it.
In this case the image is not purely decorative, as nothing else indicates the percentage is battery related. However, as you said, it's also perfectly reasonable to exclude the icon from semantics anyway and put the "battery" label as a custom semantic label on the text element instead. It just seemed a bit more straightforward to set the semantic label on the svg instead since it has that parameter anyway, but that resulted in undesirable side effects where everything in the tile got merged and read together as a single Semantic node except the svg image label, which awkwardly was on its own. (ie, the screenreader would focus and read "Samsung J327P, 55%, Last online: Yesterday" and then you would have to swipe to the next item to hear "Battery", instead of the expected: "Samsung J327P, Battery: 55%, Last online: Yesterday").
And if we wrapped the entire thing with another Semantics widget with container: true +explicitChildNodes: true in order to get the Battery icon to be merged in to the group - that resulted in other random and undesirable side effects like the button property getting ignored on the wrapped Semantics widget. It's not really clear what the expected behavior/best practices in assembling merged semantics is/I haven't seen any official guidance about it, but that behavior did seem off to us, and the straightforward solution was stripping the container property from the battery icon svg.
Regarding breaking existing behavior and tests - certainly a problem if the container property is removed entirely. Could potentially be circumvented by the introduction of a separate semanticContainer property that is default true, which is what Card does, as mentioned above, as one option. As you can see from our example though, there are numerous other approaches that might result in the same outcome we want, this was more of a feature request where we thought the default behavior was too prescriptive with potentially undesirable side-effects and would be a QoL update to make more flexible.
After thinking about this more, I think Dan is right. The solution is to not apply a semantic label to the battery icon, but instead apply a label to the 55% text. Instead of labeling the battery Battery so that it reads concatenated Battery 55%, we should just label 55% as Battery 55%. This is also likely to be better localization because it does not implicitly rely on concatenation.