Page MenuHomePhabricator
Differential D13697

[native] Fix NUX Android problem
ClosedPublic
Actions

Authored by inka on Oct 11 2024, 2:50 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 3, 11:52 AM
Unknown Object (File)
Wed, Apr 2, 2:55 PM
Unknown Object (File)
Tue, Apr 1, 5:09 PM
Unknown Object (File)
Tue, Apr 1, 3:29 PM
Unknown Object (File)
Tue, Apr 1, 4:10 AM
Unknown Object (File)
Sun, Mar 30, 11:34 AM
Unknown Object (File)
Thu, Mar 27, 7:33 PM
Unknown Object (File)
Sat, Mar 22, 9:11 AM
View All Files
Subscribers
ashoat

Details

Reviewers
tomek
kamil
Commits
rCOMMe43c6b922c25: [native] Fix NUX Android problem
Summary

issue: ENG-9472
On some Androids, we cannot measure a button that is not visible on the screen. undefineds are returned. So we have to wait of the login screen to be dismissed, before we call measure.
I wanted to pass the measure function itseft through conxtext, but that results in Cannot read property '_nativeTag' of undefined error. Not sure what that means, but I don't think passing the ref is much worse.
measure asynchronously measures the component and then calls the callback. This is by I need states and an effect, instead of a memo.

buttonMeasured flag ensures the tip doesn't jump after the measure is done. measure returns quickly and I don't think this slight delay is a problem.

Test Plan

Tested on Android 11 physical device, and iOS 17.4 simulator. Tesdted by updating NUXHandlerInner to run the effect on every app reaload.
Checked that on both Android and iOS the NUX tips are displayed in the correct position. Checked thare are no odd visual effects, like jumping (thanks to buttonMeasured)

Diff Detail

Repository
rCOMM Comm
Branch
nux_fix
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

inka created this revision.Oct 11 2024, 2:50 AM
Herald added a subscriber: ashoat. · View Herald TranscriptOct 11 2024, 2:50 AM
inka edited the summary of this revision. (Show Details)Oct 11 2024, 2:51 AM
Harbormaster completed remote builds in B32196: Diff 45092.Oct 11 2024, 3:11 AM
inka requested review of this revision.Oct 11 2024, 3:11 AM
tomek requested changes to this revision.Oct 11 2024, 4:53 AM
tomek added inline comments.
native/chat/chat-tab-bar.react.js
33–44 ↗(On Diff #45092)

This pattern doesn't make too much sense to me - there shouldn't be any need to save the ref in the context on each layout - doing it once should be enough. Also, we don't need to useRef to handle that - we could use a callback ref https://pl.legacy.reactjs.org/docs/refs-and-the-dom.html#callback-refs and set the value in the context every time it is called.

native/tooltip/nux-tips-overlay.react.js
127–137 ↗(On Diff #45092)

The types should be readonly

139–140 ↗(On Diff #45092)

Why do we need to introduce a new state for this? Can't we make the states where we keep the measurement optional?

150–152 ↗(On Diff #45092)

Is there a reason to keep these separated?

This revision now requires changes to proceed.Oct 11 2024, 4:53 AM
tomek added a comment.Oct 11 2024, 4:54 AM

I wanted to pass the measure function itseft through conxtext, but that results in Cannot read property '_nativeTag' of undefined error.

My guess is that we need to bind these methods.

inka updated this revision to Diff 45137.Oct 14 2024, 1:57 AM

Address review. Going to try biding the methods now

Harbormaster completed remote builds in B32220: Diff 45137.Oct 14 2024, 2:15 AM
inka updated this revision to Diff 45138.Oct 14 2024, 2:19 AM

Bind measure

native/tooltip/nux-tips-overlay.react.js
139–140 ↗(On Diff #45092)

I didn't like all the ifs this made me add to the code, but I can do it if you prefer that

Harbormaster completed remote builds in B32221: Diff 45138.Oct 14 2024, 2:37 AM
tomek accepted this revision.Oct 14 2024, 3:48 AM
tomek added inline comments.
native/chat/chat-tab-bar.react.js
36 ↗(On Diff #45138)

You can even define this outside the component.

native/components/nux-tips-context.react.js
4 ↗(On Diff #45138)

Are you sure this is the correct place to import from?

native/tooltip/nux-tips-overlay.react.js
139–140 ↗(On Diff #45092)

Agree, this isn't ideal. However, I think it is better to have explicit empty objects and nulls before the measurement is concluded than to use the incorrect default values.

This revision is now accepted and ready to land.Oct 14 2024, 3:48 AM
inka updated this revision to Diff 45147.Oct 14 2024, 4:46 AM

Move callbacks out of components

native/components/nux-tips-context.react.js
4 ↗(On Diff #45138)

This is the only place where this flow type is defined in the react-native repo (I searched it on github). My IDE doesn't recognise any other possibilities either.

Harbormaster completed remote builds in B32225: Diff 45147.Oct 14 2024, 5:06 AM
Closed by commit rCOMMe43c6b922c25: [native] Fix NUX Android problem (authored by inka). · Explain WhyOct 14 2024, 7:24 AM
This revision was automatically updated to reflect the committed changes.
inka added a commit: rCOMMe43c6b922c25: [native] Fix NUX Android problem.

Revision Contents
Changeset List

PathSize
native/
chat/
39 lines
36 lines
components/
10 lines
tooltip/
132 lines

Diff 45137

View Options

native/chat/chat-tab-bar.react.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

native/chat/chat.react.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

native/components/nux-tips-context.react.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines
View Options

native/tooltip/nux-tips-overlay.react.js

Show First 20 Lines • Show All 396 Lines • ▼ Show 20 Lines async ensureAndroidPushNotifsEnabled() {
let [hasPermission] = permissionPromisesResult; let [hasPermission] = permissionPromisesResult;
const [, canRequestPermission] = permissionPromisesResult; const [, canRequestPermission] = permissionPromisesResult;
if (!hasPermission && canRequestPermission) { if (!hasPermission && canRequestPermission) {
const permissionResponse = await (async () => { const permissionResponse = await (async () => {
// We issue a call to sleep to match iOS behavior where prompt // We issue a call to sleep to match iOS behavior where prompt
// doesn't appear immediately but after logged-out modal disappears // doesn't appear immediately but after logged-out modal disappears
await sleep(10); await sleep(10);
await this.requestAndroidNotificationsPermission(); return await this.requestAndroidNotificationsPermission();
marcinUnsubmitted
Not Done
Inline Actions

We definitely should return something from this lambda since otherwise it is useless to assign it to variable.

When we find we don't have Android notif permissions, we ignore the result of requestAndroidNotificationsPermission. It seems like that would cause us to fail to register notif permissions on Android.

If we find that we don't have permissions then just calling await this.requestAndroidNotificationsPermission(); would result in a prompt asking for notifications permissions. However if the user grants those permissions then hasPermissions is still falsy (since promise returned nothing), so deviceToken will be set to null. Nevertheless permissions are actually granted byt the OS, so the state on the device and keyserver would heal itself on next render.

This differential fixes this case so that if user grants permissions correct state is achieved immediately without need for additional re-render to heal the state.

marcin: We definitely should return something from this lambda since otherwise it is useless to assign…
})(); })();
hasPermission = permissionResponse; hasPermission = permissionResponse;
} }
if (!hasPermission) { if (!hasPermission) {
this.failedToRegisterPushPermissionsAndroid(!canRequestPermission); this.failedToRegisterPushPermissionsAndroid(!canRequestPermission);
return; return;
} }
▲ Show 20 Lines • Show All 328 Lines • Show Last 20 Lines