Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Android] Drops semantics query when app is not attached #52040

Merged
auto-submit merged 4 commits into from
Aug 26, 2024
Merged

[Android] Drops semantics query when app is not attached #52040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a0f9ed
Drops semantics query when app is not attached
chunhtai Apr 10, 2024
44a9ee9
update
chunhtai Apr 10, 2024
404a55e
update
chunhtai Aug 13, 2024
c9e5da9
format
chunhtai Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions shell/platform/android/io/flutter/embedding/engine/FlutterJNI.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -873,7 +873,13 @@ private native void nativeDispatchSemanticsAction(
@UiThread
public void setSemanticsEnabled(boolean enabled) {
ensureRunningOnMainThread();
ensureAttachedToNative();
if (isAttached()) {
Copy link
Contributor Author

@chunhtai chunhtai Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @reidbaker is there a good way to test this?

Things i have tried:

  1. make nativeSetSemanticsEnabled public and mock it using mockito.
  2. I try creating a public method that just call nativeSetSemanticsEnabled and use spy to mock the instance.

both of them throws UnSatisfiedLink error on nativeSetSemanticsEnabled. The only way I can think of is to extends the entire FlutterJNI and override method to do nothing, but I am wondering if that is preferred or is there any other way.

Copy link
Contributor

@reidbaker reidbaker Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this pr does not have a test I cant see what testing environment you are using. My guess is that if your test contains whatever dependency has nativeSetSemanticsEnabled then the issue is that your c code needs to run on an android device (or maybe with robolectric).

I also found this chromium documentation https://www.chromium.org/developers/testing/android-tests/testing-android-code-that-crosses-the-c-java-boundary/ that suggest that a spy is the correct solution.

Copy link
Contributor Author

@chunhtai chunhtai Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still can't find a good way to use spy without trigger the error. I end up override entire class a mock out the part i don't need

setSemanticsEnabledInNative(enabled);
}
}

@VisibleForTesting
public void setSemanticsEnabledInNative(boolean enabled) {
nativeSetSemanticsEnabled(nativeShellHolderId, enabled);
}

Expand All @@ -884,7 +890,13 @@ public void setSemanticsEnabled(boolean enabled) {
@UiThread
public void setAccessibilityFeatures(int flags) {
ensureRunningOnMainThread();
ensureAttachedToNative();
if (isAttached()) {
setAccessibilityFeaturesInNative(flags);
}
}

@VisibleForTesting
public void setAccessibilityFeaturesInNative(int flags) {
nativeSetAccessibilityFeatures(nativeShellHolderId, flags);
}

Expand Down
53 changes: 53 additions & 0 deletions shell/platform/android/test/io/flutter/embedding/engine/FlutterJNITest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import static io.flutter.Build.API_LEVELS;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
Expand Down Expand Up @@ -151,6 +153,32 @@ public void computePlatformResolvedLocaleCallsLocalizationPluginProperly() {
assertEquals(result[2], "");
}

@Test
public void setAccessibilityIfAttached() {
// --- Test Setup ---
FlutterJNITester flutterJNI = new FlutterJNITester(true);
int expectedFlag = 100;

flutterJNI.setAccessibilityFeatures(expectedFlag);
assertEquals(flutterJNI.flags, expectedFlag);

flutterJNI.setSemanticsEnabled(true);
assertTrue(flutterJNI.semanticsEnabled);
}

@Test
public void doesNotSetAccessibilityIfNotAttached() {
// --- Test Setup ---
FlutterJNITester flutterJNI = new FlutterJNITester(false);
int flags = 100;

flutterJNI.setAccessibilityFeatures(flags);
assertEquals(flutterJNI.flags, 0);

flutterJNI.setSemanticsEnabled(true);
assertFalse(flutterJNI.semanticsEnabled);
}

public void onDisplayPlatformView_callsPlatformViewsController() {
PlatformViewsController platformViewsController = mock(PlatformViewsController.class);

Expand Down Expand Up @@ -256,4 +284,29 @@ public void setRefreshRateFPS_callsUpdateRefreshRate() {
// --- Verify Results ---
verify(flutterJNI, times(1)).updateRefreshRate();
}

static class FlutterJNITester extends FlutterJNI {
FlutterJNITester(boolean attached) {
this.isAttached = attached;
}

final boolean isAttached;
boolean semanticsEnabled = false;
int flags = 0;

@Override
public boolean isAttached() {
return isAttached;
}

@Override
public void setSemanticsEnabledInNative(boolean enabled) {
semanticsEnabled = enabled;
}

@Override
public void setAccessibilityFeaturesInNative(int flags) {
this.flags = flags;
}
}
}