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.

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

Bring back paste button hide behavior 2 #56922

Merged
merged 2 commits into from May 11, 2020

Conversation

justinmc
Copy link
Contributor

Original PR that has been reverted twice now: #54902

This caused a failure where the ClipboardStatusNotifier was modified after being disposed, which seems to be caused by receiving the clipboard data after disposal. My one line fix for this is to check if it's disposed before updating the value.

@justinmc justinmc self-assigned this May 11, 2020
@fluttergithubbot fluttergithubbot added a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels. labels May 11, 2020
@justinmc justinmc requested a review from goderbauer May 11, 2020 17:08
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1524,7 +1524,7 @@ class ClipboardStatusNotifier extends ValueNotifier<ClipboardStatus> with Widget
final ClipboardStatus clipboardStatus = data != null && data.text != null && data.text.isNotEmpty
? ClipboardStatus.pasteable
: ClipboardStatus.notPasteable;
if (clipboardStatus == value) {
if (clipboardStatus == value || _disposed) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's move the if (_disposed) check to be the first thing in this callback since we don't have to do any of the other work if that happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Smart, will do.

@justinmc justinmc merged commit d988226 into flutter:master May 11, 2020
@justinmc justinmc deleted the pasteable-revert-revert-2 branch May 11, 2020 20:23
@jmagman
Copy link
Member

jmagman commented May 11, 2020

I believe this causes a new failure:

2020-05-11T14:41:55.457819: stdout: [+1992 ms] 00:09 �[32m+0�[0m�[31m -1�[0m: AccessibilityBridge TextField TextField has correct Android semantics �[1m�[31m[E]�[0m�[0m
2020-05-11T14:41:55.457984: stdout: [        ]   Expected: AndroidSemanticsNode with className: android.widget.EditText with actions: [AndroidSemanticsAction.clearAccessibilityFocus, AndroidSemanticsAction.click, AndroidSemanticsAction.setSelection] with flag isEditable: true with flag isFocusable: true with flag isFocused: true with flag isPassword: false
2020-05-11T14:41:55.458259: stdout: [        ]     Actual: AndroidSemanticsNode:<{flags: {isEditable: true, isEnabled: true, isDismissable: false, isCheckable: false, isChecked: false, isFocused: true, isLongClickable: false, isFocusable: true, isPassword: false}, id: 14, text: null, rect: {height: 144, width: 1080, bottom: 128.0, top: 80.0, left: 0.0, right: 360.0}, actions: [131072, 32768, 16, 128], className: android.widget.EditText, liveRegion: 1, contentDescription: null}>
2020-05-11T14:41:55.458442: stdout: [        ]      Which: Expected actions: [AndroidSemanticsAction.clearAccessibilityFocus, AndroidSemanticsAction.click, AndroidSemanticsAction.setSelection]
2020-05-11T14:41:55.458710: stdout: [        ]             Actual actions: [AndroidSemanticsAction.clearAccessibilityFocus, AndroidSemanticsAction.click, AndroidSemanticsAction.copy, AndroidSemanticsAction.setSelection]
2020-05-11T14:41:55.458935: stdout: [        ]             Unexpected: {AndroidSemanticsAction.copy}
2020-05-11T14:41:55.459211: stdout: [        ]             Missing: {}

android_semantics_integration_test_d988226_2.log

jmagman added a commit that referenced this pull request May 11, 2020
jmagman added a commit that referenced this pull request May 11, 2020
justinmc added a commit to justinmc/flutter that referenced this pull request May 13, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. team Infra upgrades, team productivity, code health, technical debt. See also team: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants