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

Fixes inset padding in android accessibility bridge#27083

Merged
fluttergithubbot merged 2 commits into
flutter-team-archive:masterfrom
chunhtai:issues/67835
Jul 1, 2021
Merged

Fixes inset padding in android accessibility bridge#27083
fluttergithubbot merged 2 commits into
flutter-team-archive:masterfrom
chunhtai:issues/67835

Conversation

@chunhtai

Copy link
Copy Markdown
Contributor

fixes flutter/flutter#67835

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

boolean needsToApplyCutoutInsect = true;
if (Build.VERSION.SDK_INT >= 28) {
Context context = rootAccessibilityView.getContext();
if (context instanceof Activity) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what is the best way to get the window attributes, if there is a better way, please let me know

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comment thread shell/platform/android/io/flutter/view/AccessibilityBridge.java
Comment thread shell/platform/android/io/flutter/view/AccessibilityBridge.java Outdated
* <p>This method will recursively traverse up the context chain if it is a {@link ContextWrapper}
* until it finds the first instance of the base context that is an {@link Activity}.
*/
private static Activity getActivity(Context context) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It feels weird to put it here, but I don't want to expose a public api on embedding/engine/FlutterView.java.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These packages are structured in a weird way. e.g. AccessibilityBridge should be in the same package as FlutterView. @jason-simmons do you have any context into why this is the case? Also, do you have any concerns if these are merged?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There's a io/flutter/util folder. Do you mind if we create a ViewUtil class and move this static method there, and replace the one from FlutterView as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@chunhtai

Copy link
Copy Markdown
Contributor Author

@blasten PTAL :)

final float[] identity = new float[16];
Matrix.setIdentityM(identity, 0);
// in android devices API 23 and above, the system nav bar can be placed on the left side
// In android devices API 23 and above, the system nav bar can be placed on the left side

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
// In android devices API 23 and above, the system nav bar can be placed on the left side
// In Android devices API 23 and above, the system nav bar can be placed on the left side

rootObject.globalGeometryDirty = true;
rootObject.inverseTransformDirty = true;
boolean needsToApplyLeftCutoutInsect = true;
// In android devices API 28 and above, the window attribute, layoutInDisplayCutoutMode,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
// In android devices API 28 and above, the window attribute, layoutInDisplayCutoutMode,
// In Android devices API 28 and above, the `layoutInDisplayCutoutMode` window attribute

rootObject.inverseTransformDirty = true;
boolean needsToApplyLeftCutoutInsect = true;
// In android devices API 28 and above, the window attribute, layoutInDisplayCutoutMode,
// can be set to allow overlapping content with the cutout area. Query the attribute

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
// can be set to allow overlapping content with the cutout area. Query the attribute
// can be set to allow overlapping content within the cutout area. Query the attribute

private boolean doesLayoutInDisplayCutoutModeRequireLeftInsect() {
Context context = rootAccessibilityView.getContext();
Activity activity = getActivity(context);
if (activity != null) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if activity is null then doesLayoutInDisplayCutoutModeRequireLeftInsect is true?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would expect something like this:

if (activity == null || activity.getWindow() == null) {
  // The activity is  null or not not visible, it does not matter whether to apply left insect
  // or not.
  return false;
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In that case, it probably doesn't matter? I can set it to false

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

sgtm

}

/**
* Reads the layoutInDisplayCutoutMode value from the window attribute and returns whether a left

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
* Reads the layoutInDisplayCutoutMode value from the window attribute and returns whether a left
* Reads the {@code layoutInDisplayCutoutMode} value from the window attribute and returns whether a left

@chunhtai chunhtai requested a review from blasten July 1, 2021 17:15

@blasten blasten left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks @chunhtai ! LGTM

@chunhtai chunhtai added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 1, 2021
@fluttergithubbot

Copy link
Copy Markdown
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Linux Web Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 1, 2021
@chunhtai chunhtai added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 1, 2021
@fluttergithubbot

Copy link
Copy Markdown
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Linux Web Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 1, 2021
@chinmaygarde

Copy link
Copy Markdown
Contributor

This one needs a rebase as the failing tests have been disabled on ToT.

@chinmaygarde chinmaygarde changed the title Fixes insect padding in android accessibility bridge Fixes inset padding in android accessibility bridge Jul 1, 2021
@chinmaygarde

Copy link
Copy Markdown
Contributor

Please amend the commit message to fix the typo before landing.

@chunhtai chunhtai added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jul 1, 2021
@fluttergithubbot fluttergithubbot merged commit eaf70ca into flutter-team-archive:master Jul 1, 2021
naudzghebre pushed a commit to naudzghebre/engine that referenced this pull request Sep 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-android waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Development

Successfully merging this pull request may close these issues.

[a11y][Android] TalkBack focus border is incorrectly positioned when device has a notch on left side of the screen

4 participants