Add semantics label to FadeInImage. #28799
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
I signed it! |
jonahwilliams
left a comment
jonahwilliams
left a comment
There was a problem hiding this comment.
This needs a test that verifies that the correct label is used when the placeholder or regular image is shown.
| matchTextDirection: widget.matchTextDirection, | ||
| ); | ||
|
|
||
| if (widget.excludeFromSemantics) |
There was a problem hiding this comment.
nit: Include { } around block
There was a problem hiding this comment.
I prefer this usage as well, I haven't included them because it was used this way in the Image class. I will add it.
| return _isShowingPlaceholder | ||
| ? _placeholderResolver._imageInfo | ||
| : _imageResolver._imageInfo; | ||
| ? _placeholderResolver._imageInfo |
There was a problem hiding this comment.
nit: revert unrelated change
There was a problem hiding this comment.
That's true, I reverted it. 👍
|
|
||
| String get _semanticLabel { | ||
| return _isShowingPlaceholder | ||
| ? widget.placeholderSemanticLabel |
There was a problem hiding this comment.
Fixed it as well 👍
…se. Add 3 tests to show semantics label
|
@jonahwilliams All changes requested are done. |
goderbauer
added
the
framework
|
@salihgueler Can you please fix the analyzer warnings? |
|
@goderbauer Just fixed it, sorry about that :) |
|
LGTM |
|
@salihgueler in #33370 I propose that we remove the Based on a quick code search, I didn't see any usages of this property either. Do you have any objections to this removal? If so, please comment in #33370 Thanks! |
This updates FadeInImage to use the new Image.frameBuilder API (added in #33369), to greatly simplify the implementation of FadeInImage. This also removes the FadeInImage.placeholderSemanticLabel property. This property was added in #28799 for the sake of completeness (the bug it fixed was the lack of any semantic label support in FadeInImage), but a placeholder is a transient visual artifact, not something that affects the underlying semantic meaning of the image.
5 participants
This PR is adding semantics label and Semantics to FadeInImage. It has two semantics label one for placeholder and another for the real image. It brings the semantics label according to the image shown on screen. I got the idea to add different semantics for placeholder and image from @jonahwilliams on the related issue.
This fixes #26040