Correct logic "Should ContextMenu target the selection?"

third_party/WebKit/Source/core/input/EventHandler.cpp

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All rights
  * reserved.
  * Copyright (C) 2006 Alexey Proskuryakov (ap@webkit.org)
  * Copyright (C) 2012 Digia Plc. and/or its subsidiary(-ies)
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
@@ skipping 1806 matching lines @@
 #if OS(WIN)
   int right_aligned = ::GetSystemMetrics(SM_MENUDROPALIGNMENT);
 #else
   int right_aligned = 0;
 #endif
   IntPoint location_in_root_frame;
 
   Element* focused_element =
       override_target_element ? override_target_element : doc->FocusedElement();
   FrameSelection& selection = frame_->Selection();
-  Position start =
-      selection.ComputeVisibleSelectionInDOMTreeDeprecated().Start();
   VisualViewport& visual_viewport = frame_->GetPage()->GetVisualViewport();
 
-  if (!override_target_element && start.AnchorNode() && !selection.IsHidden() &&

[Xiaocheng, 2017/05/17 02:06:27]

The current impl is incorrect. When we have an unfocused range selection in
contenteditable, the context menu is still targeted at the selection instead of
the focused element.

We should check FS::SelectionHasFocus() here instead of FS::IsHidden().

FS::IsHidden() should only be used by paint code. That's why I insisted having
two separate functions HasFocus() and IsHidden() to serve different purposes at
the beginning, instead of one IsActive().

[hugoh_UTC2, 2017/05/17 09:07:47]

Thanks. You're right. I fixed this in PS3.

-      (selection.ComputeVisibleSelectionInDOMTreeDeprecated()
-           .RootEditableElement() ||
-       selection.ComputeVisibleSelectionInDOMTreeDeprecated().IsRange())) {
+  if (!override_target_element &&
+      (selection.ComputeVisibleSelectionInDOMTreeDeprecated().IsRange() ||

[yosin_UTC9, 2017/05/17 01:54:59]

I would like to move this condition expression, except for
|!override_target_element|,
into named function, e.g. ShouldShowContextMenuAtSelection(const LocalFrame&) ->
bool.
To help to avoid to use ComputeVisibleSelectionInDOMTreeDeprecated() easier.

[hugoh_UTC2, 2017/05/17 09:07:47]

Done, see PS3.

+       selection.ComputeVisibleSelectionInDOMTreeDeprecated()
+           .RootEditableElement()) &&
+      !selection.IsHidden()) {
     // TODO(editing-dev): Use of updateStyleAndLayoutIgnorePendingStylesheets
     // needs to be audited.  See http://crbug.com/590369 for more details.
     doc->UpdateStyleAndLayoutIgnorePendingStylesheets();
 
     IntRect first_rect = frame_->GetEditor().FirstRectForRange(
         selection.ComputeVisibleSelectionInDOMTree()
             .ToNormalizedEphemeralRange());
 
     int x = right_aligned ? first_rect.MaxX() : first_rect.X();
     // In a multiline edit, firstRect.maxY() would end up on the next line, so
@@ skipping 274 matching lines @@
     MouseEventWithHitTestResults& mev,
     LocalFrame* subframe) {
   WebInputEventResult result =
       subframe->GetEventHandler().HandleMouseReleaseEvent(mev.Event());
   if (result != WebInputEventResult::kNotHandled)
     return result;
   return WebInputEventResult::kHandledSystem;
 }
 
 }  // namespace blink