Skip to content

Commit

Permalink
Revert of Use the pinch viewport offset for tap disambiguation. (patc…
Browse files Browse the repository at this point in the history 
…hset #3 id:40001 of https://codereview.chromium.org/584893004/)

Reason for revert:
Makes webkit_unit_tests fail on Android:

[  FAILED  ] 1 test, listed below:
[  FAILED  ] WebFrameTest.DisambiguationPopupPinchViewport

 1 FAILED TEST
C   79.116s Main  ********************************************************************************
C   79.116s Main  Detailed Logs
C   79.116s Main  ********************************************************************************
C   79.119s Main  [FAIL] WebFrameTest.DisambiguationPopupPinchViewport:
C   79.119s Main  ../../third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4413: Failure
C   79.119s Main  Value of: (frame->view()->scrollPosition()).y()
C   79.119s Main    Actual: 0
C   79.119s Main  Expected: (IntPoint(0, 400)).y()
C   79.119s Main  Which is: 400
C   79.119s Main  ../../third_party/WebKit/Source/web/tests/WebFrameTest.cpp:4434: Failure
C   79.119s Main  Value of: client.triggered()
C   79.119s Main    Actual: false
C   79.119s Main  Expected: true
C   79.120s Main  ********************************************************************************
C   79.120s Main  Summary
C   79.120s Main  ********************************************************************************
C   79.126s Main  [==========] 1467 tests ran.
C   79.126s Main  [  PASSED  ] 1466 tests.
C   79.126s Main  [  FAILED  ] 1 test, listed below:
C   79.126s Main  [  FAILED  ] WebFrameTest.DisambiguationPopupPinchViewport
C   79.126s Main  
C   79.126s Main  1 FAILED TEST
C   79.127s Main  ********************************************************************************

Original issue's description:
> Use the pinch viewport offset for tap disambiguation.
> 
> Account for the pinch vieport offset in the touch rectangle calculation
> and pass the offset to the client. The client needs the touch rectangle
> relative to both the main frame and to the screen.
> 
> This change is part 1 of 2 (part 2 is chromium side change https://codereview.chromium.org/595693002).
> 
> BUG=370470
> 
> Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182534

[email protected],[email protected],[email protected],[email protected],[email protected]
NOTREECHECKS=true
NOTRY=true
BUG=370470

Review URL: https://codereview.chromium.org/602583003

git-svn-id: svn://svn.chromium.org/blink/trunk@182579 bbb929c8-8fbe-4397-9dbb-9b2b20218538
  • Loading branch information
@madsager
madsager committed Sep 24, 2014
1 parent a683bcd commit 79bab42
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 136 deletions.
2 changes: 1 addition & 1 deletion Source/core/frame/PinchViewport.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class PinchViewport FINAL : public GraphicsLayerClient, public ScrollableArea {
// coordinates are in partial CSS pixels.
void setLocation(const FloatPoint&);
void move(const FloatPoint&);
FloatPoint location() const { return m_offset; }
FloatPoint location() const { return m_offset; } // used in unit tests

// Sets the size of the inner viewport when unscaled in CSS pixels.
// This will be clamped to the size of the outer viewport (the main frame).
Expand Down
19 changes: 4 additions & 15 deletions Source/web/WebViewImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -748,31 +748,20 @@ bool WebViewImpl::handleGestureEvent(const WebGestureEvent& event)
// 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.
if (event.data.tap.width > 0 && !shouldDisableDesktopWorkarounds()) {
// FIXME: didTapMultipleTargets should just take a rect instead of
// an event.
WebGestureEvent scaledEvent = event;
scaledEvent.x = event.x / pageScaleFactor();
scaledEvent.y = event.y / pageScaleFactor();
scaledEvent.data.tap.width = event.data.tap.width / pageScaleFactor();
scaledEvent.data.tap.height = event.data.tap.height / pageScaleFactor();
IntRect boundingBox(
scaledEvent.x - scaledEvent.data.tap.width / 2,
scaledEvent.y - scaledEvent.data.tap.height / 2,
scaledEvent.data.tap.width,
scaledEvent.data.tap.height);

WebSize pinchViewportOffset = pinchVirtualViewportEnabled() ?
flooredIntSize(page()->frameHost().pinchViewport().location()) : IntSize();

// Keep bounding box relative to the main frame.
boundingBox.move(pinchViewportOffset);

IntRect boundingBox(scaledEvent.x - scaledEvent.data.tap.width / 2, scaledEvent.y - scaledEvent.data.tap.height / 2, scaledEvent.data.tap.width, scaledEvent.data.tap.height);
Vector<IntRect> goodTargets;
WillBeHeapVector<RawPtrWillBeMember<Node> > highlightNodes;
findGoodTouchTargets(boundingBox, mainFrameImpl()->frame(), goodTargets, highlightNodes);
// FIXME: replace touch adjustment code when numberOfGoodTargets == 1?
// Single candidate case is currently handled by: https://bugs.webkit.org/show_bug.cgi?id=85101
if (goodTargets.size() >= 2 && m_client
&& m_client->didTapMultipleTargets(pinchViewportOffset, boundingBox, goodTargets)) {

if (goodTargets.size() >= 2 && m_client && m_client->didTapMultipleTargets(scaledEvent, goodTargets)) {
enableTapHighlights(highlightNodes);
for (size_t i = 0; i < m_linkHighlights.size(); ++i)
m_linkHighlights[i]->startHighlightAnimationIfNeeded();
Expand Down
87 changes: 10 additions & 77 deletions Source/web/tests/WebFrameTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,8 @@
#include "core/editing/VisiblePosition.h"
#include "core/events/MouseEvent.h"
#include "core/fetch/MemoryCache.h"
#include "core/frame/FrameHost.h"
#include "core/frame/FrameView.h"
#include "core/frame/LocalFrame.h"
#include "core/frame/PinchViewport.h"
#include "core/frame/Settings.h"
#include "core/html/HTMLDocument.h"
#include "core/html/HTMLFormElement.h"
Expand Down Expand Up @@ -119,25 +117,11 @@ using namespace blink;

const int touchPointPadding = 32;

#define EXPECT_RECT_EQ(a, b) \
do { \
EXPECT_EQ(a.x(), b.x()); \
EXPECT_EQ(a.y(), b.y()); \
EXPECT_EQ(a.width(), b.width()); \
EXPECT_EQ(a.height(), b.height()); \
} while (false)

#define EXPECT_POINT_EQ(expected, actual) \
do { \
EXPECT_EQ((expected).x(), (actual).x()); \
EXPECT_EQ((expected).y(), (actual).y()); \
} while (false)

#define EXPECT_FLOAT_POINT_EQ(expected, actual) \
do { \
EXPECT_FLOAT_EQ((expected).x(), (actual).x()); \
EXPECT_FLOAT_EQ((expected).y(), (actual).y()); \
} while (false)
#define EXPECT_EQ_RECT(a, b) \
EXPECT_EQ(a.x(), b.x()); \
EXPECT_EQ(a.y(), b.y()); \
EXPECT_EQ(a.width(), b.width()); \
EXPECT_EQ(a.height(), b.height());

class FakeCompositingWebViewClient : public FrameTestHelpers::TestWebViewClient {
public:
Expand Down Expand Up @@ -2214,7 +2198,7 @@ TEST_F(WebFrameTest, pageScaleFactorScalesPaintClip)
GraphicsContext context(&canvas);
context.setRegionTrackingMode(GraphicsContext::RegionTrackingOpaque);

EXPECT_RECT_EQ(IntRect(0, 0, 0, 0), context.opaqueRegion().asRect());
EXPECT_EQ_RECT(IntRect(0, 0, 0, 0), context.opaqueRegion().asRect());

FrameView* view = webViewHelper.webViewImpl()->mainFrameImpl()->frameView();
IntRect paintRect(0, 0, 200, 200);
Expand All @@ -2228,7 +2212,7 @@ TEST_F(WebFrameTest, pageScaleFactorScalesPaintClip)
int viewportWidthMinusScrollbar = 50 - (view->verticalScrollbar()->isOverlayScrollbar() ? 0 : 15);
int viewportHeightMinusScrollbar = 50 - (view->horizontalScrollbar()->isOverlayScrollbar() ? 0 : 15);
IntRect clippedRect(0, 0, viewportWidthMinusScrollbar * 2, viewportHeightMinusScrollbar * 2);
EXPECT_RECT_EQ(clippedRect, context.opaqueRegion().asRect());
EXPECT_EQ_RECT(clippedRect, context.opaqueRegion().asRect());
#endif
}

Expand Down Expand Up @@ -4225,7 +4209,7 @@ TEST_F(WebFrameTest, CompositedSelectionBoundsCleared)

class DisambiguationPopupTestWebViewClient : public FrameTestHelpers::TestWebViewClient {
public:
virtual bool didTapMultipleTargets(const WebSize&, const WebRect&, const WebVector<WebRect>& targetRects) OVERRIDE
virtual bool didTapMultipleTargets(const WebGestureEvent&, const WebVector<WebRect>& targetRects) OVERRIDE
{
EXPECT_GE(targetRects.size(), 2u);
m_triggered = true;
Expand Down Expand Up @@ -4383,57 +4367,6 @@ TEST_F(WebFrameTest, DisambiguationPopupViewportSite)
}
}

static void enableVirtualViewport(WebSettings* settings)
{
settings->setPinchVirtualViewportEnabled(true);
settings->setViewportEnabled(true);
settings->setViewportMetaEnabled(true);
settings->setShrinksViewportContentToFit(true);
}

TEST_F(WebFrameTest, DisambiguationPopupPinchViewport)
{
const std::string htmlFile = "disambiguation_popup_200_by_800.html";
registerMockedHttpURLLoad(htmlFile);

DisambiguationPopupTestWebViewClient client;

FrameTestHelpers::WebViewHelper webViewHelper;
webViewHelper.initializeAndLoad(m_baseURL + htmlFile, true, 0, &client, enableVirtualViewport);

WebViewImpl* webViewImpl = webViewHelper.webViewImpl();
ASSERT_TRUE(webViewImpl);
LocalFrame* frame = webViewImpl->mainFrameImpl()->frame();
ASSERT_TRUE(frame);

webViewHelper.webView()->resize(WebSize(200, 400));

// Scroll main frame to the bottom of the document
webViewImpl->setMainFrameScrollOffset(WebPoint(0, 400));
EXPECT_POINT_EQ(IntPoint(0, 400), frame->view()->scrollPosition());

webViewImpl->setPageScaleFactor(2.0);

// Scroll pinch viewport to the top of the main frame.
PinchViewport& pinchViewport = frame->page()->frameHost().pinchViewport();
pinchViewport.setLocation(FloatPoint(0, 0));
EXPECT_FLOAT_POINT_EQ(FloatPoint(0, 0), pinchViewport.location());

// Tap at the top: there is nothing there.
client.resetTriggered();
webViewHelper.webView()->handleInputEvent(fatTap(10, 60));
EXPECT_FALSE(client.triggered());

// Scroll pinch viewport to the bottom of the main frame.
pinchViewport.setLocation(FloatPoint(0, 200));
EXPECT_FLOAT_POINT_EQ(FloatPoint(0, 200), pinchViewport.location());

// Now the tap with the same coordinates should hit two elements.
client.resetTriggered();
webViewHelper.webView()->handleInputEvent(fatTap(10, 60));
EXPECT_TRUE(client.triggered());
}

TEST_F(WebFrameTest, DisambiguationPopupBlacklist)
{
const unsigned viewportWidth = 500;
Expand Down Expand Up @@ -5866,9 +5799,9 @@ TEST_F(WebFrameTest, FrameViewSetFrameRect)

FrameView* frameView = webViewHelper.webViewImpl()->mainFrameImpl()->frameView();
frameView->setFrameRect(IntRect(0, 0, 200, 200));
EXPECT_RECT_EQ(IntRect(0, 0, 200, 200), frameView->frameRect());
EXPECT_EQ_RECT(IntRect(0, 0, 200, 200), frameView->frameRect());
frameView->setFrameRect(IntRect(100, 100, 200, 200));
EXPECT_RECT_EQ(IntRect(100, 100, 200, 200), frameView->frameRect());
EXPECT_EQ_RECT(IntRect(100, 100, 200, 200), frameView->frameRect());
}

TEST_F(WebFrameTest, FullscreenLayerNonScrollable)
Expand Down
37 changes: 0 additions & 37 deletions Source/web/tests/data/disambiguation_popup_200_by_800.html
View file

This file was deleted.

6 changes: 0 additions & 6 deletions public/web/WebViewClient.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,15 +204,9 @@ class WebViewClient : virtual public WebWidgetClient {
// unless the view did not need a layout.
virtual void didUpdateLayout() { }

// FIXME: This is a deprecated method. Remove it after the chromium change
// that uses the second didTapMultipleTargets() propagates.
//
// Return true to swallow the input event if the embedder will start a disambiguation popup
virtual bool didTapMultipleTargets(const WebGestureEvent&, const WebVector<WebRect>& targetRects) { return false; }

// Return true to swallow the input event if the embedder will start a disambiguation popup
virtual bool didTapMultipleTargets(const WebSize& pinchViewportOffset, const WebRect& touchRect, const WebVector<WebRect>& targetRects) { return false; }

// Returns comma separated list of accept languages.
virtual WebString acceptLanguages() { return WebString(); }

Expand Down

0 comments on commit 79bab42

Please sign in to comment.