Skip to content

Reland 79654#79656

Merged
fluttergithubbot merged 3 commits intoflutter:masterfrom
LongCatIsLooong:reland-79654
Apr 2, 2021
Merged

Reland 79654#79656
fluttergithubbot merged 3 commits intoflutter:masterfrom
LongCatIsLooong:reland-79654

Conversation

@LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Apr 2, 2021

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
AndroidSemanticsAction.copy,
AndroidSemanticsAction.setSelection,
AndroidSemanticsAction.setText,
AndroidSemanticsAction.nextAtMovementGranularity,
Copy link
Contributor

@chunhtai chunhtai Apr 2, 2021

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

@LongCatIsLooong LongCatIsLooong Apr 2, 2021

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

@LongCatIsLooong LongCatIsLooong Apr 2, 2021

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

@chunhtai chunhtai Apr 2, 2021

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

@LongCatIsLooong LongCatIsLooong Apr 2, 2021

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
case _kNextAtMovementGranularityIndex:
return 'AndroidSemanticsAction.nextAtMovementGranularity';
case _kPreviousAtMovementGranularityIndex:
return 'AndroidSemanticsAction.nextAtMovementGranularity';
Copy link
Contributor

@chunhtai chunhtai Apr 2, 2021

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