Drop-down closes via tap/touch again.

third_party/WebKit/Source/web/WebViewImpl.cpp

Index: third_party/WebKit/Source/web/WebViewImpl.cpp
diff --git a/third_party/WebKit/Source/web/WebViewImpl.cpp b/third_party/WebKit/Source/web/WebViewImpl.cpp
index df6b5c3064c39466c296131328663f9907c1df9f..5cb01dc91f4dfe65e15049c20c719244407bfc4c 100644
--- a/third_party/WebKit/Source/web/WebViewImpl.cpp
+++ b/third_party/WebKit/Source/web/WebViewImpl.cpp
@@ -816,13 +816,6 @@ WebInputEventResult WebViewImpl::handleGestureEvent(
 
   switch (event.type) {
     case WebInputEvent::GestureTap: {
-      // If there is a popup open, close it as the user is clicking on the page
-      // (outside of the popup). We also save it so we can prevent a tap on an
-      // element from immediately reopening the same popup.
-      RefPtr<WebPagePopupImpl> pagePopup = m_pagePopup;
-      hidePopups();
-      DCHECK(!m_pagePopup);
-
       m_client->cancelScheduledContentIntents();
       if (detectContentOnTouch(targetedEvent)) {
         eventResult = WebInputEventResult::HandledSystem;
@@ -874,13 +867,14 @@ WebInputEventResult WebViewImpl::handleGestureEvent(
 
       eventResult = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(
           targetedEvent);
-
-      if (m_pagePopup && pagePopup &&
-          m_pagePopup->hasSamePopupClient(pagePopup.get())) {
+      if (m_pagePopup && m_lastCancelledPagePopup &&
+          m_pagePopup->hasSamePopupClient(m_lastCancelledPagePopup.get())) {
         // The tap triggered a page popup that is the same as the one we just
-        // closed.  It needs to be closed.
+        // closed. It needs to be closed.
+        m_lastCancelledPagePopup = nullptr;

[bokan, 2016/12/08 16:28:35]

This should be cleared even if we opened a different popup or none at all.

[sahel, 2016/12/09 15:36:37]

A gestureTapDown will close a popup and store it in the lastHiddenPopUp, (null
if no popup was open) there are three possible cases next:

a) the next tap will try to open the same popup. This case it's prevented.

b) the next tap will open nothing. In this case the next gestureTapDown will set
the lastHiddenPopUp to null. The following tap will open a popup if needed.

c) the next tap will open another popup. In this case the next gestureTapDown
will close the new popup and update the lastHiddenPopUp.

In fact, GestureTapDown will always happen before a gestureTap, and update/clear
the lastHiddenPopUp. Clearing it in case of the attempt to open the same popup,
or a tapCancel is not necessary. I can get rid of them if it makes the code
simpler/less confusing.

[bokan, 2016/12/09 16:58:08]

Yep, I agree the logic is correct. But this value is used only to prevent the
GestureTap from opening the menu hidden by its associated GestureTapDown. Once
GestureTap is done you don't need it anymore so it seems more clear and less bug
prone to reset it here. As code moves around and changes over time, it's better
not to leave around dangling values unless you actually need them.

         cancelPagePopup();
       }
+

[bokan, 2016/12/08 16:28:35]

Nit: remove line.

[sahel, 2016/12/09 15:36:36]

Done.

       break;
     }
     case WebInputEvent::GestureTwoFingerTap:
@@ -900,14 +894,28 @@ WebInputEventResult WebViewImpl::handleGestureEvent(
 
       break;
     }
+    case WebInputEvent::GestureTapDown: {
+      // Touch pinch zoom and scroll on the page (outside of a popup) must hide
+      // the popup. In case of a touch scroll or pinch zoom, this function is
+      // called with GestureTapDown rather than a GSB/GSU/GSE or GPB/GPU/GPE.
+      // When we close a popup because of a GestureTapDown, we also save it so
+      // we can prevent the following GestureTap from immediately reopening the
+      // same popup.
+      m_lastCancelledPagePopup = m_pagePopup;
+      hidePopups();
+      DCHECK(!m_pagePopup);
+      eventResult = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(
+          targetedEvent);
+      break;
+    }
+    case WebInputEvent::GestureTapCancel: {
+      m_lastCancelledPagePopup = nullptr;
+      eventResult = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(
+          targetedEvent);
+      break;
+    }
     case WebInputEvent::GestureShowPress:
       m_client->cancelScheduledContentIntents();

[bokan, 2016/12/08 16:28:34]

For consistency, add a handleGestureEvent call here, break, and braces.

[sahel, 2016/12/09 15:36:36]

Done.

-    case WebInputEvent::GestureTapDown:
-      // Touch pinch zoom and scroll must hide the popup. In case of a touch
-      // scroll or pinch zoom, this function is called with GestureTapDown
-      // rather than a GSB/GSU/GSE or GPB/GPU/GPE.
-      hidePopups();
-    case WebInputEvent::GestureTapCancel:
     case WebInputEvent::GestureTapUnconfirmed: {
       eventResult = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(
           targetedEvent);