Automattic / Automattic/newspack-rolling-coverage

Follow button hangs on a dismissed permission prompt, and can show the wrong state

Open
#34 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
PHP
Stars
1
Forks
1
Avg merge
2d 18h
Merged PRs (30d)
3

Description

## Summary

The Coverage Follow button has two states it cannot recover from, and one that reports the wrong thing. The bounded SDK wait added in [#18](https://github.com/Automattic/newspack-rolling-coverage/pull/18) covers the case where the OneSignal SDK never loads. It does not cover a reader dismissing the browser's permission prompt, which leaves the button disabled and labelled **Following** for a subscription that was never recorded.

From the [#18](https://github.com/Automattic/newspack-rolling-coverage/pull/18) review.

## Dismissed permission prompt leaves the button stuck

`requestNotificationPermission()` resolves only through the `permissionChange` event. A reader who dismisses the native prompt with the X leaves `Notification.permission` at `default`, so no event fires, the promise never settles, and the `permissionChange` listener is left attached — one per click.

The 10-second timeout does not help here, because it is cleared before the await:

```ts
// src/blocks/coverage-follow/view.ts:164-165
hasResolvedSdkWait = true;
window.clearTimeout( timeoutId );
```

```ts
// src/blocks/coverage-follow/view.ts:177
await requestNotificationPermission( OneSignal );
```

The optimistic label set on click then stands: the button reads **Following** with `aria-pressed="true"` while `addTag()` was never called, so the UI and the reader's real tag state disagree with no way back short of a reload.

`OneSignal.Notifications.requestPermission()` returns a promise. Awaiting that and re-checking `Notification.permission`, or racing the event against a timeout the way the SDK wait now does, settles it either way.

The `Notification.permission === 'denied'` short-circuit added in #18 correctly handles the already-denied case, which is a related but different state.

## Initial state can be wrong for a returning follower

`syncFollowButtons` runs once, in the first deferred callback. `OneSignal.User.getTags()` reads the SDK's locally cached tag state, which is not guaranteed to be hydrated from the server at that moment, so a reader who already follows a coverage can be shown **Follow**. Clicking re-adds the tag, which is harmless, but the label stays wrong until reload. Re-running the sync once the SDK reports the user as loaded would close it.

## Status message may not be announced

`setStatusMessage` writes `textContent` and then unhides the element:

```ts
// src/blocks/coverage-follow/view.ts:54
```

An `aria-live` region whose content changes while hidden is generally not announced, and unhiding an already-populated region is not a content mutation. Several screen readers announce nothing, on exactly the blocked-notifications and error cases a non-sighted reader most needs. Setting `hidden = false` before writing `textContent` fixes it.

The rest of the button's accessibility holds up: `aria-pressed` is in the server-rendered markup and kept in sync, and the region is created on click so it exists before the async message arrives.

## Rendering and messaging

Two smaller things in the same area:

- **The button is gated on a send-side function.** `should_render()` reaches `is_onesignal_v3_active()`, which tests `function_exists( 'onesignal_create_notification' )` (`includes/class-push-notifications.php:198`). That is a sending function, evaluated at `init` priority 10 during block registration. Gating the front-end render on what the front end needs — the `OneSignalWPSetting` `app_id`, meaning the Web SDK will be present — would decouple the reader-facing button from the publishing path.
- **The v2 notice points at the wrong fix.** It asks the editor to update the plugin, but `onesignal.php:28-33` decides which branch loads from `get_option( 'onesignal_plugin_migrated' )` and the presence of existing settings. A site with pre-v3 settings that has not migrated stays on v2 however current the files are. Pointing at OneSignal's migration would save a round trip.
- **Existing coverage blocks never get a button.** `INNER_TEMPLATE` applies only to newly inserted blocks, so a Rolling Coverage block already on the site renders nothing here. That is defensible given the block is removable, but nothing tells an editor the feature exists or how to add it.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with src/blocks/coverage-follow/view.ts, especially requestNotificationPermission, syncFollowButtons, and setStatusMessage, then inspect includes/class-push-notifications.php and onesignal.php for the rendering and migration paths. Trace each reported state before changing behavior. Done means dismissed prompts recover, returning followers show the correct state, status messages are announced, rendering uses the Web SDK setting, and the v2 notice and existing-block behavior are addressed.

Written by the indexing model from the issue text.

Assessment

Tech stack
php, typescript
Domain
accessibility, backend, frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
50/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.