From a5f2a1a5e1af2ab96a9d8dee3c45f1648b9e4b19 Mon Sep 17 00:00:00 2001 From: Harlen Batagelo Date: Wed, 16 Oct 2024 17:19:49 -0300 Subject: [PATCH] Handle out-of-order add/remove pointer events (#55740) Fixes [#146251](https://github.com/flutter/flutter/issues/146251). On Windows, a "pointer add" event to a new view is sometimes sent before the "pointer remove" event from the old view. When `PointerDataPacketConverter` handles the add event, it asserts that the device's pointer state has been cleared. Since the pointer state is cleared only when the remove event is processed, the assertion fails if the events arrive out of order. The solution proposed here is to synthesize a remove event if an add event is received while the pointer state hasn't been cleared (i.e., if the pointer is added without being removed from the previous view). To avoid duplicate remove events, the view ID is now tracked in `PointerState`. If the original "remove" arrives after the synthesized one, the state's view ID will differ from the pointer data's view ID, allowing it to be safely ignored. When synthesizing the remove event, it's possible that the old view has already been destroyed, meaning no remove event will be received later. For example, this occurs when a window is destroyed while the pointer is inside it. However, the framework expects an "add" event to always follow a "remove" event, and "remove" events with invalid views are ignored. To meet the framework's expectations, the view ID of the "add" event will be used for the "remove" event if the old view has been destroyed. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- .../window/pointer_data_packet_converter.cc | 46 +++++++- lib/ui/window/pointer_data_packet_converter.h | 1 + ...pointer_data_packet_converter_unittests.cc | 102 ++++++++++++++++++ 3 files changed, 148 insertions(+), 1 deletion(-) diff --git a/lib/ui/window/pointer_data_packet_converter.cc b/lib/ui/window/pointer_data_packet_converter.cc index 45b05a00dd852..b7640288fcc7e 100644 --- a/lib/ui/window/pointer_data_packet_converter.cc +++ b/lib/ui/window/pointer_data_packet_converter.cc @@ -74,7 +74,45 @@ void PointerDataPacketConverter::ConvertPointerData( break; } case PointerData::Change::kAdd: { - FML_DCHECK(states_.find(pointer_data.device) == states_.end()); + auto iter = states_.find(pointer_data.device); + if (iter != states_.end()) { + // Synthesizes a remove event if the pointer was not previously + // removed. + PointerState state = iter->second; + PointerData synthesized_data = pointer_data; + synthesized_data.physical_x = state.physical_x; + synthesized_data.physical_y = state.physical_y; + synthesized_data.pan_x = state.pan_x; + synthesized_data.pan_y = state.pan_y; + synthesized_data.scale = state.scale; + synthesized_data.rotation = state.rotation; + synthesized_data.buttons = state.buttons; + synthesized_data.synthesized = 1; + + // The framework expects an add event to always follow a remove event, + // and remove events with invalid views are ignored. + // To meet the framework's expectations, the view ID of the add event + // is used for the remove event if the old view has been destroyed. + if (delegate_.ViewExists(state.view_id)) { + synthesized_data.view_id = state.view_id; + + if (state.is_down) { + // Synthesizes cancel event if the pointer is down. + PointerData synthesized_cancel_event = synthesized_data; + synthesized_cancel_event.change = PointerData::Change::kCancel; + UpdatePointerIdentifier(synthesized_cancel_event, state, false); + + state.is_down = false; + converted_pointers.push_back(synthesized_cancel_event); + } + } + + PointerData synthesized_remove_event = synthesized_data; + synthesized_remove_event.change = PointerData::Change::kRemove; + + converted_pointers.push_back(synthesized_remove_event); + } + EnsurePointerState(pointer_data); converted_pointers.push_back(pointer_data); break; @@ -84,6 +122,11 @@ void PointerDataPacketConverter::ConvertPointerData( auto iter = states_.find(pointer_data.device); FML_DCHECK(iter != states_.end()); PointerState state = iter->second; + if (state.view_id != pointer_data.view_id) { + // Ignores remove event if the pointer was previously added to a + // different view. + break; + } if (state.is_down) { // Synthesizes cancel event if the pointer is down. @@ -362,6 +405,7 @@ PointerState PointerDataPacketConverter::EnsurePointerState( state.physical_y = pointer_data.physical_y; state.pan_x = 0; state.pan_y = 0; + state.view_id = pointer_data.view_id; states_[pointer_data.device] = state; return state; } diff --git a/lib/ui/window/pointer_data_packet_converter.h b/lib/ui/window/pointer_data_packet_converter.h index 146ade258a4a4..7b1b823f53dc9 100644 --- a/lib/ui/window/pointer_data_packet_converter.h +++ b/lib/ui/window/pointer_data_packet_converter.h @@ -38,6 +38,7 @@ struct PointerState { double scale; double rotation; int64_t buttons; + int64_t view_id; }; //------------------------------------------------------------------------------ diff --git a/lib/ui/window/pointer_data_packet_converter_unittests.cc b/lib/ui/window/pointer_data_packet_converter_unittests.cc index 4f6a37f4536d8..0eb9667e2c707 100644 --- a/lib/ui/window/pointer_data_packet_converter_unittests.cc +++ b/lib/ui/window/pointer_data_packet_converter_unittests.cc @@ -510,6 +510,108 @@ TEST(PointerDataPacketConverterTest, CanSynthesizeAdd) { ASSERT_EQ(result[3].buttons, 0); } +TEST(PointerDataPacketConverterTest, CanSynthesizeRemove) { + TestDelegate delegate; + delegate.AddView(100); + delegate.AddView(200); + PointerDataPacketConverter converter(delegate); + auto packet = std::make_unique(3); + + PointerData data; + CreateSimulatedPointerData(data, PointerData::Change::kAdd, 0, 0.0, 0.0, 0); + data.view_id = 100; + packet->SetPointerData(0, data); + CreateSimulatedPointerData(data, PointerData::Change::kDown, 0, 3.0, 4.0, 1); + data.view_id = 100; + packet->SetPointerData(1, data); + CreateSimulatedPointerData(data, PointerData::Change::kAdd, 0, 0.0, 0.0, 0); + data.view_id = 200; + packet->SetPointerData(2, data); + auto converted_packet = converter.Convert(*packet); + + std::vector result; + UnpackPointerPacket(result, std::move(converted_packet)); + + ASSERT_EQ(result.size(), (size_t)6); + ASSERT_EQ(result[0].synthesized, 0); + ASSERT_EQ(result[0].view_id, 100); + + // A hover should be synthesized. + ASSERT_EQ(result[1].change, PointerData::Change::kHover); + ASSERT_EQ(result[1].synthesized, 1); + ASSERT_EQ(result[1].physical_delta_x, 3.0); + ASSERT_EQ(result[1].physical_delta_y, 4.0); + ASSERT_EQ(result[1].buttons, 0); + + ASSERT_EQ(result[2].change, PointerData::Change::kDown); + ASSERT_EQ(result[2].pointer_identifier, 1); + ASSERT_EQ(result[2].synthesized, 0); + ASSERT_EQ(result[2].buttons, 1); + + // A cancel should be synthesized. + ASSERT_EQ(result[3].change, PointerData::Change::kCancel); + ASSERT_EQ(result[3].pointer_identifier, 1); + ASSERT_EQ(result[3].synthesized, 1); + ASSERT_EQ(result[3].physical_x, 3.0); + ASSERT_EQ(result[3].physical_y, 4.0); + ASSERT_EQ(result[3].buttons, 1); + + // A remove should be synthesized. + ASSERT_EQ(result[4].physical_x, 3.0); + ASSERT_EQ(result[4].physical_y, 4.0); + ASSERT_EQ(result[4].synthesized, 1); + ASSERT_EQ(result[4].view_id, 100); + + ASSERT_EQ(result[5].synthesized, 0); + ASSERT_EQ(result[5].view_id, 200); +} + +TEST(PointerDataPacketConverterTest, + CanAvoidDoubleRemoveAfterSynthesizedRemove) { + TestDelegate delegate; + delegate.AddView(100); + delegate.AddView(200); + PointerDataPacketConverter converter(delegate); + auto packet = std::make_unique(2); + + PointerData data; + CreateSimulatedPointerData(data, PointerData::Change::kAdd, 0, 0.0, 0.0, 0); + data.view_id = 100; + packet->SetPointerData(0, data); + CreateSimulatedPointerData(data, PointerData::Change::kAdd, 0, 0.0, 0.0, 0); + data.view_id = 200; + packet->SetPointerData(1, data); + auto converted_packet = converter.Convert(*packet); + + std::vector result; + UnpackPointerPacket(result, std::move(converted_packet)); + + ASSERT_EQ(result.size(), (size_t)3); + ASSERT_EQ(result[0].synthesized, 0); + ASSERT_EQ(result[0].view_id, 100); + + // A remove should be synthesized. + ASSERT_EQ(result[1].synthesized, 1); + ASSERT_EQ(result[1].view_id, 100); + + ASSERT_EQ(result[2].synthesized, 0); + ASSERT_EQ(result[2].view_id, 200); + + // Simulate a double remove. + packet = std::make_unique(1); + CreateSimulatedPointerData(data, PointerData::Change::kRemove, 0, 0.0, 0.0, + 0); + data.view_id = 100; + packet->SetPointerData(0, data); + converted_packet = converter.Convert(*packet); + + result.clear(); + UnpackPointerPacket(result, std::move(converted_packet)); + + // The double remove should be ignored. + ASSERT_EQ(result.size(), (size_t)0); +} + TEST(PointerDataPacketConverterTest, CanHandleThreeFingerGesture) { // Regression test https://github.com/flutter/flutter/issues/20517. TestDelegate delegate;