Drop-down closes via tap/touch again.

third_party/WebKit/Source/web/WebViewImpl.cpp

 /*
  * Copyright (C) 2011, 2012 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 798 matching lines @@
     case WebInputEvent::GestureLongPress:
       for (size_t i = 0; i < m_linkHighlights.size(); ++i)
         m_linkHighlights[i]->startHighlightAnimationIfNeeded();
       break;
     default:
       break;
   }
 
   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;
         break;
       }
 
       // Don't trigger a disambiguation popup on sites designed for mobile
       // devices.  Instead, assume that the page has been designed with big
       // enough buttons and links.  Don't trigger a disambiguation popup when
       // screencasting, since it's implemented outside of compositor pipeline
@@ skipping 31 matching lines @@
               m_linkHighlights[i]->startHighlightAnimationIfNeeded();
             eventResult = WebInputEventResult::HandledSystem;
             eventCancelled = true;
             break;
           }
         }
       }
 
       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:
     case WebInputEvent::GestureLongPress:
     case WebInputEvent::GestureLongTap: {
       if (!mainFrameImpl() || !mainFrameImpl()->frameView())
         break;
 
       m_client->cancelScheduledContentIntents();
       m_page->contextMenuController().clearContextMenu();
       {
         ContextMenuAllowedScope scope;
         eventResult =
             mainFrameImpl()->frame()->eventHandler().handleGestureEvent(
                 targetedEvent);
       }
 
       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);
       break;
     }
     default:
       NOTREACHED();

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

Nit: add braces here.

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

Done.

   }
   m_client->didHandleGestureEvent(event, eventCancelled);
   return eventResult;
 }
 
 WebInputEventResult WebViewImpl::handleSyntheticWheelFromTouchpadPinchEvent(
     const WebGestureEvent& pinchEvent) {
   DCHECK_EQ(pinchEvent.type, WebInputEvent::GesturePinchUpdate);
 
   // For pinch gesture events, match typical trackpad behavior on Windows by
@@ skipping 3276 matching lines @@
   if (focusedFrame->localFrameRoot() != mainFrameImpl()->frame())
     return nullptr;
   return focusedFrame;
 }
 
 LocalFrame* WebViewImpl::focusedLocalFrameAvailableForIme() const {
   return m_imeAcceptEvents ? focusedLocalFrameInWidget() : nullptr;
 }
 
 }  // namespace blink