inputs: Add extended Wayland API for ConfirmComposition.

ConfirmComposition commits the current composition. Create a dedicated
extended Wayland API for this operation to avoid race conditions.

Bug: b/265853952
Change-Id: Ic26427b030e8c05f60567f0fc9d7edc392216c99
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4551404
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1150177}

ash/constants/ash_features.cc

@@ -816,6 +816,11 @@ BASE_FEATURE(kExoConsumedByImeByFlag,
              "ExoConsumedByImeByFlag",
              base::FEATURE_DISABLED_BY_DEFAULT);
 
+// Enables using an extended Wayland API for ConfirmCompositionText.
+BASE_FEATURE(kExoExtendedConfirmComposition,
+             "ExoExtendedConfirmComposition",
+             base::FEATURE_DISABLED_BY_DEFAULT);
+
 // Enables to check KeyEvent flag to see if the event is consumed by IME
 // or not (=decides using heuristics based on key code etc.).
 BASE_FEATURE(kExoSurroundingTextOffset,

ash/constants/ash_features.h

@@ -243,6 +243,8 @@ COMPONENT_EXPORT(ASH_CONSTANTS) BASE_DECLARE_FEATURE(kExoLinuxDmabufV4);
 COMPONENT_EXPORT(ASH_CONSTANTS) BASE_DECLARE_FEATURE(kExoLinuxDmabufModifiers);
 COMPONENT_EXPORT(ASH_CONSTANTS) BASE_DECLARE_FEATURE(kExoOrdinalMotion);
 COMPONENT_EXPORT(ASH_CONSTANTS) BASE_DECLARE_FEATURE(kExoConsumedByImeByFlag);
+COMPONENT_EXPORT(ASH_CONSTANTS)
+BASE_DECLARE_FEATURE(kExoExtendedConfirmComposition);
 COMPONENT_EXPORT(ASH_CONSTANTS) BASE_DECLARE_FEATURE(kExoSurroundingTextOffset);
 COMPONENT_EXPORT(ASH_CONSTANTS)
 BASE_DECLARE_FEATURE(kExperimentalRgbKeyboardPatterns);

chrome/browser/ash/crosapi/input_method_test_interface_ash.cc

@@ -43,7 +43,8 @@ bool HasCapability(const base::StringPiece capability) {
   return capability == kInputMethodTestCapabilitySendKeyModifiers ||
          capability == kInputMethodTestCapabilityConfirmComposition ||
          capability == kInputMethodTestCapabilityAlwaysConfirmComposition ||
-         capability == kInputMethodTestCapabilityDeleteSurroundingText;
+         capability == kInputMethodTestCapabilityDeleteSurroundingText ||
+         capability == kInputMethodTestCapabilityExtendedConfirmComposition;
 }
 
 }  // namespace

chrome/browser/lacros/input_method_lacros_browsertest.cc

@@ -49,7 +49,13 @@ struct TestParam {
   // - WaylandKeepSelectionFix (Lacros),
   // - AlwaysConfirmComposition (Ash).
   // - WaylandCancelComposition (Lacros).
+  //
+  // Will not be true if `extended_confirm_composition` is false.
   bool fix_265853952 = false;
+
+  // Enables kExoExtendedConfirmComposition, which uses an extended Wayland API
+  // for ConfirmCompositionText.
+  bool extended_confirm_composition = false;
 };
 
 // Binds an InputMethodTestInterface to Ash-Chrome, which allows these tests to
@@ -544,6 +550,9 @@ class InputMethodLacrosBrowserTest
     if (GetParam().fix_265853952) {
       enabled_ash_features.push_back("AlwaysConfirmComposition");
     }
+    if (GetParam().extended_confirm_composition) {
+      enabled_ash_features.push_back("ExoExtendedConfirmComposition");
+    }
     if (!enabled_ash_features.empty()) {
       StartUniqueAshChrome(
           enabled_ash_features, /*disabled_features=*/{},
@@ -557,10 +566,14 @@ class InputMethodLacrosBrowserTest
   base::test::ScopedFeatureList feature_list_override_;
 };
 
-INSTANTIATE_TEST_SUITE_P(InputMethodLacrosBrowserTestAllParams,
-                         InputMethodLacrosBrowserTest,
-                         ::testing::Values(TestParam{.fix_265853952 = true},
-                                           TestParam{.fix_265853952 = false}));
+INSTANTIATE_TEST_SUITE_P(
+    InputMethodLacrosBrowserTestAllParams,
+    InputMethodLacrosBrowserTest,
+    ::testing::Values(
+        TestParam{.fix_265853952 = true, .extended_confirm_composition = true},
+        TestParam{.fix_265853952 = false, .extended_confirm_composition = true},
+        TestParam{.fix_265853952 = false,
+                  .extended_confirm_composition = false}));
 
 IN_PROC_BROWSER_TEST_P(InputMethodLacrosBrowserTest,
                        FocusingInputFieldSendsFocus) {
@@ -1500,7 +1513,8 @@ IN_PROC_BROWSER_TEST_P(InputMethodLacrosBrowserTest,
           {InputMethodTestInterface::MethodMinVersions::kWaitForFocusMinVersion,
            InputMethodTestInterface::MethodMinVersions::
                kWaitForNextSurroundingTextChangeMinVersion},
-          {kInputMethodTestCapabilityConfirmComposition});
+          {kInputMethodTestCapabilityConfirmComposition,
+           kInputMethodTestCapabilityExtendedConfirmComposition});
   if (!input_method.is_bound()) {
     GTEST_SKIP() << "Unsupported ash version";
   }
@@ -1526,7 +1540,8 @@ IN_PROC_BROWSER_TEST_P(InputMethodLacrosBrowserTest,
           {InputMethodTestInterface::MethodMinVersions::kWaitForFocusMinVersion,
            InputMethodTestInterface::MethodMinVersions::
                kWaitForNextSurroundingTextChangeMinVersion},
-          {kInputMethodTestCapabilityConfirmComposition});
+          {kInputMethodTestCapabilityConfirmComposition,
+           kInputMethodTestCapabilityExtendedConfirmComposition});
   if (!input_method.is_bound()) {
     GTEST_SKIP() << "Unsupported ash version";
   }
@@ -1551,7 +1566,8 @@ IN_PROC_BROWSER_TEST_P(InputMethodLacrosBrowserTest,
           {InputMethodTestInterface::MethodMinVersions::kWaitForFocusMinVersion,
            InputMethodTestInterface::MethodMinVersions::
                kWaitForNextSurroundingTextChangeMinVersion},
-          {kInputMethodTestCapabilityConfirmComposition});
+          {kInputMethodTestCapabilityConfirmComposition,
+           kInputMethodTestCapabilityExtendedConfirmComposition});
   if (!input_method.is_bound()) {
     GTEST_SKIP() << "Unsupported ash version";
   }
@@ -1586,7 +1602,8 @@ IN_PROC_BROWSER_TEST_P(InputMethodLacrosBrowserTest,
            InputMethodTestInterface::MethodMinVersions::
                kWaitForNextSurroundingTextChangeMinVersion},
           {kInputMethodTestCapabilityConfirmComposition,
-           kInputMethodTestCapabilityAlwaysConfirmComposition});
+           kInputMethodTestCapabilityAlwaysConfirmComposition,
+           kInputMethodTestCapabilityExtendedConfirmComposition});
   if (!input_method.is_bound()) {
     GTEST_SKIP() << "Unsupported ash version";
   }

chromeos/crosapi/cpp/input_method_test_interface_constants.h

@@ -30,6 +30,11 @@ COMPONENT_EXPORT(CROSAPI)
 inline constexpr base::StringPiece
     kInputMethodTestCapabilityDeleteSurroundingText = "DeleteSurroundingText";
 
+COMPONENT_EXPORT(CROSAPI)
+inline constexpr base::StringPiece
+    kInputMethodTestCapabilityExtendedConfirmComposition =
+        "ExtendedConfirmComposition";
+
 }  // namespace crosapi
 
 #endif  // CHROMEOS_CROSAPI_CPP_INPUT_METHOD_TEST_INTERFACE_CONSTANTS_H_

components/exo/text_input.cc

@@ -215,14 +215,22 @@ size_t TextInput::ConfirmCompositionText(bool keep_selection) {
   const auto& [surrounding_text, utf16_offset, cursor_pos, composition] =
       predicted_state;
 
-  if (keep_selection && cursor_pos.IsValid() &&
-      cursor_pos.IsBoundedBy(predicted_state.GetSurroundingTextRange())) {
-    delegate_->SetCursor(surrounding_text,
-                         RemoveOffset(cursor_pos, utf16_offset));
+  if (!(base::FeatureList::IsEnabled(
+            ash::features::kExoExtendedConfirmComposition) &&
+        delegate_->ConfirmComposition(keep_selection))) {
+    // Fallback to SetCursor and Commit if ConfirmComposition is not supported.
+    // TODO(b/265853952): Remove once all versions of Lacros supports
+    // ConfirmComposition.
+    if (keep_selection && cursor_pos.IsValid() &&
+        cursor_pos.IsBoundedBy(predicted_state.GetSurroundingTextRange())) {
+      delegate_->SetCursor(surrounding_text,
+                           RemoveOffset(cursor_pos, utf16_offset));
+    }
+
+    delegate_->Commit(
+        predicted_state.GetCompositionText().value_or(base::StringPiece16()));
   }
 
-  delegate_->Commit(
-      predicted_state.GetCompositionText().value_or(base::StringPiece16()));
   // Preserve the result value before updating the tracker's state.
   const size_t composition_text_length = composition.length();
   surrounding_text_tracker_.OnConfirmCompositionText(keep_selection);

components/exo/text_input.h

@@ -123,6 +123,12 @@ class TextInput : public ui::TextInputClient,
     // |surrounding_text|. All offsets are in UTF16, and must be valid.
     virtual void SetAutocorrectRange(base::StringPiece16 surrounding_text,
                                      const gfx::Range& range) = 0;
+
+    // Commits the current composition text.
+    // If `keep_selection` is true, keep the selection range unchanged.
+    // Otherwise, set the selection range to be after the committed text.
+    // Returns whether the operation is supported by the client.
+    virtual bool ConfirmComposition(bool keep_selection) = 0;
   };
 
   explicit TextInput(std::unique_ptr<Delegate> delegate);

components/exo/text_input_unittest.cc

@@ -102,6 +102,7 @@ class MockTextInputDelegate : public TextInput::Delegate {
               SetAutocorrectRange,
               (base::StringPiece16, const gfx::Range&),
               (override));
+  MOCK_METHOD(bool, ConfirmComposition, (bool), (override));
 };
 
 class TestingInputMethodObserver : public ui::InputMethodObserver {
@@ -249,6 +250,31 @@ class TextInputTestWithConsumedByIme
 
 INSTANTIATE_TEST_SUITE_P(, TextInputTestWithConsumedByIme, ::testing::Bool());
 
+// Test for both kExoExtendedConfirmComposition enabled and disabled.
+class TextInputTestWithExtendedConfirmComposition
+    : public TextInputTest,
+      public testing::WithParamInterface<bool> {
+ public:
+  void SetUp() override {
+    if (GetParam()) {
+      feature_list_.InitAndEnableFeature(
+          ash::features::kExoExtendedConfirmComposition);
+    } else {
+      feature_list_.InitAndDisableFeature(
+          ash::features::kExoExtendedConfirmComposition);
+    }
+
+    TextInputTest::SetUp();
+  }
+
+ private:
+  base::test::ScopedFeatureList feature_list_;
+};
+
+INSTANTIATE_TEST_SUITE_P(,
+                         TextInputTestWithExtendedConfirmComposition,
+                         ::testing::Bool());
+
 TEST_F(TextInputTest, Activate) {
   EXPECT_EQ(ui::TEXT_INPUT_TYPE_NONE, text_input()->GetTextInputType());
   EXPECT_EQ(ui::TEXT_INPUT_MODE_DEFAULT, text_input()->GetTextInputMode());
@@ -533,11 +559,16 @@ TEST_F(TextInputTest, CompositionTextEmpty) {
   text_input()->ClearCompositionText();
 }
 
-TEST_F(TextInputTest, ConfirmCompositionText) {
+TEST_P(TextInputTestWithExtendedConfirmComposition,
+       ConfirmCompositionTextDontKeepSelection) {
   SetCompositionText(u"composition");
 
-  EXPECT_CALL(*delegate(), Commit(base::StringPiece16(u"composition")))
-      .Times(1);
+  if (GetParam()) {
+    EXPECT_CALL(*delegate(), ConfirmComposition(/*keep_selection=*/false))
+        .Times(1);
+  } else {
+    EXPECT_CALL(*delegate(), Commit(base::StringPiece16(u"composition")));
+  }
   const size_t composition_text_length =
       text_input()->ConfirmCompositionText(/*keep_selection=*/false);
   EXPECT_EQ(composition_text_length, 11u);
@@ -548,18 +579,23 @@ TEST_F(TextInputTest, ConfirmCompositionText) {
   EXPECT_FALSE(text_input()->HasCompositionText());
 }
 
-TEST_F(TextInputTest, ConfirmCompositionTextKeepSelection) {
+TEST_P(TextInputTestWithExtendedConfirmComposition,
+       ConfirmCompositionTextKeepSelection) {
   constexpr char16_t kCompositionText[] = u"composition";
   SetCompositionText(kCompositionText);
   text_input()->SetEditableSelectionRange(gfx::Range(2, 3));
   text_input()->SetSurroundingText(kCompositionText, 0u, gfx::Range(2, 3),
                                    absl::nullopt, absl::nullopt);
 
-  EXPECT_CALL(*delegate(), SetCursor(base::StringPiece16(kCompositionText),
-                                     gfx::Range(2, 3)))
-      .Times(1);
-  EXPECT_CALL(*delegate(), Commit(base::StringPiece16(kCompositionText)))
-      .Times(1);
+  if (GetParam()) {
+    EXPECT_CALL(*delegate(), ConfirmComposition(/*keep_selection=*/true))
+        .Times(1);
+  } else {
+    EXPECT_CALL(*delegate(), SetCursor(base::StringPiece16(kCompositionText),
+                                       gfx::Range(2, 3)))
+        .Times(1);
+    EXPECT_CALL(*delegate(), Commit(base::StringPiece16(kCompositionText)));
+  }
   const uint32_t composition_text_length =
       text_input()->ConfirmCompositionText(/*keep_selection=*/true);
   EXPECT_EQ(composition_text_length, static_cast<uint32_t>(11));

components/exo/wayland/server.cc

@@ -154,6 +154,18 @@ void wayland_log(const char* fmt, va_list argp) {
   LOG(WARNING) << "libwayland: " << base::StringPrintV(fmt, argp);
 }
 
+int GetTextInputExtensionV1Version() {
+  if (base::FeatureList::IsEnabled(
+          ash::features::kExoExtendedConfirmComposition) &&
+      base::FeatureList::IsEnabled(ash::features::kExoSurroundingTextOffset)) {
+    return 11;
+  }
+  if (base::FeatureList::IsEnabled(ash::features::kExoSurroundingTextOffset)) {
+    return 10;
+  }
+  return 9;
+}
+
 }  // namespace
 
 bool Server::Open() {
@@ -382,12 +394,10 @@ void Server::Initialize() {
 
   zcr_text_input_extension_data_ =
       std::make_unique<WaylandTextInputExtension>();
-  wl_global_create(
-      wl_display_.get(), &zcr_text_input_extension_v1_interface,
-      base::FeatureList::IsEnabled(ash::features::kExoSurroundingTextOffset)
-          ? 10
-          : 9,
-      zcr_text_input_extension_data_.get(), bind_text_input_extension);
+  wl_global_create(wl_display_.get(), &zcr_text_input_extension_v1_interface,
+                   GetTextInputExtensionV1Version(),
+                   zcr_text_input_extension_data_.get(),
+                   bind_text_input_extension);
 
   zxdg_shell_data_ =
       std::make_unique<WaylandZxdgShell>(display_, serial_tracker_.get());

components/exo/wayland/zwp_text_input_manager.cc

@@ -402,6 +402,25 @@ class WaylandTextInputDelegate : public TextInput::Delegate {
     }
   }
 
+  bool ConfirmComposition(bool keep_selection) override {
+    if (!extended_text_input_) {
+      return false;
+    }
+
+    if (wl_resource_get_version(extended_text_input_) <
+        ZCR_EXTENDED_TEXT_INPUT_V1_CONFIRM_PREEDIT_SINCE_VERSION) {
+      return false;
+    }
+
+    zcr_extended_text_input_v1_send_confirm_preedit(
+        extended_text_input_,
+        keep_selection
+            ? ZCR_EXTENDED_TEXT_INPUT_V1_CONFIRM_PREEDIT_SELECTION_BEHAVIOR_UNCHANGED
+            : ZCR_EXTENDED_TEXT_INPUT_V1_CONFIRM_PREEDIT_SELECTION_BEHAVIOR_AFTER_PREEDIT);
+    wl_client_flush(client());
+    return true;
+  }
+
   raw_ptr<wl_resource, ExperimentalAsh> text_input_;
   raw_ptr<wl_resource, ExperimentalAsh> extended_text_input_ = nullptr;
   raw_ptr<wl_resource, ExperimentalAsh> surface_ = nullptr;

third_party/wayland-protocols/unstable/text-input/text-input-extension-unstable-v1.xml

@@ -24,7 +24,7 @@
     DEALINGS IN THE SOFTWARE.
   </copyright>
 
-  <interface name="zcr_text_input_extension_v1" version="10">
+  <interface name="zcr_text_input_extension_v1" version="11">
     <description summary="extends text_input to support richer operations">
       Allows a text_input to sends more variation of operations to support
       richer features, such as set_preedit_region.
@@ -57,7 +57,7 @@
 
   </interface>
 
-  <interface name="zcr_extended_text_input_v1" version="10">
+  <interface name="zcr_extended_text_input_v1" version="11">
     <description summary="extension of text_input protocol">
       The zcr_extended_text_input_v1 interface extends the text_input interface
       to support more rich operations on text_input.
@@ -392,5 +392,21 @@
       <arg name="offset_utf16" type="uint"/>
     </request>
 
+    <!-- Version 11 -->
+
+    <enum name="confirm_preedit_selection_behavior" since="11">
+      <description summary="How the selection range is affected by confirm_preedit"></description>
+      <entry name="after_preedit" value="0" summary="The cursor is moved to the end of the committed preedit text, if any."/>
+      <entry name="unchanged" value="1" summary="The selection range is not affected at all."/>
+    </enum>
+
+    <event name="confirm_preedit" since="11">
+      <description summary="Commits the current preedit">
+        Commits the current preedit and modify the selection range according to selection_behavior.
+        Has no effect if there's no preedit text.
+      </description>
+      <arg name="selection_behavior" type="uint" enum="confirm_preedit_selection_behavior" />
+    </event>
+
   </interface>
 </protocol>

ui/base/ime/linux/input_method_auralinux_unittest.cc

@@ -1214,6 +1214,28 @@ TEST_F(InputMethodAuraLinuxTest, OnSetVirtualKeyboardOccludedBounds) {
   RemoveLastClient(client.get());
 }
 
+TEST_F(InputMethodAuraLinuxTest, OnConfirmCompositionText) {
+  auto client =
+      std::make_unique<TextInputClientForTesting>(TEXT_INPUT_TYPE_TEXT);
+  InstallFirstClient(client.get());
+
+  input_method_auralinux_->OnPreeditStart();
+  CompositionText comp;
+  comp.text = u"a";
+  input_method_auralinux_->OnPreeditChanged(comp);
+
+  test_result_->ExpectAction("compositionstart");
+  test_result_->ExpectAction("compositionupdate:a");
+  test_result_->Verify();
+
+  input_method_auralinux_->OnConfirmCompositionText(/*keep_selection=*/true);
+
+  test_result_->ExpectAction("compositionend");
+  test_result_->ExpectAction("textinput:a");
+
+  RemoveLastClient(client.get());
+}
+
 TEST_F(InputMethodAuraLinuxTest, GetVirtualKeyboardController) {
   EXPECT_EQ(input_method_auralinux_->GetVirtualKeyboardController(),
             context_->GetVirtualKeyboardController());

ui/ozone/platform/wayland/host/wayland_connection.cc

@@ -80,7 +80,7 @@ constexpr uint32_t kMaxXdgShellVersion = 5;
 constexpr uint32_t kMaxWpPresentationVersion = 1;
 constexpr uint32_t kMaxWpViewporterVersion = 1;
 constexpr uint32_t kMaxTextInputManagerVersion = 1;
-constexpr uint32_t kMaxTextInputExtensionVersion = 10;
+constexpr uint32_t kMaxTextInputExtensionVersion = 11;
 constexpr uint32_t kMaxExplicitSyncVersion = 2;
 constexpr uint32_t kMaxAlphaCompositingVersion = 1;
 constexpr uint32_t kMaxXdgDecorationVersion = 1;