Skip to content

Reland 79654 #79656

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
Merged
merged 3 commits into from
Apr 2, 2021
Merged

Reland 79654 #79656

merged 3 commits into from
Apr 2, 2021

Conversation

LongCatIsLooong
Copy link
Contributor

Diff b2b7d41

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, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

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

@flutter-dashboard flutter-dashboard bot added a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: tests "flutter test", flutter_test, or one of our tests f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. c: contributor-productivity Team-specific productivity, code health, technical debt. labels Apr 2, 2021
@LongCatIsLooong LongCatIsLooong requested a review from chunhtai April 2, 2021 18:25
@google-cla google-cla bot added the cla: yes label Apr 2, 2021
@LongCatIsLooong
Copy link
Contributor Author

LongCatIsLooong commented Apr 2, 2021 •
edited
Loading

@chunhtai does the integration test change look right to you?

chunhtai
chunhtai reviewed Apr 2, 2021
View reviewed changes
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

I left a concern

dev/integration_tests/android_semantics_testing/test_driver/main_test.dart Outdated
@@ -157,6 +157,7 @@ void main() {
AndroidSemanticsAction.copy,
AndroidSemanticsAction.setSelection,
AndroidSemanticsAction.setText,
AndroidSemanticsAction.nextAtMovementGranularity,
Copy link
Contributor

Choose a reason for hiding this comment

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

If enter text sets the selection to the end , why there is only nextAtMovementGranularity but no previousAtMovementGranularity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure this is what the error message told me to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit confusing to me: https://developer.android.com/reference/android/view/accessibility/AccessibilityNodeInfo#ACTION_NEXT_AT_MOVEMENT_GRANULARITY

The example says "Move to the previous character"

Copy link
Contributor

Choose a reason for hiding this comment

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

We added semantics action in RenderEditable.describeSemanticsConfiguration. These action will propagate to AccessibilityBridge.java and the real android semantics action will be added in createAccessibilityNodeInfo in that file which will then show up in the test here. I think we should figure out why there is nextAtMovementGranularity because the RenderEditable.describeSemanticsConfiguration should only add previousAtMovementGranularity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok there were 2 typos. Fixed now.

@LongCatIsLooong LongCatIsLooong requested a review from chunhtai April 2, 2021 20:10
chunhtai
chunhtai approved these changes Apr 2, 2021
View reviewed changes
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

dev/integration_tests/android_semantics_testing/lib/src/constants.dart
@@ -144,7 +144,7 @@ class AndroidSemanticsAction {
case _kNextAtMovementGranularityIndex:
return 'AndroidSemanticsAction.nextAtMovementGranularity';
case _kPreviousAtMovementGranularityIndex:
return 'AndroidSemanticsAction.nextAtMovementGranularity';
Copy link
Contributor

Choose a reason for hiding this comment

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

lol, nice one

@LongCatIsLooong LongCatIsLooong mentioned this pull request Apr 2, 2021
Merged
8 tasks
@fluttergithubbot fluttergithubbot merged commit c1c5b49 into flutter:master Apr 2, 2021
@LongCatIsLooong LongCatIsLooong deleted the reland-79654 branch April 2, 2021 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chunhtai chunhtai chunhtai approved these changes

Assignees
No one assigned
Labels
a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LongCatIsLooong @chunhtai @fluttergithubbot