Fix iOS safari keyboard issue when semantics is enabled #38822

nbayati · 2023-01-12T23:07:13Z

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.

yjbanov · 2023-01-12T23:14:13Z

lib/web_ui/lib/src/engine/semantics/text_field.dart

+  Timer? _positionInputElementTimer;
+  static const Duration _delayBeforePlacement = Duration(milliseconds: 100);
+
+  void _createEditableElement() {


Is it legal to call this method if editableElement != null? If not, should we add an assert that checks that this doesn't happen by mistake?

nit: "create" is typically used for functions that return the value that's created, but this method creates and assigns it to a field but returns nothing. For such methods a more common verb is "initialize".

yjbanov · 2023-01-12T23:15:15Z

lib/web_ui/lib/src/engine/semantics/text_field.dart

+    activeEditableElement.style.transform = 'translate(-999px,-999px)';
+    _positionInputElementTimer?.cancel();
+    _positionInputElementTimer = Timer(_delayBeforePlacement, () {
+      editableElement?.style.transform = '';


We might want to null out _positionInputElementTimer when it fires to avoid having a stale timer object lurking in the system.

Good point! Done!

yjbanov · 2023-01-12T23:19:32Z

lib/web_ui/lib/src/engine/semantics/text_field.dart

+    });
+
+    activeEditableElement.focus();
+    semanticsObject.element.removeAttribute('role');


Is this to avoid having two elements with the "textfield" role representing the same text field? If so, might be worth leaving a comment explaining it for a future reader.

Yes, VoiceOver starts acting weird if we don't remove the role.
Sure! I added some comments.

yjbanov · 2023-01-12T23:23:48Z

lib/web_ui/lib/src/engine/semantics/text_field.dart

-      _hasFocused = false;
+  @override
+  void update() {
+    if (editableElement != null) {


I think all the editableElement != null checks can be combined into one top-level check, so that we don't need to check it again multiple times.

Yep, you're right.

yjbanov · 2023-01-12T23:26:19Z

lib/web_ui/lib/src/engine/semantics/text_field.dart

-    editableElement.remove();
+    _positionInputElementTimer?.cancel();
+    _positionInputElementTimer = null;
+    activeEditableElement.remove();


Are we guaranteed to have an activeEditableElement? If not, this can throw. I can imagine a text field to never be focused and disposed. Also, we might want to have a test for disposing both focused and unfocused text fields.

Good point! Done!

yjbanov · 2023-01-12T23:28:28Z

lib/web_ui/test/engine/semantics/text_field_test.dart

+      expect(await logger.actionLog.first, ui.SemanticsAction.tap);
+    });
+
+      test('Syncs editing state from framework', () {


There's an extra indent compared to sigling tests.

yjbanov · 2023-01-12T23:30:02Z

lib/web_ui/test/engine/semantics/text_field_test.dart

+    });
+  }, skip: isIosSafari);
+
+  group('$SemanticsTextEditingStrategy in iOS', () {


Since we're only emulating iOS here, we probably want to limit the number of browsers we run this on to avoid browser quirks. I think we should pick one browser that's closest to iOS Safari, which I think is macOS Safari.

yjbanov

yjbanov · 2023-01-13T21:11:22Z

lib/web_ui/lib/src/engine/semantics/text_field.dart

+    }
+
+    _initializeEditableElement();
+    activeEditableElement.style.transform = 'translate(-999px,-999px)';


With 8k screen resolutions getting to 7680x4320 these days, I wonder if 999 is a sufficient #offset. Would using a bigger number, e.g. 20000, work?

You're right. I've increased it to 9999 to match what we have in the non semantic iOS strategy.

…en semantics is enabled

…utter/engine#38822)

…118720) * 5aee7914c Roll Dart SDK from 645fd748e79e to 0097dcbada06 (3 revisions) (flutter/engine#38883) * 78a68b9df Roll Skia from c64a10d525d1 to aa99fa0bdd31 (10 revisions) (flutter/engine#38886) * 398af8c20 Roll Fuchsia Mac SDK from w0hr1ZMvYGJnWInwK... to nPbEqgIfE8TLOokQH... (flutter/engine#38889) * 703502712 Roll Skia from aa99fa0bdd31 to 5a51583d09dd (2 revisions) (flutter/engine#38892) * 2075cf3f8 Roll Fuchsia Mac SDK from nPbEqgIfE8TLOokQH... to SmEVvAFBC8oriPBou... (flutter/engine#38897) * ef06ef602 Roll Skia from 5a51583d09dd to 34a14247bd4b (1 revision) (flutter/engine#38901) * 0d374a919 Roll Skia from 34a14247bd4b to c727016cc09e (1 revision) (flutter/engine#38903) * 31f2020d9 Roll Skia from c727016cc09e to 214411b09e92 (2 revisions) (flutter/engine#38907) * a3006de27 Bump actions/setup-python from 4.4.0 to 4.5.0 (flutter/engine#38909) * eacc09c47 Bump github/codeql-action from 2.1.37 to 2.1.38 (flutter/engine#38910) * c33c11c93 Roll Skia from 214411b09e92 to 3704033dd106 (1 revision) (flutter/engine#38908) * afa093dbe [Impeller] Map UVs correctly for TiledTextureContents (flutter/engine#38894) * 46ea2b3cc Roll Fuchsia Mac SDK from SmEVvAFBC8oriPBou... to bZfX8KCoI3CfxEzsG... (flutter/engine#38912) * aadb97112 Roll Skia from 3704033dd106 to 58e745419425 (4 revisions) (flutter/engine#38936) * 24b21a5b4 Roll Fuchsia Mac SDK from bZfX8KCoI3CfxEzsG... to iwCuzAuMgsk75hlT0... (flutter/engine#38938) * e65558087 Roll Skia from 58e745419425 to 6afb97022fa7 (1 revision) (flutter/engine#38943) * 5932badce Revert "Revert "[web] Don't overwrite editing state with semantic updates (#38271)" (#38562)" (flutter/engine#38854) * 928772eba [web] Reduce code size impact of fallback font data (flutter/engine#38787) * 88a8d9a36 Roll Skia from 6afb97022fa7 to 8ea9b39f7213 (18 revisions) (flutter/engine#38952) * 3a479434b Roll Skia from 8ea9b39f7213 to 69d8378097df (6 revisions) (flutter/engine#38953) * 641623a51 wrap focus() in a post update callback (flutter/engine#38958) * 7d71ee3bf Roll Fuchsia Mac SDK from iwCuzAuMgsk75hlT0... to -1N2YUML8GqNhK8YP... (flutter/engine#38960) * d762142bb reland fix canvas drawLine bugs (flutter/engine#38949) * 0d7953387 [Impeller] Fix back-to-back clip restoration (flutter/engine#38964) * 19fe86ca9 Update scenario screenshots to iOS 16 (flutter/engine#38962) * eb18ac0a7 Fix iOS safari keyboard issue when semantics is enabled (flutter/engine#38822) * 84a73d92f [Impeller Scene] Add SceneNodeValue for synchronously fetching loaded ipscenes (flutter/engine#38913) * 7bff5dd11 Revert "Roll Dart SDK from 645fd748e79e to 0097dcbada06 (3 revisions) (#38883)" (flutter/engine#38973) * c52b29081 Roll Fuchsia Mac SDK from -1N2YUML8GqNhK8YP... to P5QcCJU8I71xVXuMT... (flutter/engine#38976)

…lutter#118720) * 5aee7914c Roll Dart SDK from 645fd748e79e to 0097dcbada06 (3 revisions) (flutter/engine#38883) * 78a68b9df Roll Skia from c64a10d525d1 to aa99fa0bdd31 (10 revisions) (flutter/engine#38886) * 398af8c20 Roll Fuchsia Mac SDK from w0hr1ZMvYGJnWInwK... to nPbEqgIfE8TLOokQH... (flutter/engine#38889) * 703502712 Roll Skia from aa99fa0bdd31 to 5a51583d09dd (2 revisions) (flutter/engine#38892) * 2075cf3f8 Roll Fuchsia Mac SDK from nPbEqgIfE8TLOokQH... to SmEVvAFBC8oriPBou... (flutter/engine#38897) * ef06ef602 Roll Skia from 5a51583d09dd to 34a14247bd4b (1 revision) (flutter/engine#38901) * 0d374a919 Roll Skia from 34a14247bd4b to c727016cc09e (1 revision) (flutter/engine#38903) * 31f2020d9 Roll Skia from c727016cc09e to 214411b09e92 (2 revisions) (flutter/engine#38907) * a3006de27 Bump actions/setup-python from 4.4.0 to 4.5.0 (flutter/engine#38909) * eacc09c47 Bump github/codeql-action from 2.1.37 to 2.1.38 (flutter/engine#38910) * c33c11c93 Roll Skia from 214411b09e92 to 3704033dd106 (1 revision) (flutter/engine#38908) * afa093dbe [Impeller] Map UVs correctly for TiledTextureContents (flutter/engine#38894) * 46ea2b3cc Roll Fuchsia Mac SDK from SmEVvAFBC8oriPBou... to bZfX8KCoI3CfxEzsG... (flutter/engine#38912) * aadb97112 Roll Skia from 3704033dd106 to 58e745419425 (4 revisions) (flutter/engine#38936) * 24b21a5b4 Roll Fuchsia Mac SDK from bZfX8KCoI3CfxEzsG... to iwCuzAuMgsk75hlT0... (flutter/engine#38938) * e65558087 Roll Skia from 58e745419425 to 6afb97022fa7 (1 revision) (flutter/engine#38943) * 5932badce Revert "Revert "[web] Don't overwrite editing state with semantic updates (flutter#38271)" (flutter#38562)" (flutter/engine#38854) * 928772eba [web] Reduce code size impact of fallback font data (flutter/engine#38787) * 88a8d9a36 Roll Skia from 6afb97022fa7 to 8ea9b39f7213 (18 revisions) (flutter/engine#38952) * 3a479434b Roll Skia from 8ea9b39f7213 to 69d8378097df (6 revisions) (flutter/engine#38953) * 641623a51 wrap focus() in a post update callback (flutter/engine#38958) * 7d71ee3bf Roll Fuchsia Mac SDK from iwCuzAuMgsk75hlT0... to -1N2YUML8GqNhK8YP... (flutter/engine#38960) * d762142bb reland fix canvas drawLine bugs (flutter/engine#38949) * 0d7953387 [Impeller] Fix back-to-back clip restoration (flutter/engine#38964) * 19fe86ca9 Update scenario screenshots to iOS 16 (flutter/engine#38962) * eb18ac0a7 Fix iOS safari keyboard issue when semantics is enabled (flutter/engine#38822) * 84a73d92f [Impeller Scene] Add SceneNodeValue for synchronously fetching loaded ipscenes (flutter/engine#38913) * 7bff5dd11 Revert "Roll Dart SDK from 645fd748e79e to 0097dcbada06 (3 revisions) (flutter#38883)" (flutter/engine#38973) * c52b29081 Roll Fuchsia Mac SDK from -1N2YUML8GqNhK8YP... to P5QcCJU8I71xVXuMT... (flutter/engine#38976)

* Update branch with changes in main - Fix iOS safari keyboard issue when semantics is enabled * Update branch with main - small enhancements * set offset to -9999px instead of -999 * Add editing state tests to ios * replace editableElement with the null checked one

flutter-dashboard bot added the platform-web Code specifically for the web engine label Jan 12, 2023

nbayati requested a review from yjbanov January 12, 2023 23:07

yjbanov reviewed Jan 12, 2023

View reviewed changes

nbayati requested a review from yjbanov January 13, 2023 02:24

yjbanov approved these changes Jan 13, 2023

View reviewed changes

nbayati added 3 commits January 17, 2023 17:22

Update branch with changes in main - Fix iOS safari keyboard issue wh…

f5178f2

…en semantics is enabled

Update branch with main - small enhancements

c4380da

set offset to -9999px instead of -999

0514a21

nbayati force-pushed the ios-a11y-input branch from 961791d to 0514a21 Compare January 18, 2023 01:37

nbayati added 2 commits January 17, 2023 18:07

Add editing state tests to ios

ef4b6da

replace editableElement with the null checked one

2b2abdb

nbayati merged commit eb18ac0 into flutter:main Jan 18, 2023

engine-flutter-autoroll mentioned this pull request Jan 18, 2023

Roll Flutter Engine from f79030440948 to 84a73d92ffc4 (27 revisions) flutter/flutter#118683

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2023

eb18ac0a7 Fix iOS safari keyboard issue when semantics is enabled (fl…

91a6bd3

…utter/engine#38822)

engine-flutter-autoroll mentioned this pull request Jan 18, 2023

Roll Flutter Engine from f79030440948 to 7bff5dd115d7 (28 revisions) flutter/flutter#118686

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2023

eb18ac0a7 Fix iOS safari keyboard issue when semantics is enabled (fl…

2c6a248

…utter/engine#38822)

engine-flutter-autoroll mentioned this pull request Jan 18, 2023

Roll Flutter Engine from f79030440948 to c52b290813bd (29 revisions) flutter/flutter#118720

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 18, 2023

eb18ac0a7 Fix iOS safari keyboard issue when semantics is enabled (fl…

8e54fec

…utter/engine#38822)

Fix iOS safari keyboard issue when semantics is enabled #38822

Fix iOS safari keyboard issue when semantics is enabled #38822

Uh oh!

Conversation

nbayati commented Jan 12, 2023

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!