Ability to disable browser context menu

[justinmc]

The goal of this PR is to create a way to disable the browser-rendered context menu on web.

The main reason that someone might want to do this is to use a custom Flutter-rendered context menu instead, which this PR will unblock.

After this PR:

Screen.Recording.2023-01-06.at.4.34.53.PM.mov

The current, basic approach

I'm just calling event.preventDefault() on contextmenu events at the root domWindow level. I've tied this to platform channel methods that could be exposed in the framework.

  static final DomEventListener _handleContextMenu = allowInterop((DomEvent event) {
    event.preventDefault();
  });

  static void disableContextMenu() {
    domWindow.addEventListener('contextmenu', _handleContextMenu);
  }

  static void enableContextMenu() {
    domWindow.removeEventListener('contextmenu', _handleContextMenu);
  }

Alternative, more fully featured approach

It might be nice if, instead of toggling the context menus for the whole app, users could toggle it for a widget subtree. Would it be at all possible to do this? I guess there would have to be a specific DOM element for every given widget that we could add/remove the listener on.

With either approach, please excuse my lack of web engine knowledge and let me know if I'm on the right track!

References

Part of: flutter/flutter#78671
Framework PR: flutter/flutter#118194

Example code

import 'package:flutter/material.dart';
import 'package:flutter/services.dart';

void main() {
  runApp(const MyApp());
}

class MyApp extends StatelessWidget {
  const MyApp({super.key});

  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      theme: ThemeData(
        primarySwatch: Colors.blue,
      ),
      home: const MyHomePage(title: 'Flutter Demo Home Page'),
    );
  }
}

class MyHomePage extends StatefulWidget {
  const MyHomePage({super.key, required this.title});

  final String title;

  @override
  State<MyHomePage> createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  bool _contextMenuEnabled = true;

  void _toggleContextMenu() {
    if (_contextMenuEnabled) {
      ContextMenu.disableContextMenu();
    } else {
      ContextMenu.enableContextMenu();
    }
    setState(() {
      _contextMenuEnabled = !_contextMenuEnabled;
    });
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        title: Text(widget.title),
      ),
      floatingActionButton: FloatingActionButton(
        onPressed: _toggleContextMenu,
        tooltip: _contextMenuEnabled ? 'Disable' : 'Enable',
        child: Icon(_contextMenuEnabled ? Icons.thumb_down: Icons.thumb_up),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const TextField(),

            // This container does not show a context menu.
            GestureDetector(
              onSecondaryTapDown: (TapDownDetails details) async {
                await ContextMenu.disableContextMenu();
                await Future.delayed(const Duration(seconds: 0));
                ContextMenu.enableContextMenu();
              },
              child: Container(
                width: 200,
                height: 200,
                color: Colors.tealAccent,
              ),
            ),
          ],
        ),
      ),
    );
  }
}

[mdebbar]

Both PRs (engine & framework) look good to me overall. They are still in draft mode, so I'll assume that tests will be added later.

[mdebbar]

The path of this file needs to be added here:

engine/ci/licenses_golden/licenses_flutter

Lines 1910 to 1914 in eb5c6f0

	ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/clipboard.dart + ../../../flutter/LICENSE
	ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/color_filter.dart + ../../../flutter/LICENSE
	ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/configuration.dart + ../../../flutter/LICENSE
	ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/dom.dart + ../../../flutter/LICENSE
	ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/embedder.dart + ../../../flutter/LICENSE

[mdebbar]

The flutter app could be embedded inside a host element (instead of taking over the entire window). We should add the event listener to the host element. cc @ditman for the right way to do this.

[ditman]

The contextmenu event bubbles (spec), so this should be enough?

If this needs to be aware of the embedding, the enable/disable methods could be added to the EmbeddingStrategy abstract class (and its implementations):

https://github.com/flutter/engine/blob/main/lib/web_ui/lib/src/engine/view_embedder/embedding_strategy/embedding_strategy.dart#L23

[mdebbar]

The contextmenu event bubbles (spec), so this should be enough?

Yes, but it's more than enough :) It disables the context menu for the entire page, which we don't want in the host element mode.

[ditman]

You're very right :P

[justinmc]

Thanks for the advice here! I've moved the enable/disable methods to the embedding strategy and made them accessible via FlutterViewEmbedder.

[mdebbar]

It might be nice if, instead of toggling the context menus for the whole app, users could toggle it for a widget subtree. Would it be at all possible to do this?

This isn't possible because the entire app could be rendered on a canvas element.

To achieve this, you want the pointer event listener on the framework side to decide whether it wants to show a context menu or not (e.g. based on which widget was hit tested). And it should immediately tell the engine that a context menu shouldn't be shown for this event.

[justinmc]

Thanks for the review! Understood about specific widget subtrees. I just played around with using the API in this PR to quickly disable and then reenable the context menu for a specific GestureDetector in the framework, and it worked, which is a good endorsement for this API.

I'm going to go ahead with cleaning these two PRs up and writing tests for them when I'm back next week, and I'll let you know when they're ready for a real review.

[justinmc]

Is there anything else I could expect here, like could I somehow verify that the event listener was added to the dom element?

[ditman]

You could add your own contextmenu Event handler to the root DOM element of the strategy in the test, and check if isDefaultPrevented has been set to true. (IIRC, once you prevent the default in a handler, the event will be modified to set defaultPrevented to true.)

In order to programmatically trigger the event, you should be able to dispatchEvent a contextmenu event using the root DOM element of the strategy.

[mdebbar]

There are some examples in our code base. E.g.

engine/lib/web_ui/test/raw_keyboard_test.dart

Line 57 in 7bff5dd

expect(event.defaultPrevented, isFalse);

You don't have to create an event listener on the root element. Just check defaultPrevented on the event you dispatched.

[justinmc]

That worked, thank you all for the guidance here! I think the tests are much more robust this way, and I don't have to use @visibleForTest.

[ditman]

Just check defaultPrevented on the event you dispatched.

Damn this approach is good! Thanks for the tests @justinmc!

[justinmc]

I've omitted any web or browser namespacing (not "flutter/webcontextmenu") because other methods don't seem to include platform names like that. And maybe in the future this could be used on other platforms.

In the framework PR I'll make the API specific (BrowserContextMenu.disable or something like that).