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

Conversation

knopp
Copy link
Member

@knopp knopp commented Aug 6, 2024

Fixes flutter/flutter#151428

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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 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.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@knopp knopp changed the title macOS: Udate platform node when AXNodeData role changes macOS: Update platform node when AXNodeData role changes Aug 6, 2024
@knopp knopp requested review from dkwingsmt and cbracken August 6, 2024 17:31
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

LGTM stamp from a Japanese personal seal

@cbracken
Copy link
Member

cbracken commented Aug 6, 2024

I kind of wonder if we should jam a debug assertion in shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMac.mm in the code near line 165 where we're casting directly to FlutterTextField unchecked. Something similar-ish to:

--- i/shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMac.mm
+++ w/shell/platform/darwin/macos/framework/Source/AccessibilityBridgeMac.mm
@@ -165,11 +165,12 @@
         // If it is a text field, the value change notifications are handled by
         // the FlutterTextField directly. Only need to make sure it is the
         // first responder.
-        FlutterTextField* native_text_field =
-            (FlutterTextField*)mac_platform_node_delegate->GetNativeViewAccessible();
+        id native_text_field = mac_platform_node_delegate->GetNativeViewAccessible();
+        FML_DCHECK([native_text_field isKindOfClass:FlutterTextField.class]);
         id focused = mac_platform_node_delegate->GetFocus();
         if (!focused || native_text_field == focused) {
-          [native_text_field startEditing];
+          [(FlutterTextField*)native_text_field startEditing];
         }
         break;
       }

Cast at the bottom is technically unnecessary, but nice to catch if we ever renamed startEditing.

@knopp knopp merged commit 206e86e into flutter:main Aug 6, 2024
26 checks passed
@knopp knopp deleted the update_platform_node_on_change branch August 6, 2024 19:51
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 6, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Aug 6, 2024
…152962)

flutter/engine@aabd582...206e86e

2024-08-06 [email protected] macOS: Update platform node when AXNodeData role changes (flutter/engine#54364)
2024-08-06 [email protected] Roll Skia from 690c8d946e03 to f04ba9cd0e4b (2 revisions) (flutter/engine#54370)
2024-08-06 [email protected] Roll Dart SDK from 71e84a69ce6d to dedfa3393296 (1 revision) (flutter/engine#54354)
2024-08-06 [email protected] Roll Skia from a15e5cfe046c to 690c8d946e03 (1 revision) (flutter/engine#54369)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
TytaniumDev pushed a commit to TytaniumDev/flutter that referenced this pull request Aug 7, 2024
…lutter#152962)

flutter/engine@aabd582...206e86e

2024-08-06 [email protected] macOS: Update platform node when AXNodeData role changes (flutter/engine#54364)
2024-08-06 [email protected] Roll Skia from 690c8d946e03 to f04ba9cd0e4b (2 revisions) (flutter/engine#54370)
2024-08-06 [email protected] Roll Dart SDK from 71e84a69ce6d to dedfa3393296 (1 revision) (flutter/engine#54354)
2024-08-06 [email protected] Roll Skia from a15e5cfe046c to 690c8d946e03 (1 revision) (flutter/engine#54369)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Buchimi pushed a commit to Buchimi/flutter that referenced this pull request Sep 2, 2024
…lutter#152962)

flutter/engine@aabd582...206e86e

2024-08-06 [email protected] macOS: Update platform node when AXNodeData role changes (flutter/engine#54364)
2024-08-06 [email protected] Roll Skia from 690c8d946e03 to f04ba9cd0e4b (2 revisions) (flutter/engine#54370)
2024-08-06 [email protected] Roll Dart SDK from 71e84a69ce6d to dedfa3393296 (1 revision) (flutter/engine#54354)
2024-08-06 [email protected] Roll Skia from a15e5cfe046c to 690c8d946e03 (1 revision) (flutter/engine#54369)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typing in TextField crashes app on macOs if virtual keyboard is open while enabling
2 participants