-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Android 13 media permissions #18183
Android 13 media permissions #18183
Conversation
WRITE_EXTERNAL_STORAGE is not required for SDK higher than 29.
For devices with API 33 and higher, we must request one or more of the granular media permissions instead of the READ_EXTERNAL_STORAGE permission.
For devices with API 33 and higher, we must request one or more of the granular media permissions instead of the READ_EXTERNAL_STORAGE permission.
📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
|
👋 @irfano - Nice work! All the steps are working, except Take a Photo or Video is asking for Storage permission, and that leads to nowhere in system settings where it can be enabled! Looked through System Settings -> Apps and through Privacy -> Permissions Manager. Happening on both JP and WP. |
Seems the Storage permission is asked due to the logic when taking a photo from the editor: Lines 284 to 287 in 8a07a96
Lines 774 to 775 in 8a07a96
It relies on @irfano @ravishanker Maybe a potential workaround would be to apply a similar approach like this one on the WordPress-Android/WordPress/src/main/java/org/wordpress/android/ui/media/MediaBrowserActivity.java Lines 1006 to 1012 in bb26d70
|
Thank you so much for the help @fluiddot! I thought the change was required on gutenberg-mobile but it was on Utils library as you said. |
@ravishanker, I updated the Utils library, and the camera permission issue should be fixed now. |
Yes, working now. Published a new version of Utils 3.5.0, please update it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@irfano 👋
Just leaving a note that mobile-ui-testing-squad
was pinged for a review because the PR removes the write permission from two UI tests-related file. This does not seem to affect the execution negatively, so from our side, it's good to go as well 🙂
Hey @irfano Nice work. 🙌🏼 . I have tested the app on Android 12, Pixel, and Android 13, Samsung. While the positive scenario(all permissions granted) works in all the cases on both the devices, I found a behavior in Android 13, Samsung. I am not sure if it's right. More details below I haven't worked on the permissions for some time, so I am not sure what the issue is or whether this is to be expected. |
🤔 Thank you for testing, @AjeshRPai! Can you see photos even though permission for the "Photos & videos" is not allowed? I tested it on both the emulator and the Android 13 Samsung device but couldn't reproduce your case. Can you write me testing instructions to reproduce it? |
Hey Irfan 👋🏼 I can see some photos even when the permission Steps to reproduce
Here the
@irfano I will spend some additional time checking whether I can reproduce the scenario I have described here . I will make sure to let you know with a comment. |
@@ -192,7 +187,8 @@ class MediaPickerLauncher @Inject constructor( | |||
primaryDataSource = DEVICE, | |||
availableDataSources = setOf(), | |||
canMultiselect = canMultiselect, | |||
requiresStoragePermissions = true, | |||
requiresPhotosVideosPermissions = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓I believe this is the function that's being called when you navigate from Media → + Icon → Choose from Device. Is the requiresPhotosVideosPermissions
having correct value here?.
I did a quick run and test after changing this requiresPhotosVideosPermissions
to true
. I was able to see the photos in the Scenario
- Allowed Photos and Video & Dont allow Audio and Music - I can see the photos and videos
- Dont allow Photos and Video & Allow Audio and Music - I can't see any photos and video, Permission message is shown
- Dont allow both permissions - Permission message is shown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for testing and finding a critical problem with the implementation, @AjeshRPai!
I added the support for "Image picker" or "Audio picker" but not for "Image + video + audio picker".
Now, I have added all media support to the media picker: ac21e3c.
I have tested the app again, I can reproduce this issue. ✅ I have ensured that I am on the correct branch, and the Android version is 13. 📹 Please find below the video capture of this issue. WhatsApp.Video.2023-03-31.at.12.53.58.PM.mp4I looked into this issue, and I think I found the cause of the issue and left a suggestion here. I didn't commit the change. You can verify and make the change yourself. Feel free to let me know if you need anything else from me. |
Warning Message: "WRITE_EXTERNAL_STORAGE no longer provides write access when targeting Android 11+, even when using 'requestLegacyExternalStorage'" Explanation: "Scoped storage is enforced on Android 10+ (or Android 11+ if using requestLegacyExternalStorage). In particular, WRITE_EXTERNAL_STORAGE will no longer provide write access to all files; it will provide the equivalent of READ_EXTERNAL_STORAGE instead." ------------------------------------------------------------------------ This warning is already addressed and will be fixed as part of the ongoing Android 13 media permissions work (see PR below): PR: #18183
…13)' into android-13-media-permissions # Conflicts: # build.gradle
I discovered another issue: the 'Story post' screen was not functioning properly. |
Hey there! 👋 Looking at this PR, I've been wondering why WPAndroid has its own version of the Media Picker instead of the using the library, which was designed to be customizable and reusable across different apps. Is there anything specific that the library doesn't have or why it can't be integrated? |
The reason for not using the MediaPicker library previously was that it required Hilt, which was not available when I attempted to add the Media Picker library to WPAndroid. Now we have Hilt on WPAndroid, and we can add the library. I'll create an issue. Thanks for bringing it up, @0nko. Btw, I recall we discussed this exact topic before. 😅 (Internal ref: p1637924023002400-slack-C02814L5EGP) |
Good memory, it was 2.5 years ago. I completely forgot 😆 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @irfano
I have tested the app for the Media permissions, everything looks good to me 👍🏼
I have tested all the features by enabling/disabling permissions, and everything works as expected. I was able to select and upload photos and video when given permission and I was shown the message to allow permissions when I disabled the permissions.
The following features works as expected.
- Media picker ✅
- Media browser ✅
- Story ✅
Thanks for working on this. I have left a Nitpick for you to address. Feel free to merge it once you had a look at it. I am approving but not merging so that you can have a look at the Nitpick I have left.
@@ -2850,6 +2854,10 @@ | |||
<string name="permissions_denied_title">Permissions</string> | |||
<string name="permissions_denied_message">It looks like you turned off permissions required for this feature.<br/><br/>To change this, edit your permissions and make sure <strong>%s</strong> is enabled.</string> | |||
<string name="permission_storage">Storage</string> | |||
<string name="permission_images">Photos and videos</string> | |||
<string name="permission_video">Photos and videos</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nip 🔍 : cant we reference the pemission_images
string here?. Like
<string name="permission_video" translatable="false">@string/permission_images </string>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the idea. I've done it: 21f9975
Generated by 🚫 dangerJS |
I have also updated the RELEASE-NOTES. |
a76ef47
into
Parent-PR-for-updating-targetSdkVersion-to-33-(Android-13)
Warning Message: "WRITE_EXTERNAL_STORAGE no longer provides write access when targeting Android 11+, even when using 'requestLegacyExternalStorage'" Explanation: "Scoped storage is enforced on Android 10+ (or Android 11+ if using requestLegacyExternalStorage). In particular, WRITE_EXTERNAL_STORAGE will no longer provide write access to all files; it will provide the equivalent of READ_EXTERNAL_STORAGE instead." ------------------------------------------------------------------------ This warning is already addressed and will be fixed as part of the ongoing Android 13 media permissions work (see PR below): PR: #18183
This updates media permissions for Android 13 compatibility.
Granular media permissions
READ_EXTERNAL_STORAGE
is used for accessing photos, videos, and audio files. After upgradingtargetSdk
to 33, we need to use specific permissions,READ_MEDIA_IMAGES
,READ_MEDIA_VIDEO
, andREAD_MEDIA_AUDIO
.>=API 33
. READ_EXTERNAL_STORAGE is still used for older APIs.READ_MEDIA_IMAGES
andREAD_MEDIA_VIDEO
are different permissions on the code, but they appear as single permission, "Photos and videos". So, I requested bothREAD_MEDIA_IMAGES
andREAD_MEDIA_VIDEO
even though just photos are needed.Storage and Storage
→Photos and videos
)To test:
Please perform all tests using a device with API 33 or higher and a device with an API version lower than 33.
Media Picker - Image - Choose from device
Media Picker - Image - Other options
Media Picker - Video
Repeat Media Picker - Image tests with the video.
Media Picker - Audio
Repeat Media Picker - Image tests with the audio.
Media Browser
Story
Regression Notes
Potential unintended areas of impact
All screens that need storage permission.
What I did to test those areas of impact (or what existing automated tests I relied on)
Tested all cases manually.
What automated tests I added (or what prevented me from doing so)
Updated existing tests.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.