Commit 48496741 authored by hugoh's avatar hugoh Committed by Commit bot

Correct logic "Should ContextMenu target the selection?"

If the selection doesn't have focus, it shouldn't be the target
of the context menu.

For example, an _unfocused_ range selection should not be the
context menu's target (the focused element should be the target).

BUG=725005, 725022

Review-Url: https://codereview.chromium.org/2880313002
Cr-Commit-Position: refs/heads/master@{#473842}
parent 3c1005ee
...@@ -6,8 +6,19 @@ var contextForMenu; ...@@ -6,8 +6,19 @@ var contextForMenu;
function catchContextMenu(event) { function catchContextMenu(event) {
contextForMenu = event.currentTarget.tagName; contextForMenu = event.currentTarget.tagName;
} }
function focusLinkAndAssertLinkIsTargetOfContextMenu() {
eventSender.keyDown('Escape'); // Hide menu.
document.querySelector('A').focus();
contextForMenu = undefined;
eventSender.keyDown('ContextMenu');
assert_equals(contextForMenu, 'A',
'ContextMenu should use the focused link as target.');
}
</script> </script>
<div contenteditable oncontextmenu="catchContextMenu(event);">Some editable text.</div>
<span oncontextmenu="catchContextMenu(event);">Some text to select.</span>
<input oncontextmenu="catchContextMenu(event);"> <input oncontextmenu="catchContextMenu(event);">
<a href="www" oncontextmenu="catchContextMenu(event);">A link</a> <a href="www" oncontextmenu="catchContextMenu(event);">A link</a>
...@@ -18,15 +29,37 @@ test(function() { ...@@ -18,15 +29,37 @@ test(function() {
document.querySelector('INPUT').focus(); document.querySelector('INPUT').focus();
eventSender.keyDown('ContextMenu'); eventSender.keyDown('ContextMenu');
assert_equals(contextForMenu, 'INPUT', assert_equals(contextForMenu, 'INPUT',
'ContextMenu should use the focused input field as context.'); 'ContextMenu should use the focused input field as target.');
focusLinkAndAssertLinkIsTargetOfContextMenu();
}, 'ContextMenu should target the focused link (not the unfocused field).');
// Hide INPUT's context menu before we display A's context menu. test(function() {
eventSender.keyDown('Escape'); assert_exists(window, 'eventSender', 'This test requires eventSender.');
document.querySelector('A').focus(); document.querySelector('div').focus();
eventSender.keyDown('ContextMenu'); eventSender.keyDown('ContextMenu');
assert_equals(contextForMenu, 'A', assert_equals(contextForMenu, 'DIV',
'ContextMenu should use the focused link as context.'); 'ContextMenu should use the editable div\'s caret as target.');
focusLinkAndAssertLinkIsTargetOfContextMenu();
}, 'ContextMenu should target the focused link (not the div\'s caret).');
test(function() {
assert_exists(window, 'eventSender', 'This test requires eventSender.');
const div = document.querySelector('div');
div.focus();
window.getSelection().selectAllChildren(div);
eventSender.keyDown('ContextMenu');
assert_equals(contextForMenu, 'DIV',
'ContextMenu should use the editable div\'s range selection as target.');
focusLinkAndAssertLinkIsTargetOfContextMenu();
}, 'ContextMenu should target the focused link (not the div\'s selection).');
test(function() {
assert_exists(window, 'eventSender', 'This test requires eventSender.');
}, 'ContextMenu should always follow focused element.'); const span = document.querySelector('span');
window.getSelection().selectAllChildren(span);
focusLinkAndAssertLinkIsTargetOfContextMenu();
}, 'ContextMenu should target the focused link (not the unfocused selection).');
</script> </script>
...@@ -1802,6 +1802,14 @@ WebInputEventResult EventHandler::SendContextMenuEvent( ...@@ -1802,6 +1802,14 @@ WebInputEventResult EventHandler::SendContextMenuEvent(
event, mev.GetHitTestResult().CanvasRegionId(), 0); event, mev.GetHitTestResult().CanvasRegionId(), 0);
} }
static bool ShouldShowContextMenuAtSelection(const FrameSelection& selection) {
const VisibleSelection& visible_selection =
selection.ComputeVisibleSelectionInDOMTreeDeprecated();
if (!visible_selection.IsRange() && !visible_selection.RootEditableElement())
return false;
return selection.SelectionHasFocus();
}
WebInputEventResult EventHandler::SendContextMenuEventForKey( WebInputEventResult EventHandler::SendContextMenuEventForKey(
Element* override_target_element) { Element* override_target_element) {
FrameView* view = frame_->View(); FrameView* view = frame_->View();
...@@ -1824,14 +1832,9 @@ WebInputEventResult EventHandler::SendContextMenuEventForKey( ...@@ -1824,14 +1832,9 @@ WebInputEventResult EventHandler::SendContextMenuEventForKey(
Element* focused_element = Element* focused_element =
override_target_element ? override_target_element : doc->FocusedElement(); override_target_element ? override_target_element : doc->FocusedElement();
FrameSelection& selection = frame_->Selection(); FrameSelection& selection = frame_->Selection();
Position start =
selection.ComputeVisibleSelectionInDOMTreeDeprecated().Start();
VisualViewport& visual_viewport = frame_->GetPage()->GetVisualViewport(); VisualViewport& visual_viewport = frame_->GetPage()->GetVisualViewport();
if (!override_target_element && start.AnchorNode() && !selection.IsHidden() && if (!override_target_element && ShouldShowContextMenuAtSelection(selection)) {
(selection.ComputeVisibleSelectionInDOMTreeDeprecated()
.RootEditableElement() ||
selection.ComputeVisibleSelectionInDOMTreeDeprecated().IsRange())) {
// TODO(editing-dev): Use of updateStyleAndLayoutIgnorePendingStylesheets // TODO(editing-dev): Use of updateStyleAndLayoutIgnorePendingStylesheets
// needs to be audited. See http://crbug.com/590369 for more details. // needs to be audited. See http://crbug.com/590369 for more details.
doc->UpdateStyleAndLayoutIgnorePendingStylesheets(); doc->UpdateStyleAndLayoutIgnorePendingStylesheets();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment