-
Notifications
You must be signed in to change notification settings - Fork 6k
Ability to disable browser context menu #38682
Changes from 9 commits
b543526
8d38b6b
733ba64
3503fd4
1bce49f
98cdb32
f25fbca
4fd56bd
acbc3be
4ace58d
4bc3f04
a2c5a88
a0e294b
ad8bc4e
f626594
edb57c3
781148d
85b6d88
167babb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -48,6 +48,12 @@ class FullPageEmbeddingStrategy extends EmbeddingStrategy { | |||||
registerElementForCleanup(resourceHost); | ||||||
} | ||||||
|
||||||
@override | ||||||
void disableContextMenu() => disableContextMenuOn(domDocument.body!); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We usually set these event listeners on
Suggested change
|
||||||
|
||||||
@override | ||||||
void enableContextMenu() => enableContextMenuOn(domDocument.body!); | ||||||
|
||||||
void _setHostAttribute(String name, String value) { | ||||||
domDocument.body!.setAttribute(name, value); | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -121,4 +121,33 @@ void doTests() { | |||
reason: 'Should be injected `nextTo` the passed element.'); | ||||
}); | ||||
}); | ||||
|
||||
group('context menu', () { | ||||
setUp(() { | ||||
target = createDomElement('this-is-the-target'); | ||||
domDocument.body!.append(target); | ||||
strategy = CustomElementEmbeddingStrategy(target); | ||||
strategy.initialize(); | ||||
}); | ||||
|
||||
tearDown(() { | ||||
target.remove(); | ||||
}); | ||||
|
||||
test('disableContextMenu and enableContextMenu can toggle the context menu', () { | ||||
expect(strategy.contextMenuEnabled, isTrue); | ||||
|
||||
strategy.disableContextMenu(); | ||||
expect(strategy.contextMenuEnabled, isFalse); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there anything else I could expect here, like could I somehow verify that the event listener was added to the dom element? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could add your own In order to programmatically trigger the event, you should be able to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are some examples in our code base. E.g.
You don't have to create an event listener on the root element. Just check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Damn this approach is good! Thanks for the tests @justinmc! |
||||
|
||||
strategy.disableContextMenu(); | ||||
expect(strategy.contextMenuEnabled, isFalse); | ||||
|
||||
strategy.enableContextMenu(); | ||||
expect(strategy.contextMenuEnabled, isTrue); | ||||
|
||||
strategy.enableContextMenu(); | ||||
expect(strategy.contextMenuEnabled, isTrue); | ||||
}); | ||||
}); | ||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).