Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: do not suppress accessibility services #289

Merged
merged 11 commits into from
Mar 29, 2020
Merged

feat: do not suppress accessibility services #289

merged 11 commits into from
Mar 29, 2020

Conversation

KazuCocoa
Copy link
Member

@KazuCocoa KazuCocoa commented Jul 15, 2019 •
edited
Loading

Related to appium/appium#4910

Set UiAutomation.FLAG_DONT_SUPPRESS_ACCESSIBILITY_SERVICES if DISABLE_SUPPRESS_ACCESSIBILITY_SERVICESkey and value, true or false is given as a parameter of instrument.

Respect existing setting if no DISABLE_SUPPRESS_ACCESSIBILITY_SERVICES key is given

@KazuCocoa KazuCocoa changed the title [do not merge]do not supress accessibility services do not supress accessibility services Aug 12, 2019
@tryge
Copy link

tryge commented Oct 25, 2019

@KazuCocoa Any idea or plan how to fix the failing checks? (And whether to merge this fix at some point?)

@KazuCocoa
Copy link
Member Author

Do you have time to run this change with the accessibility service enabled app?

I haven't tested this case, appium/appium#4910, well since I had no such app.
If someone could test this change and work fine, I'd like to continue to work this and appium/appium-espresso-driver#468

@tryge
Copy link

tryge commented Oct 25, 2019

We can run the test on Monday, what do we need to do? (We've only ever used appium releases so far...).

@KazuCocoa
Copy link
Member Author

okay, thanks.
I will leave a comment on how to use this apk in your release env later.

@KazuCocoa
Copy link
Member Author

KazuCocoa commented Oct 26, 2019 •
edited
Loading

  1. Download apks.zip: Get debug build of this change
  2. Unzip and copy them into /usr/local/lib/node_modules/appium/node_modules/appium-uiautomator2-server/apks on your local
    • Overwrite: appium-uiautomator2-server-debug-androidTest.apk
    • Rename appium-uiautomator2-server-v4.3.0.apk in the zip to your local machine's appium-uiautomator2-server-vX_X_X.apk and overwrite it.
      • The name depends on appium version
  3. Uninstall UIA2 servers from your test device
  4. Run your tests

Then, make sure the above two overwritten apks will be installed on your Appium log.

The apks in the apk.zip were generated from this branch by npm run build

@tryge
Copy link

tryge commented Oct 31, 2019

We tried to get it to work with these apks, but we haven't managed to do so. What do you need from us?

@KazuCocoa
Copy link
Member Author

The result of whether this UIA2 server apk works with the app under test which uses accessibility service without stopping the accessibility service

@tryge
Copy link

tryge commented Oct 31, 2019

We didn't see a difference in behaviour.

@KazuCocoa
Copy link
Member Author

thanks :)

@KazuCocoa KazuCocoa mentioned this pull request Mar 25, 2020
Closed
@KazuCocoa KazuCocoa changed the title do not supress accessibility services [wip] do not supress accessibility services Mar 25, 2020
@KazuCocoa KazuCocoa changed the title [wip] do not supress accessibility services feat: do not suppress accessibility services Mar 28, 2020
@KazuCocoa KazuCocoa mentioned this pull request Mar 28, 2020
Merged
mykola-mokhnach
mykola-mokhnach reviewed Mar 28, 2020
View reviewed changes
app/src/main/java/io/appium/uiautomator2/utils/Device.java Outdated
// The flag is necessary not to stop running accessibility service
// https://github.com/appium/appium/issues/4910
// https://developer.android.com/reference/android/app/UiAutomation.html#FLAG_DONT_SUPPRESS_ACCESSIBILITY_SERVICES
Configurator.getInstance().setUiAutomationFlags(UiAutomation.FLAG_DONT_SUPPRESS_ACCESSIBILITY_SERVICES);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we do this each time we get the instrumentation or it would be enough to call the configurator once?

@KazuCocoa
Copy link
Member Author

I ensured this logic also works well with Configurator.getInstance().getUiAutomationFlags() in getUiDevice by breakpoint debugging.

Configurator.getInstance().getUiAutomationFlags() is 0 without UiAutomation.FLAG_DONT_SUPPRESS_ACCESSIBILITY_SERVICES via disableSuppressAccessibilityService: false in appium/appium-uiautomator2-driver#376
Configurator.getInstance().getUiAutomationFlags() is 1 with UiAutomation.FLAG_DONT_SUPPRESS_ACCESSIBILITY_SERVICES via disableSuppressAccessibilityService: true in appium/appium-uiautomator2-driver#376

mykola-mokhnach
mykola-mokhnach reviewed Mar 29, 2020
View reviewed changes
app/src/main/java/io/appium/uiautomator2/server/ServerInstrumentation.java Outdated
Configurator.getInstance().setUiAutomationFlags(0);
}

private void disableSuppressAccessibilityService() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather rename this method to setAccessibilityServiceState

mykola-mokhnach
mykola-mokhnach reviewed Mar 29, 2020
View reviewed changes
app/src/main/java/io/appium/uiautomator2/server/ServerInstrumentation.java Outdated
private ServerInstrumentation(Context context, int serverPort) {
if (!isValidPort(serverPort)) {
throw new UiAutomator2Exception(String.format(
"The port is out of valid range [%s;%s]: %s", MIN_PORT, MAX_PORT, serverPort));
}
this.serverPort = serverPort;
this.powerManager = (PowerManager) context.getSystemService(Context.POWER_SERVICE);

setDefaultAutomationFlag();
Copy link
Contributor

Choose a reason for hiding this comment

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

should we always reset these flags or only before we actually change them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point.
I continued to observe the lifecycle of Configurator.getInstance() after the latest commit.
The value could remain before/after uia2 server lifecycle.

So, changing the state explicitly is better.

@KazuCocoa KazuCocoa changed the title feat: do not suppress accessibility services [wip]feat: do not suppress accessibility services Mar 29, 2020
@KazuCocoa KazuCocoa changed the title [wip]feat: do not suppress accessibility services feat: do not suppress accessibility services Mar 29, 2020
@KazuCocoa
Copy link
Member Author

Updated

mykola-mokhnach
mykola-mokhnach approved these changes Mar 29, 2020
View reviewed changes
Copy link
Contributor

@mykola-mokhnach mykola-mokhnach left a comment

Choose a reason for hiding this comment

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

I would be nice to document this feature somewhere

@mykola-mokhnach mykola-mokhnach merged commit 1e2c7dc into appium:master Mar 29, 2020
@KazuCocoa KazuCocoa deleted the do-no-suppress-as branch March 29, 2020 13:51
@nurdtechie98 nurdtechie98 mentioned this pull request Mar 30, 2022
Merged
@bartekpacia bartekpacia mentioned this pull request Sep 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mykola-mokhnach mykola-mokhnach mykola-mokhnach approved these changes

@dpgraham dpgraham Awaiting requested review from dpgraham

@imurchie imurchie Awaiting requested review from imurchie

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KazuCocoa @tryge @mykola-mokhnach