feat(android): add photo-permission rationale and recent-photos strip#510
Draft
jkmassel wants to merge 4 commits into
Draft
feat(android): add photo-permission rationale and recent-photos strip#510jkmassel wants to merge 4 commits into
jkmassel wants to merge 4 commits into
Conversation
This was referenced May 13, 2026
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/510")Built from 764b1ab |
6353b11 to
0943b8a
Compare
04fa2fc to
d7bd4ad
Compare
Layers on the Photos / Camera tiles introduced in the previous PR with
a full permission state machine, a rationale card, and a recent-photos
thumbnail strip.
- Three media-strip states resolved at runtime by `resolveMediaStripView`
in `PhotoAccessState.kt`:
1. **Rationale card** — shown when `READ_MEDIA_IMAGES` isn't granted
and the user hasn't dismissed it. Body copy and primary-button
label switch across three sub-states (`Unasked` / `Denied` /
`PermanentlyDenied`); SharedPreferences tracks the first-prompt
flag because `shouldShowRequestPermissionRationale` alone can't
tell "never asked" from "permanently denied".
2. **Compact tiles** — once the rationale is rejected, the Photos /
Camera column flattens into a full-width 88dp row. The rejection
auto-clears when permission is later granted, so a future
revocation surfaces the rationale again.
3. **Full strip** — once granted, queries MediaStore for the 64 most
recent images and renders them as a horizontally-scrolling 2-row
thumbnail grid. LazyRow keeps off-screen thumbnails out of memory.
- Strip is gated to Android 10+ (`ContentResolver.loadThumbnail` is
API 29+); on Android 7-9 the inserter falls back to the permissionless
Photos/Camera tile row and never requests the media permission.
- Observes the host Activity's lifecycle (not the BottomSheetDialog's
own) for `ON_RESUME`, so grants made via system Settings update the
strip on return without restart.
- `GutenbergView.resetBlockPickerPhotoPreferences(context)` exposed for
host apps that want to clear the rationale-rejection / first-prompt
flags from a settings screen — also wired into the demo's `⋮` menu
as **Manage Permissions**.
- Library declares `READ_MEDIA_IMAGES` and `READ_EXTERNAL_STORAGE`
(max SDK 32). Host apps can opt out via `tools:node="remove"`;
documented in `docs/integration.md` (Android → Manifest Permissions),
with the opt-out XML inlined as a comment in `AndroidManifest.xml` for
auditors diffing the merged manifest.
- Demo's "Enable Native Inserter" toggle now defaults to on so
reviewers see the new strip without flipping a setting.
Touch targets on the rationale buttons meet the Material 48dp minimum
via a wrapper that keeps the visual 32dp pill while inflating the click
area; a shared `MutableInteractionSource` keeps the ripple drawing
inside the pill instead of as a square halo.
…-ups Iterative follow-ups to the inserter's photo-permission strip after empirical testing on Pixel 9 Pro XL / Android 16: - **Process-wide thumbnail cache** keyed on `(uri, sizePx)` to skip the binder + GC churn of `ContentResolver.loadThumbnail` on scroll-back and dialog reopen. `RealThumbnail` seeds from the cache synchronously at composition so cache hits skip the grey-placeholder flash entirely. Sized at 32 entries (~6MB worst case at 64dp/3.5x density). Including `sizePx` in the key means a density change can't serve a wrong-sized bitmap. - **Drop unloadable thumbnails** from the displayed strip rather than rendering a permanent grey placeholder. Failed URIs are tracked in a `failed: Set<Uri>` keyed on the `Granted` instance and re-attempted on the next dialog open. `Log.w` on failure for engineer visibility (matches `EditorHTTPClient` / `EditorAssetsLibrary` convention). - **Soft-input mode** changed from `STATE_HIDDEN | ADJUST_NOTHING` to `STATE_HIDDEN | ADJUST_RESIZE`. The latched `ADJUST_NOTHING` was letting the IME cover the search field on in-dialog search taps; `STATE_HIDDEN` alone fixes the original double-launch race. - **Android 14 partial-grant** treated as granted by also checking `READ_MEDIA_VISUAL_USER_SELECTED` (API 34+) when `READ_MEDIA_IMAGES` is denied. Without this, a "Select photos" choice fell through to the rationale's "Open Settings" state for a permission the user had just granted. MediaStore automatically scopes the recent-images query to the user-selected items. - **Photo-prefs reads off the main thread** via a process-wide `PhotoPrefs` cache populated by `Dispatchers.IO` warmup dispatched from `GutenbergView`'s constructor. Writes update the cache synchronously (so subsequent reads see the new value) and queue the `SharedPreferences.edit().apply()` on the IO scope. Synchronous prefs access during the sheet's open animation was a StrictMode `detectDiskReads()` violation. Verified on Pixel 9 Pro XL with `./gradlew :Gutenberg:detekt :Gutenberg:assembleDebug :Gutenberg:testDebugUnitTest`.
The previous comment claimed Android 16 "doesn't reliably honor" `revokeSelfPermissionOnKill` based on empirical observation that the process died but the grant stuck. That's consistent with the docs' "may delay the revocation" language — the API schedules a revocation for the next *safe* kill, which isn't necessarily the kill the caller observes — so the platform-blaming overclaims. The real reason to route through system Settings is simpler and harder to argue with: `revokeSelfPermissionOnKill` is API 33+ only, and the demo's `minSdk = 24`. We'd need a Settings hand-off code path regardless. Settings also gives the user an OS-owned confirmation surface and avoids the lifecycle race of an async self-kill. Comment-only change in the demo app; no behaviour change.
The standalone-editor E2E test (`EditorTestHelpers.insertBlock`) taps "Add block" in the editor toolbar and waits for the web Gutenberg inserter (`[role='dialog'][aria-modal='true']`). With the demo's `enableNativeInserter` toggle defaulted to on (this PR), the same tap opens the native `BlockPickerDialog` bottom sheet instead, so the WebView selector never resolves and the test times out. Two narrow changes: - Add a `contentDescription = "Enable Native Inserter"` semantics modifier to the Switch in `SitePreparationActivity` so the test can find it without coupling to switch-order or sibling-text traversal. - In `EditorTestHelpers.navigateToEditor`, toggle the switch off after the configuration screen renders and before tapping Start. The WebView-side test path then drives the web inserter as before. The default-on toggle is preserved for reviewer ergonomics — fresh demo installs still see the photo-permission strip on first inserter open. The native inserter has its own manual test plan in this PR's description; the existing E2E suite continues to cover the web flow.
764b1ab to
64a055f
Compare
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Layers on top of #509 with the photo-permission state machine, the rationale card, and the recent-photos thumbnail strip.
Three media-strip states resolved at runtime by
resolveMediaStripViewinPhotoAccessState.kt:READ_MEDIA_IMAGESisn't granted and the user hasn't dismissed it. Body copy and primary-button label swap across three sub-states (Unasked/Denied/PermanentlyDenied); SharedPreferences tracks the first-prompt flag becauseshouldShowRequestPermissionRationalealone can't tell "never asked" from "permanently denied".clearRejectedRationale, so a future revocation surfaces the rationale again instead of stranding the user in CompactTiles with no in-app affordance to re-engage.LazyRowkeeps off-screen thumbnails out of memory.Lifecycle observer
The strip observes the host Activity's lifecycle (via
LocalContext.findActivity()walking theContextWrapperchain), not theBottomSheetDialog's ownLifecycleRegistry. The dialog's lifecycle only dispatchesON_RESUMEfromshow()and never refires when the user leaves to system Settings and returns; the Activity's does. This is what makes the "grant via Settings → strip updates on return" path work even when the inserter sheet stays open through the round-trip.canRepromptstateModelled as its own
mutableStateOf, explicitly recomputed at every permission-relevant signal (launcher result, lifecycle resume) via therefreshAccessStatehelper. Compose's snapshot system can't see the OS-levelshouldShowRequestPermissionRationaleflip fromtrue → falseon the 2nd denial otherwise —grantedandpromptedBeforeare already in their post-deny values, so neither would notify Compose and the rationale would stay stuck on "Try Again".Manifest permissions
This PR adds two host-visible
<uses-permission>declarations to the library manifest:READ_MEDIA_IMAGESREAD_EXTERNAL_STORAGEAndroid 14+'s three-option permission dialog ("Allow all" / "Select photos" / "Don't allow") is surfaced by the system whenever an app at targetSdk ≥ 34 requests
READ_MEDIA_IMAGES, regardless of whether the manifest opts in toREAD_MEDIA_VISUAL_USER_SELECTED(verified empirically on Pixel 9 Pro XL / Android 16).hasPhotosPermissionchecks both permissions, so a "Select photos" choice is treated as granted and MediaStore scopes the strip to the user-selected items. DeclaringREAD_MEDIA_VISUAL_USER_SELECTEDin the library manifest to unlock the "Select more photos" re-prompt UX is #511.Hosts that don't ship the media strip can opt out:
With the permissions removed,
hasPhotosPermissionreturnsfalse, the rationale is unreachable (the launcher request hits the merged-manifest check and resolves to denied), and the strip falls back to the CompactTiles layout from the parent PR. The Photos tile (system picker) and Camera tile continue to work — they're permissionless.Host integration
GutenbergView.resetBlockPickerPhotoPreferences(context)is exposed for host apps that want to clear the rationale-rejection / first-prompt flags from a settings screen.The demo's
⋮menu now has a Manage Permissions screen for replaying the rationale flow during review: it shows the current OS state forREAD_MEDIA_IMAGES, lets reviewers reset the in-app flags via the library API above, and hands off to the system permission settings for OS-level revokes.revokeSelfPermissionOnKillwas the obvious path for the OS-revoke side but requires API 33+; the demo'sminSdk = 24, so a Settings hand-off code path is needed regardless. Settings also gives the user an OS-owned confirmation surface and avoids the lifecycle race of an async self-kill. The screen refreshes onON_RESUMEso returning from Settings reflects the new state immediately.The demo's "Enable Native Inserter" toggle now defaults to on so reviewers see the new strip without flipping a setting.
Test plan
⋮→ Manage Permissions shows current OS state (Granted/Denied (system will re-prompt)/Denied (system prompt suppressed)).ON_RESUMEobserver../gradlew :Gutenberg:detekt :Gutenberg:assembleDebug :Gutenberg:testDebugUnitTestpasses (includesPhotoAccessStateTest).