feat: Add alt prop to Image component #34550

gabrieldonadel · 2022-08-31T21:47:16Z

Summary

This adds the alt prop to the Image component as requested on #34424. Using this new alt prop enables the accessibility prop and passes down the alt text to accessibilityLabel. This PR also updates RNTester ImageExample in order to facilitate the manual QA.

Open questions

~~On web alt text is displayed on the page if the image can't be loaded for some reason, should we implement this same behavior if the Image component fails to load source?~~ Not for now

Changelog

[General] [Added] - Add alt prop to Image component

Test Plan

Open the RNTester app and navigate to the Image page
Test the alt prop through the Accessibility Label via alt prop section, this can be tested either by enabling Voice Over if you're using a real device or through the Accessibility Inspector if you're using a simulator

Screen.Recording.2022-08-31.at.18.41.15.mov

analysis-bot · 2022-08-31T22:10:40Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,728,076	+509
android	hermes	armeabi-v7a	7,130,955	+502
android	hermes	x86	8,034,620	+492
android	hermes	x86_64	8,007,908	+501
android	jsc	arm64-v8a	9,597,281	+363
android	jsc	armeabi-v7a	8,363,583	+354
android	jsc	x86	9,541,506	+355
android	jsc	x86_64	10,133,905	+358

Base commit: 8cdc9e7
Branch: main

analysis-bot · 2022-08-31T23:22:13Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 8cdc9e7
Branch: main

ryancat · 2022-09-01T00:30:09Z

On web alt text is displayed on the page if the image can't be loaded for some reason, should we implement this same behavior if the Image component fails to load source?

It's tricky, but I think we should do it.

Pros:

There's no hover with mobile screens, so the alt really can only be rendered into words, instead of using a placeholder image with hovered text
Matching web spec

Cons:

Mobile screen is much smaller, and we need to avoid super long alt that breaks user experiences unexpectedly (How web handles super long alt?).

Libraries/Image/Image.android.js

necolas · 2022-09-01T01:52:30Z

At this stage, I don't think we need to display the alt text if the image doesn't load. Often on web we try to prevent broken images breaking layout by hiding fallback text anyway. This is what RNWeb does today. We can always revisit this decision in the future once people start using it and if a case is made.

necolas · 2022-09-06T18:49:37Z

Ping @cipolleschi or @jacdebug to import

gabrieldonadel · 2022-09-08T16:10:24Z

Could someone please import this? @cipolleschi @NickGerleman @jacdebug @ryancat

facebook-github-bot · 2022-09-19T12:30:23Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

lunaleaps

Can we also update the TS types?

gabrieldonadel · 2022-09-19T21:58:53Z

Can we also update the TS types?

Sure, I've just updated it

necolas · 2022-09-20T00:25:05Z

The TS types are already out of sync with the Flow types, as they don't have props from the other merged Web PRs

lunaleaps · 2022-09-20T06:26:05Z

The TS types are already out of sync with the Flow types, as they don't have props from the other merged Web PRs

I'm actively updating them to catch up (like the accessibility, id ones) and for going forward, we can keep them up to date.

Summary: While working on #34550 I noticed that a couple of words inside the RNTester package were misspelled, this gave me the idea to test other files as well using the VS Code extension [Code Spell Checker](https://marketplace.visualstudio.com/items?itemName=streetsidesoftware.code-spell-checker) which unveiled quite a few other typos. ## Changelog [Internal] [Fixed] - Fix RNTester typos Pull Request resolved: #34757 Test Plan: Shouldn't require much testing as this is just fixing some typos inside the RNTester package. Reviewed By: dmytrorykun Differential Revision: D39722889 Pulled By: cortinico fbshipit-source-id: a575ab8337586c5fe2d68ce73d2aae27d24a6384

cipolleschi · 2022-09-22T11:04:07Z

Hi @gabrieldonadel could you please fix the linting warning? After that I'll reimport this. ;)

gabrieldonadel · 2022-09-22T11:53:10Z

Hi @gabrieldonadel could you please fix the linting warning? After that I'll reimport this. ;)

Sure thing, you're talking about these lint warnings that the github-actions bot commented, right?

gabrieldonadel · 2022-09-22T18:31:35Z

Hi @gabrieldonadel could you please fix the linting warning? After that I'll reimport this. ;)

@cipolleschi I've pushed a commit fixing all warnings

facebook-github-bot · 2022-09-23T13:11:35Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-09-26T09:12:12Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2022-09-27T09:34:15Z

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

react-native-bot · 2022-09-27T11:10:04Z

This pull request was successfully merged by @gabrieldonadel in 71fda5e.

^{When will my fix make it into a release? | Upcoming Releases}

Summary: While working on facebook#34550 I noticed that a couple of words inside the RNTester package were misspelled, this gave me the idea to test other files as well using the VS Code extension [Code Spell Checker](https://marketplace.visualstudio.com/items?itemName=streetsidesoftware.code-spell-checker) which unveiled quite a few other typos. ## Changelog [Internal] [Fixed] - Fix RNTester typos Pull Request resolved: facebook#34757 Test Plan: Shouldn't require much testing as this is just fixing some typos inside the RNTester package. Reviewed By: dmytrorykun Differential Revision: D39722889 Pulled By: cortinico fbshipit-source-id: a575ab8337586c5fe2d68ce73d2aae27d24a6384

Summary: This adds the `alt` prop to the `Image` component as requested on facebook#34424. Using this new `alt` prop enables the `accessibility` prop and passes down the alt text to `accessibilityLabel`. This PR also updates RNTester ImageExample in order to facilitate the manual QA. #### Open questions - ~~On web `alt` text is displayed on the page if the image can't be loaded for some reason, should we implement this same behavior if the `Image` component fails to load `source`?~~ Not for now ## Changelog [General] [Added] - Add alt prop to Image component Pull Request resolved: facebook#34550 Test Plan: 1. Open the RNTester app and navigate to the Image page 2. Test the `alt` prop through the `Accessibility Label via alt prop` section, this can be tested either by enabling Voice Over if you're using a real device or through the Accessibility Inspector if you're using a simulator https://user-images.githubusercontent.com/11707729/187790249-0d851363-c30e-41b6-8c24-73e72467f4ba.mov Reviewed By: lunaleaps Differential Revision: D39618453 Pulled By: cipolleschi fbshipit-source-id: 0e26b2574514e76ce7e98ddb578f587a9cc30ee9

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 31, 2022

react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Aug 31, 2022

ryancat reviewed Sep 1, 2022

View reviewed changes

Libraries/Image/Image.android.js Outdated Show resolved Hide resolved

gabrieldonadel requested a review from ryancat September 1, 2022 01:56

necolas mentioned this pull request Sep 1, 2022

[w3c] ☂️ Web Props (Part 1) umbrella issue #34424

Closed

70 tasks

gabrieldonadel requested review from necolas and removed request for ryancat September 2, 2022 02:10

gabrieldonadel force-pushed the feat/add-alt-to-image branch from bbd6b3e to d6e24d9 Compare September 5, 2022 17:25

gabrieldonadel requested review from ryancat and removed request for necolas September 5, 2022 18:41

gabrieldonadel force-pushed the feat/add-alt-to-image branch 2 times, most recently from 34db210 to 2490538 Compare September 8, 2022 15:40

gabrieldonadel force-pushed the feat/add-alt-to-image branch 2 times, most recently from 53027c8 to f551198 Compare September 9, 2022 19:02

gabrieldonadel requested review from necolas and removed request for ryancat September 16, 2022 16:19

necolas approved these changes Sep 16, 2022

View reviewed changes

lunaleaps requested changes Sep 19, 2022

View reviewed changes

gabrieldonadel force-pushed the feat/add-alt-to-image branch from f551198 to 44da077 Compare September 19, 2022 21:56

gabrieldonadel requested a review from lunaleaps September 19, 2022 21:59

lunaleaps approved these changes Sep 19, 2022

View reviewed changes

gabrieldonadel mentioned this pull request Sep 21, 2022

chore: Fix RNTester typos #34757

Closed

cipolleschi approved these changes Sep 26, 2022

View reviewed changes

gabrieldonadel added 5 commits September 26, 2022 16:28

feat: Add alt prop to Image component

71920d0

chore: Update Accessibility Label ImageExample

3fb3624

chore: Invert accessibilityLabel precedence

53c31dd

chore: Add alt prop to TS definition

0c3db7a

fix: Eslint warnings inside ImageExample

989d8d8

gabrieldonadel force-pushed the feat/add-alt-to-image branch from c467683 to 989d8d8 Compare September 26, 2022 19:30

facebook-github-bot closed this in 71fda5e Sep 27, 2022

react-native-bot added the Merged This PR has been merged. label Sep 27, 2022

gabrieldonadel deleted the feat/add-alt-to-image branch September 27, 2022 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add alt prop to Image component #34550

feat: Add alt prop to Image component #34550

gabrieldonadel commented Aug 31, 2022 •

edited

Loading

analysis-bot commented Aug 31, 2022 •

edited

Loading

analysis-bot commented Aug 31, 2022 •

edited

Loading

ryancat commented Sep 1, 2022

necolas commented Sep 1, 2022

necolas commented Sep 6, 2022

gabrieldonadel commented Sep 8, 2022

facebook-github-bot commented Sep 19, 2022

lunaleaps left a comment

gabrieldonadel commented Sep 19, 2022

necolas commented Sep 20, 2022

lunaleaps commented Sep 20, 2022 •

edited

Loading

cipolleschi commented Sep 22, 2022

gabrieldonadel commented Sep 22, 2022

gabrieldonadel commented Sep 22, 2022

facebook-github-bot commented Sep 23, 2022

facebook-github-bot commented Sep 26, 2022

facebook-github-bot commented Sep 27, 2022

react-native-bot commented Sep 27, 2022

feat: Add alt prop to Image component #34550

feat: Add alt prop to Image component #34550

Conversation

gabrieldonadel commented Aug 31, 2022 • edited Loading

Summary

Open questions

Changelog

Test Plan

analysis-bot commented Aug 31, 2022 • edited Loading

analysis-bot commented Aug 31, 2022 • edited Loading

ryancat commented Sep 1, 2022

necolas commented Sep 1, 2022

necolas commented Sep 6, 2022

gabrieldonadel commented Sep 8, 2022

facebook-github-bot commented Sep 19, 2022

lunaleaps left a comment

Choose a reason for hiding this comment

gabrieldonadel commented Sep 19, 2022

necolas commented Sep 20, 2022

lunaleaps commented Sep 20, 2022 • edited Loading

cipolleschi commented Sep 22, 2022

gabrieldonadel commented Sep 22, 2022

gabrieldonadel commented Sep 22, 2022

facebook-github-bot commented Sep 23, 2022

facebook-github-bot commented Sep 26, 2022

facebook-github-bot commented Sep 27, 2022

react-native-bot commented Sep 27, 2022

gabrieldonadel commented Aug 31, 2022 •

edited

Loading

analysis-bot commented Aug 31, 2022 •

edited

Loading

analysis-bot commented Aug 31, 2022 •

edited

Loading

lunaleaps commented Sep 20, 2022 •

edited

Loading