From eb139936eb2c17694aba3fd922adfe8a85c5e8df Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Fri, 3 Jan 2020 09:21:04 -0800 Subject: [PATCH] [web] Fix right click issues (#15103) --- .../lib/src/engine/pointer_binding.dart | 101 +++-- .../lib/src/engine/pointer_converter.dart | 65 ++- .../test/engine/pointer_binding_test.dart | 389 +++++++++++++++++- 3 files changed, 513 insertions(+), 42 deletions(-) diff --git a/lib/web_ui/lib/src/engine/pointer_binding.dart b/lib/web_ui/lib/src/engine/pointer_binding.dart index d7fb941d4..30e55f90e 100644 --- a/lib/web_ui/lib/src/engine/pointer_binding.dart +++ b/lib/web_ui/lib/src/engine/pointer_binding.dart @@ -288,7 +288,7 @@ class _SanitizedDetails { class _ButtonSanitizer { int _pressedButtons = 0; - // Transform html.PointerEvent.buttons to Flutter's PointerEvent buttons. + /// Transform [html.PointerEvent.buttons] to Flutter's PointerEvent buttons. int _htmlButtonsToFlutterButtons(int buttons) { // Flutter's button definition conveniently matches that of JavaScript // from primary button (0x1) to forward button (0x10), which allows us to @@ -296,33 +296,73 @@ class _ButtonSanitizer { return buttons & _kButtonsMask; } - List<_SanitizedDetails> sanitizeDownEvent({@required int buttons}) { - final List<_SanitizedDetails> result = <_SanitizedDetails>[]; - // TODO(flutter_web): Remove this temporary fix for right click - // on web platform once context gesture is implemented. + /// Given [html.PointerEvent.button] and [html.PointerEvent.buttons], tries to + /// infer the correct value for Flutter buttons. + int _inferDownFlutterButtons(int button, int buttons) { + if (buttons == 0 && button > -1) { + // In some cases, the browser sends `buttons:0` in a down event. In such + // case, we try to infer the value from `button`. + buttons = convertButtonToButtons(button); + } + return _htmlButtonsToFlutterButtons(buttons); + } + + List<_SanitizedDetails> sanitizeDownEvent({ + @required int button, + @required int buttons, + }) { + // If the pointer is already down, we just send a move event with the new + // `buttons` value. if (_pressedButtons != 0) { - _pressedButtons = 0; - result.add(_SanitizedDetails( - change: ui.PointerChange.up, - buttons: 0, - )); + return sanitizeMoveEvent(buttons: buttons); } - _pressedButtons = _htmlButtonsToFlutterButtons(buttons); - result.add(_SanitizedDetails( - change: ui.PointerChange.down, - buttons: _pressedButtons, - )); - return result; + + _pressedButtons = _inferDownFlutterButtons(button, buttons); + return <_SanitizedDetails>[ + _SanitizedDetails( + change: ui.PointerChange.down, + buttons: _pressedButtons, + ) + ]; } List<_SanitizedDetails> sanitizeMoveEvent({@required int buttons}) { - _pressedButtons = _htmlButtonsToFlutterButtons(buttons); - return <_SanitizedDetails>[_SanitizedDetails( - change: _pressedButtons == 0 - ? ui.PointerChange.hover - : ui.PointerChange.move, - buttons: _pressedButtons, - )]; + final int newPressedButtons = _htmlButtonsToFlutterButtons(buttons); + // This could happen when the context menu is active and the user clicks + // RMB somewhere else. The browser sends a down event with `buttons:0`. + // + // In this case, we keep the old `buttons` value so we don't confuse the + // framework. + if (_pressedButtons != 0 && newPressedButtons == 0) { + return <_SanitizedDetails>[ + _SanitizedDetails( + change: ui.PointerChange.move, + buttons: _pressedButtons, + ) + ]; + } + + // This could happen when the user clicks RMB then moves the mouse quickly. + // The brower sends a move event with `buttons:2` even though there's no + // buttons down yet. + if (_pressedButtons == 0 && newPressedButtons != 0) { + return <_SanitizedDetails>[ + _SanitizedDetails( + change: ui.PointerChange.hover, + buttons: _pressedButtons, + ) + ]; + } + + _pressedButtons = newPressedButtons; + return <_SanitizedDetails>[ + _SanitizedDetails( + change: _pressedButtons == 0 + ? ui.PointerChange.hover + : ui.PointerChange.move, + buttons: _pressedButtons, + ) + ]; } List<_SanitizedDetails> sanitizeUpEvent() { @@ -396,7 +436,11 @@ class _PointerAdapter extends _BaseAdapter with _WheelEventListenerMixin { _addPointerEventListener('pointerdown', (html.PointerEvent event) { final int device = event.pointerId; final List pointerData = []; - final List<_SanitizedDetails> detailsList = _ensureSanitizer(device).sanitizeDownEvent(buttons: event.buttons); + final List<_SanitizedDetails> detailsList = + _ensureSanitizer(device).sanitizeDownEvent( + button: event.button, + buttons: event.buttons, + ); _convertEventsToPointerData(data: pointerData, event: event, detailsList: detailsList); _callback(pointerData); }); @@ -697,10 +741,11 @@ class _MouseAdapter extends _BaseAdapter with _WheelEventListenerMixin { void setup() { _addMouseEventListener('mousedown', (html.MouseEvent event) { final List pointerData = []; - final bool isStartOfDrag = event.buttons == convertButtonToButtons(event.button); - final List<_SanitizedDetails> sanitizedDetails = isStartOfDrag ? - _sanitizer.sanitizeDownEvent(buttons: event.buttons) : - _sanitizer.sanitizeMoveEvent(buttons: event.buttons); + final List<_SanitizedDetails> sanitizedDetails = + _sanitizer.sanitizeDownEvent( + button: event.button, + buttons: event.buttons, + ); _convertEventsToPointerData(data: pointerData, event: event, detailsList: sanitizedDetails); _callback(pointerData); }); diff --git a/lib/web_ui/lib/src/engine/pointer_converter.dart b/lib/web_ui/lib/src/engine/pointer_converter.dart index 6bd4e7c8a..584dc990f 100644 --- a/lib/web_ui/lib/src/engine/pointer_converter.dart +++ b/lib/web_ui/lib/src/engine/pointer_converter.dart @@ -364,7 +364,38 @@ class PointerDataConverter { ) ); } - assert(!_locationHasChanged(device, physicalX, physicalY)); + if (_locationHasChanged(device, physicalX, physicalY)) { + assert(alreadyAdded); + // Synthesize a hover of the pointer to the down location before + // sending the down event, if necessary. + result.add( + _synthesizePointerData( + timeStamp: timeStamp, + change: ui.PointerChange.hover, + kind: kind, + device: device, + physicalX: physicalX, + physicalY: physicalY, + buttons: 0, + obscured: obscured, + pressure: 0.0, + pressureMin: pressureMin, + pressureMax: pressureMax, + distance: distance, + distanceMax: distanceMax, + size: size, + radiusMajor: radiusMajor, + radiusMinor: radiusMinor, + radiusMin: radiusMin, + radiusMax: radiusMax, + orientation: orientation, + tilt: tilt, + platformData: platformData, + scrollDeltaX: scrollDeltaX, + scrollDeltaY: scrollDeltaY, + ) + ); + } state.down = true; result.add( _generateCompletePointerData( @@ -442,7 +473,37 @@ class PointerDataConverter { physicalX = state.x; physicalY = state.y; } - assert(!_locationHasChanged(device, physicalX, physicalY)); + if (_locationHasChanged(device, physicalX, physicalY)) { + // Synthesize a move of the pointer to the up location before + // sending the up event, if necessary. + result.add( + _synthesizePointerData( + timeStamp: timeStamp, + change: ui.PointerChange.move, + kind: kind, + device: device, + physicalX: physicalX, + physicalY: physicalY, + buttons: buttons, + obscured: obscured, + pressure: pressure, + pressureMin: pressureMin, + pressureMax: pressureMax, + distance: distance, + distanceMax: distanceMax, + size: size, + radiusMajor: radiusMajor, + radiusMinor: radiusMinor, + radiusMin: radiusMin, + radiusMax: radiusMax, + orientation: orientation, + tilt: tilt, + platformData: platformData, + scrollDeltaX: scrollDeltaX, + scrollDeltaY: scrollDeltaY, + ) + ); + } state.down = false; result.add( _generateCompletePointerData( diff --git a/lib/web_ui/test/engine/pointer_binding_test.dart b/lib/web_ui/test/engine/pointer_binding_test.dart index 6ba4bed7f..fea3f7979 100644 --- a/lib/web_ui/test/engine/pointer_binding_test.dart +++ b/lib/web_ui/test/engine/pointer_binding_test.dart @@ -376,7 +376,7 @@ void main() { // ALL ADAPTERS - _testEach( + _testEach<_BasicEventContext>( [_PointerEventContext(), _MouseEventContext(), _TouchEventContext()], 'can receive pointer events on the glass pane', (_BasicEventContext context) { @@ -393,7 +393,7 @@ void main() { }, ); - _testEach( + _testEach<_BasicEventContext>( [_PointerEventContext(), _MouseEventContext(), _TouchEventContext()], 'does create an add event if got a pointerdown', (_BasicEventContext context) { @@ -438,7 +438,7 @@ void main() { _testEach<_ButtonedEventMixin>( [_PointerEventContext(), _MouseEventContext()], - 'synthesizes a pointerup event on two pointerdowns in a row', + 'sends a pointermove event instead of the second pointerdown in a row', (_ButtonedEventMixin context) { PointerBinding.instance.debugOverrideDetector(context); List packets = []; @@ -446,24 +446,33 @@ void main() { packets.add(packet); }; - glassPane.dispatchEvent(context.primaryDown()); - - glassPane.dispatchEvent(context.primaryDown()); - - expect(packets, hasLength(2)); + glassPane.dispatchEvent(context.primaryDown( + clientX: 10.0, + clientY: 10.0, + )); + expect(packets, hasLength(1)); // An add will be synthesized. expect(packets[0].data, hasLength(2)); expect(packets[0].data[0].change, equals(ui.PointerChange.add)); expect(packets[0].data[0].synthesized, equals(true)); expect(packets[0].data[1].change, equals(ui.PointerChange.down)); - expect(packets[1].data[0].change, equals(ui.PointerChange.up)); - expect(packets[1].data[1].change, equals(ui.PointerChange.down)); + packets.clear(); + + glassPane.dispatchEvent(context.primaryDown( + clientX: 20.0, + clientY: 20.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(1)); + expect(packets[0].data[0].change, equals(ui.PointerChange.move)); + expect(packets[0].data[0].buttons, equals(1)); + packets.clear(); }, ); _testEach<_ButtonedEventMixin>( [_PointerEventContext(), _MouseEventContext()], - 'does synthesize add or hover or more for scroll', + 'does synthesize add or hover or move for scroll', (_ButtonedEventMixin context) { PointerBinding.instance.debugOverrideDetector(context); List packets = []; @@ -1080,6 +1089,362 @@ void main() { }, ); + _testEach<_ButtonedEventMixin>( + [_PointerEventContext(), _MouseEventContext()], + 'handles RMB click when the browser sends it as a move', + (_ButtonedEventMixin context) { + PointerBinding.instance.debugOverrideDetector(context); + // When the user clicks the RMB and moves the mouse quickly (before the + // context menu shows up), the browser sends a move event before down. + // The move event will have "button:-1, buttons:2". + + List packets = []; + ui.window.onPointerDataPacket = (ui.PointerDataPacket packet) { + packets.add(packet); + }; + + // Press RMB and hold, popping up the context menu. + glassPane.dispatchEvent(context.mouseMove( + button: -1, + buttons: 2, + clientX: 10.0, + clientY: 10.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(2)); + expect(packets[0].data[0].change, equals(ui.PointerChange.add)); + expect(packets[0].data[0].synthesized, equals(true)); + + expect(packets[0].data[1].change, equals(ui.PointerChange.hover)); + expect(packets[0].data[1].synthesized, equals(false)); + expect(packets[0].data[1].buttons, equals(0)); + packets.clear(); + }, + ); + + _testEach<_ButtonedEventMixin>( + [_PointerEventContext(), _MouseEventContext()], + 'correctly handles hover after RMB click', + (_ButtonedEventMixin context) { + PointerBinding.instance.debugOverrideDetector(context); + // This can happen with the following gesture sequence: + // + // - Pops up the context menu by right clicking, but holds RMB; + // - Move the pointer to hover. + + List packets = []; + ui.window.onPointerDataPacket = (ui.PointerDataPacket packet) { + packets.add(packet); + }; + + // Press RMB and hold, popping up the context menu. + glassPane.dispatchEvent(context.mouseDown( + button: 2, + buttons: 2, + clientX: 10.0, + clientY: 10.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(2)); + expect(packets[0].data[0].change, equals(ui.PointerChange.add)); + expect(packets[0].data[0].synthesized, equals(true)); + + expect(packets[0].data[1].change, equals(ui.PointerChange.down)); + expect(packets[0].data[1].synthesized, equals(false)); + expect(packets[0].data[1].buttons, equals(2)); + packets.clear(); + + // Move the mouse. The event will have "buttons: 0" because RMB was + // released but the browser didn't send a pointerup/mouseup event. + // The hover is also triggered at a different position. + glassPane.dispatchEvent(context.hover( + clientX: 20.0, + clientY: 20.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(1)); + expect(packets[0].data[0].change, equals(ui.PointerChange.move)); + expect(packets[0].data[0].synthesized, equals(false)); + expect(packets[0].data[0].buttons, equals(2)); + packets.clear(); + }, + ); + + _testEach<_ButtonedEventMixin>( + [_PointerEventContext(), _MouseEventContext()], + 'correctly handles LMB click after RMB click', + (_ButtonedEventMixin context) { + PointerBinding.instance.debugOverrideDetector(context); + // This can happen with the following gesture sequence: + // + // - Pops up the context menu by right clicking, but holds RMB; + // - Clicks LMB in a different location; + // - Release LMB. + // + // The LMB click occurs in a different location because when RMB is + // clicked, and the contextmenu is shown, the browser stops sending + // `pointermove`/`mousemove` events. Then when the LMB click comes in, it + // could be in a different location without any `*move` events in between. + + List packets = []; + ui.window.onPointerDataPacket = (ui.PointerDataPacket packet) { + packets.add(packet); + }; + + // Press RMB and hold, popping up the context menu. + glassPane.dispatchEvent(context.mouseDown( + button: 2, + buttons: 2, + clientX: 10.0, + clientY: 10.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(2)); + expect(packets[0].data[0].change, equals(ui.PointerChange.add)); + expect(packets[0].data[0].synthesized, equals(true)); + + expect(packets[0].data[1].change, equals(ui.PointerChange.down)); + expect(packets[0].data[1].synthesized, equals(false)); + expect(packets[0].data[1].buttons, equals(2)); + packets.clear(); + + // Press LMB. + glassPane.dispatchEvent(context.mouseDown( + button: 0, + buttons: 3, + clientX: 20.0, + clientY: 20.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(1)); + expect(packets[0].data[0].change, equals(ui.PointerChange.move)); + expect(packets[0].data[0].synthesized, equals(false)); + expect(packets[0].data[0].buttons, equals(3)); + packets.clear(); + + // Release LMB. + glassPane.dispatchEvent(context.primaryUp( + clientX: 20.0, + clientY: 20.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(1)); + expect(packets[0].data[0].change, equals(ui.PointerChange.up)); + expect(packets[0].data[0].synthesized, equals(false)); + expect(packets[0].data[0].buttons, equals(0)); + packets.clear(); + }, + ); + + _testEach<_ButtonedEventMixin>( + [_PointerEventContext(), _MouseEventContext()], + 'correctly handles two consecutive RMB clicks with no up in between', + (_ButtonedEventMixin context) { + PointerBinding.instance.debugOverrideDetector(context); + // This can happen with the following gesture sequence: + // + // - Pops up the context menu by right clicking, but holds RMB; + // - Clicks RMB again in a different location; + + List packets = []; + ui.window.onPointerDataPacket = (ui.PointerDataPacket packet) { + packets.add(packet); + }; + + // Press RMB and hold, popping up the context menu. + glassPane.dispatchEvent(context.mouseDown( + button: 2, + buttons: 2, + clientX: 10.0, + clientY: 10.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(2)); + expect(packets[0].data[0].change, equals(ui.PointerChange.add)); + expect(packets[0].data[0].synthesized, equals(true)); + expect(packets[0].data[1].change, equals(ui.PointerChange.down)); + expect(packets[0].data[1].synthesized, equals(false)); + expect(packets[0].data[1].buttons, equals(2)); + packets.clear(); + + // Press RMB again. In Chrome, when RMB is clicked again while the + // context menu is still active, it sends a pointerdown/mousedown event + // with "buttons:0". + glassPane.dispatchEvent(context.mouseDown( + button: 2, + buttons: 0, + clientX: 20.0, + clientY: 20.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(1)); + expect(packets[0].data[0].change, equals(ui.PointerChange.move)); + expect(packets[0].data[0].synthesized, equals(false)); + expect(packets[0].data[0].buttons, equals(2)); + packets.clear(); + + // Release RMB. + glassPane.dispatchEvent(context.mouseUp( + button: 2, + clientX: 20.0, + clientY: 20.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(1)); + expect(packets[0].data[0].change, equals(ui.PointerChange.up)); + expect(packets[0].data[0].synthesized, equals(false)); + expect(packets[0].data[0].buttons, equals(0)); + packets.clear(); + }, + ); + + _testEach<_ButtonedEventMixin>( + [_PointerEventContext(), _MouseEventContext()], + 'correctly handles two consecutive RMB clicks with up in between', + (_ButtonedEventMixin context) { + PointerBinding.instance.debugOverrideDetector(context); + // This can happen with the following gesture sequence: + // + // - Pops up the context menu by right clicking, but doesn't hold RMB; + // - Clicks RMB again in a different location; + // + // This seems to be happening sometimes when using RMB on the Mac trackpad. + + List packets = []; + ui.window.onPointerDataPacket = (ui.PointerDataPacket packet) { + packets.add(packet); + }; + + // Press RMB, popping up the context menu. + glassPane.dispatchEvent(context.mouseDown( + button: 2, + buttons: 2, + clientX: 10.0, + clientY: 10.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(2)); + expect(packets[0].data[0].change, equals(ui.PointerChange.add)); + expect(packets[0].data[0].synthesized, equals(true)); + expect(packets[0].data[1].change, equals(ui.PointerChange.down)); + expect(packets[0].data[1].synthesized, equals(false)); + expect(packets[0].data[1].buttons, equals(2)); + packets.clear(); + + // RMB up. + glassPane.dispatchEvent(context.mouseUp( + button: 2, + clientX: 10.0, + clientY: 10.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(1)); + expect(packets[0].data[0].change, equals(ui.PointerChange.up)); + expect(packets[0].data[0].synthesized, equals(false)); + expect(packets[0].data[0].buttons, equals(0)); + packets.clear(); + + // Press RMB again. In Chrome, when RMB is clicked again while the + // context menu is still active, it sends a pointerdown/mousedown event + // with "buttons:0". + glassPane.dispatchEvent(context.mouseDown( + button: 2, + buttons: 0, + clientX: 20.0, + clientY: 20.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(2)); + expect(packets[0].data[0].change, equals(ui.PointerChange.hover)); + expect(packets[0].data[0].synthesized, equals(true)); + expect(packets[0].data[0].buttons, equals(0)); + expect(packets[0].data[1].change, equals(ui.PointerChange.down)); + expect(packets[0].data[1].synthesized, equals(false)); + expect(packets[0].data[1].buttons, equals(2)); + packets.clear(); + + // Release RMB. + glassPane.dispatchEvent(context.mouseUp( + button: 2, + clientX: 20.0, + clientY: 20.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(1)); + expect(packets[0].data[0].change, equals(ui.PointerChange.up)); + expect(packets[0].data[0].synthesized, equals(false)); + expect(packets[0].data[0].buttons, equals(0)); + packets.clear(); + }, + ); + + _testEach<_ButtonedEventMixin>( + [_PointerEventContext(), _MouseEventContext()], + 'correctly handles two consecutive RMB clicks in two different locations', + (_ButtonedEventMixin context) { + PointerBinding.instance.debugOverrideDetector(context); + // This can happen with the following gesture sequence: + // + // - Pops up the context menu by right clicking; + // - The browser sends RMB up event; + // - Click RMB again in a different location; + // + // This scenario happens occasionally. I'm still not sure why, but in some + // cases, the browser actually sends an `up` event for the RMB click even + // when the context menu is shown. + + List packets = []; + ui.window.onPointerDataPacket = (ui.PointerDataPacket packet) { + packets.add(packet); + }; + + // Press RMB and hold, popping up the context menu. + glassPane.dispatchEvent(context.mouseDown( + button: 2, + buttons: 2, + clientX: 10.0, + clientY: 10.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(2)); + expect(packets[0].data[0].change, equals(ui.PointerChange.add)); + expect(packets[0].data[0].synthesized, equals(true)); + expect(packets[0].data[1].change, equals(ui.PointerChange.down)); + expect(packets[0].data[1].synthesized, equals(false)); + expect(packets[0].data[1].buttons, equals(2)); + packets.clear(); + + // Release RMB. + glassPane.dispatchEvent(context.mouseUp( + button: 2, + clientX: 10.0, + clientY: 10.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(1)); + expect(packets[0].data[0].change, equals(ui.PointerChange.up)); + expect(packets[0].data[0].buttons, equals(0)); + packets.clear(); + + // Press RMB again, in a different location. + glassPane.dispatchEvent(context.mouseDown( + button: 2, + buttons: 2, + clientX: 20.0, + clientY: 20.0, + )); + expect(packets, hasLength(1)); + expect(packets[0].data, hasLength(2)); + expect(packets[0].data[0].change, equals(ui.PointerChange.hover)); + expect(packets[0].data[0].synthesized, equals(true)); + expect(packets[0].data[0].buttons, equals(0)); + expect(packets[0].data[1].change, equals(ui.PointerChange.down)); + expect(packets[0].data[1].synthesized, equals(false)); + expect(packets[0].data[1].buttons, equals(2)); + packets.clear(); + }, + ); + // MULTIPOINTER ADAPTERS _testEach<_MultiPointerEventMixin>( @@ -1314,7 +1679,7 @@ void main() { // POINTER ADAPTER - _testEach( + _testEach<_PointerEventContext>( [_PointerEventContext()], 'does not synthesize pointer up if from different device', (_PointerEventContext context) { -- GitLab