Hide popups when handling mouse down, mouse wheel, or gesture tap in a WebFrameWidget

third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp

 /*
  * Copyright (C) 2014 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 53 matching lines @@
 #include "web/CompositionUnderlineVectorBuilder.h"
 #include "web/CompositorMutatorImpl.h"
 #include "web/CompositorWorkerProxyClientImpl.h"
 #include "web/ContextMenuAllowedScope.h"
 #include "web/InspectorOverlay.h"
 #include "web/PageOverlay.h"
 #include "web/WebDevToolsAgentImpl.h"
 #include "web/WebInputEventConversion.h"
 #include "web/WebInputMethodControllerImpl.h"
 #include "web/WebLocalFrameImpl.h"
+#include "web/WebPagePopupImpl.h"
 #include "web/WebPluginContainerImpl.h"
 #include "web/WebRemoteFrameImpl.h"
 #include "web/WebViewFrameWidget.h"
 #include "wtf/AutoReset.h"
 #include "wtf/PtrUtil.h"
 
 namespace blink {
 
 // WebFrameWidget ------------------------------------------------------------
 
@@ skipping 677 matching lines @@
 
 void WebFrameWidgetImpl::handleMouseLeave(LocalFrame& mainFrame,
                                           const WebMouseEvent& event) {
   // FIXME: WebWidget doesn't have the method below.
   // m_client->setMouseOverURL(WebURL());
   PageWidgetEventHandler::handleMouseLeave(mainFrame, event);
 }
 
 void WebFrameWidgetImpl::handleMouseDown(LocalFrame& mainFrame,
                                          const WebMouseEvent& event) {
+  RefPtr<WebPagePopupImpl> pagePopup;
+  if (event.button == WebMouseEvent::Button::Left) {
+    // The mouse down has been outside of any open popups so we should cancel
+    // the current page popup for this process.

[dcheng, 2017/04/07 06:32:01]

Nit: I prefer the original comment (since it also describes why we save
pagePopup)

[EhsanK, 2017/04/07 16:16:12]

Acknowledged.

+    pagePopup = static_cast<WebPagePopupImpl*>(view()->pagePopup());

[dcheng, 2017/04/07 06:32:01]

Let's take advantage of the fact that view() already returns a WebViewImpl and
change pagePopup() to just return a WebPagePopupImpl for WebViewImpl. Covariant
return types are allowed for virtual functions, so this should work.

[EhsanK, 2017/04/07 16:16:12]

Yes good idea. Thanks!

+    view()->hidePopups();
+  }
+
   // Take capture on a mouse down on a plugin so we can send it mouse events.
   // If the hit node is a plugin but a scrollbar is over it don't start mouse
   // capture because it will interfere with the scrollbar receiving events.
   IntPoint point(event.positionInWidget().x, event.positionInWidget().y);
   if (event.button == WebMouseEvent::Button::Left) {
     point = m_localRoot->frameView()->rootFrameToContents(point);
     HitTestResult result(
         m_localRoot->frame()->eventHandler().hitTestResultAtPoint(point));
     result.setToShadowHostIfInRestrictedShadowRoot();
     Node* hitNode = result.innerNode();
 
     if (!result.scrollbar() && hitNode && hitNode->layoutObject() &&
         hitNode->layoutObject()->isEmbeddedObject()) {
       m_mouseCaptureNode = hitNode;
       TRACE_EVENT_ASYNC_BEGIN0("input", "capturing mouse", this);
     }
   }
 
   PageWidgetEventHandler::handleMouseDown(mainFrame, event);
 
   if (event.button == WebMouseEvent::Button::Left && m_mouseCaptureNode)
     m_mouseCaptureGestureToken =
         mainFrame.eventHandler().takeLastMouseDownGestureToken();
 
+  WebPagePopupImpl* pagePopupImpl =

[dcheng, 2017/04/07 06:32:01]

Nit: Call this currentPagePopup

[EhsanK, 2017/04/07 16:16:12]

Or I can just remove it now that view()->pagePopup() is a WebPagePopupImpl*.

+      static_cast<WebPagePopupImpl*>(view()->pagePopup());
+  if (pagePopupImpl && pagePopupImpl == pagePopup &&
+      pagePopupImpl->hasSamePopupClient(pagePopup.get())) {
+    // That click triggered a page popup that is the same as the one we just
+    // closed.  It needs to be closed.
+    view()->hidePopups();

[dcheng, 2017/04/07 06:32:01]

The logic in WebViewImpl calls cancel() -- is it OK to call a different method
here?

[EhsanK, 2017/04/07 16:16:12]

hidePopups() simply calls cancelPagePopup(). That being said, I guess
hidePopups() should be replaced by cancelPagePopup() in the public API. The
reason is that hidePopups() used to call hideSelectPopup() or hideAutofill...
amongs other things. But they are removed now, e.g., here
https://codereview.chromium.org/1142303005

Now it simply calls cancelPagePopup() and I think its name is only as accurate
as the comment above it in WebView.h (which is wrong):
https://cs.chromium.org/chromium/src/third_party/WebKit/public/web/WebView.h?...

+  }
+
   // Dispatch the contextmenu event regardless of if the click was swallowed.
   if (!page()->settings().getShowContextMenuOnMouseUp()) {
 #if OS(MACOSX)
     if (event.button == WebMouseEvent::Button::Right ||
         (event.button == WebMouseEvent::Button::Left &&
          event.modifiers() & WebMouseEvent::ControlKey))
       mouseContextMenu(event);
 #else
     if (event.button == WebMouseEvent::Button::Right)
       mouseContextMenu(event);
@@ skipping 353 matching lines @@
   return nullptr;
 }
 
 LocalFrame* WebFrameWidgetImpl::focusedLocalFrameAvailableForIme() const {
   if (!m_imeAcceptEvents)
     return nullptr;
   return focusedLocalFrameInWidget();
 }
 
 }  // namespace blink