Skip to content

Commit

Permalink
Drop-down closes via tap/touch again.
Browse files Browse the repository at this point in the history
The drop-down list closes on a GestureTapDown, and the next GestureTap
cannot open the same list. The regression was because of the following
cl: https://codereview.chromium.org/2517253002/

BUG=670185, 569092
Test=LayoutTests/fast/forms/select-popup/popup-menu-touch-operations.html

Review-Url: https://codereview.chromium.org/2558113002
Cr-Commit-Position: refs/heads/master@{#437650}
  • Loading branch information
sahel authored and Commit bot committed Dec 9, 2016
1 parent 841a6a8 commit 59dc4be
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ PASS window.internals.pagePopupWindow is not null
PASS menuElement2.value is "2"
==> Test popup closes on outside GestureTapDawn
PASS window.internals.pagePopupWindow is null
==> Test popup doesn't reopen immediately after closing
PASS window.internals.pagePopupWindow is null
PASS window.internals.pagePopupWindow is null
PASS window.internals.pagePopupWindow is not null
PASS window.internals.pagePopupWindow is null
PASS window.internals.pagePopupWindow is null
PASS successfullyParsed is true

TEST COMPLETE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,13 +155,35 @@
openPicker(menuElement, function () {
eventSender.gestureTapDown(300, 300);
shouldBeNull('window.internals.pagePopupWindow');
finishJSTest();
test4();
}, function () {
testFailed('picker didn\'t open')
finishJSTest();
});
}

function test4() {
debug("==> Test popup doesn't reopen immediately after closing");
eventSender.clearTouchPoints();
shouldBeNull('window.internals.pagePopupWindow');

// Open the popup with a GestureTap.
var position = elementCenterPosition(menuElement);
eventSender.gestureTapDown(position[0], position[1]);
shouldBeNull('window.internals.pagePopupWindow');
eventSender.gestureTap(position[0], position[1]);
shouldNotBe('window.internals.pagePopupWindow', 'null');

// GestureTapDown on an open popup closes it.
eventSender.gestureTapDown(position[0], position[1]);
shouldBeNull('window.internals.pagePopupWindow');

// The next GestureTap on the recently closed popup shouldn't open it.
eventSender.gestureTap(position[0], position[1]);
shouldBeNull('window.internals.pagePopupWindow');

finishJSTest();
}

</script>
</body>
Expand Down
50 changes: 30 additions & 20 deletions third_party/WebKit/Source/web/WebViewImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -874,13 +867,13 @@ WebInputEventResult WebViewImpl::handleGestureEvent(

eventResult = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(
targetedEvent);

if (m_pagePopup && pagePopup &&
m_pagePopup->hasSamePopupClient(pagePopup.get())) {
if (m_pagePopup && m_lastHiddenPagePopup &&
m_pagePopup->hasSamePopupClient(m_lastHiddenPagePopup.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.
cancelPagePopup();
}
m_lastHiddenPagePopup = nullptr;
break;
}
case WebInputEvent::GestureTwoFingerTap:
Expand All @@ -900,21 +893,38 @@ WebInputEventResult WebViewImpl::handleGestureEvent(

break;
}
case WebInputEvent::GestureShowPress:
m_client->cancelScheduledContentIntents();
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.
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_lastHiddenPagePopup = m_pagePopup;
hidePopups();
case WebInputEvent::GestureTapCancel:
DCHECK(!m_pagePopup);
eventResult = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(
targetedEvent);
break;
}
case WebInputEvent::GestureTapCancel: {
m_lastHiddenPagePopup = nullptr;
eventResult = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(
targetedEvent);
break;
}
case WebInputEvent::GestureShowPress: {
m_client->cancelScheduledContentIntents();
eventResult = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(
targetedEvent);
break;
}
case WebInputEvent::GestureTapUnconfirmed: {
eventResult = mainFrameImpl()->frame()->eventHandler().handleGestureEvent(
targetedEvent);
break;
}
default:
NOTREACHED();
default: { NOTREACHED(); }
}
m_client->didHandleGestureEvent(event, eventCancelled);
return eventResult;
Expand Down
5 changes: 5 additions & 0 deletions third_party/WebKit/Source/web/WebViewImpl.h
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,11 @@ class WEB_EXPORT WebViewImpl final
// The popup associated with an input/select element.
RefPtr<WebPagePopupImpl> m_pagePopup;

// This stores the last hidden page popup. If a GestureTap attempts to open
// the popup that is closed by its previous GestureTapDown, the popup remains
// closed.
RefPtr<WebPagePopupImpl> m_lastHiddenPagePopup;

Persistent<DevToolsEmulator> m_devToolsEmulator;
std::unique_ptr<PageOverlay> m_pageColorOverlay;

Expand Down

0 comments on commit 59dc4be

Please sign in to comment.