Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
Do not show a tooltip same as the previous one.
Browse files Browse the repository at this point in the history
If a tooltip is closed by a keyboard event and the mouse cursor was
not moved, the same tooltip was shown again after some delay only on
Windows.  Blink receives native mousemove events repeatedly on Windows
even if the mouse cursor is not moved.

ChromeClient saves the last tooltip string and the last mouse position, and
do not call virtual setToolTip() if they are not changed. This CL not only
fixes the bug, but also reduces unnecessary IPC messages for tooltip.

BUG=557660
TEST=automated

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

Cr-Commit-Position: refs/heads/master@{#363174}
(cherry picked from commit 91db68e)

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

Cr-Commit-Position: refs/branch-heads/2564@{#248}
Cr-Branched-From: 1283eca-refs/heads/master@{#359700}
  • Loading branch information
tkent-google committed Dec 7, 2015
1 parent ae05498 commit 7d44e9f
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 1 deletion.
1 change: 1 addition & 0 deletions third_party/WebKit/Source/core/core.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -3896,6 +3896,7 @@
'loader/LinkHeaderTest.cpp',
'loader/LinkLoaderTest.cpp',
'loader/MixedContentCheckerTest.cpp',
'page/ChromeClientTest.cpp',
'page/ContextMenuControllerTest.cpp',
'page/NetworkStateNotifierTest.cpp',
'page/PagePopupClientTest.cpp',
Expand Down
2 changes: 1 addition & 1 deletion third_party/WebKit/Source/core/input/EventHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3151,7 +3151,7 @@ bool EventHandler::handleAccessKey(const PlatformKeyboardEvent& evt)
bool EventHandler::keyEvent(const PlatformKeyboardEvent& initialKeyEvent)
{
RefPtrWillBeRawPtr<FrameView> protector(m_frame->view());
m_frame->chromeClient().setToolTip(String(), LTR);
m_frame->chromeClient().clearToolTip();

if (initialKeyEvent.windowsVirtualKeyCode() == VK_CAPITAL)
capsLockStateMayHaveChanged();
Expand Down
11 changes: 11 additions & 0 deletions third_party/WebKit/Source/core/page/ChromeClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -167,9 +167,20 @@ void ChromeClient::setToolTip(const HitTestResult& result)
}
}

if (m_lastToolTipPoint == result.hitTestLocation().point() && m_lastToolTipText == toolTip)
return;
m_lastToolTipPoint = result.hitTestLocation().point();
m_lastToolTipText = toolTip;
setToolTip(toolTip, toolTipDirection);
}

void ChromeClient::clearToolTip()
{
// Do not check m_lastToolTip* and do not update them intentionally.
// We don't want to show tooltips with same content after clearToolTip().
setToolTip(String(), LTR);
}

void ChromeClient::print(LocalFrame* frame)
{
// Defer loads in case the client method runs a new event loop that would
Expand Down
7 changes: 7 additions & 0 deletions third_party/WebKit/Source/core/page/ChromeClient.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#ifndef ChromeClient_h
#define ChromeClient_h

#include "base/gtest_prod_util.h"
#include "core/CoreExport.h"
#include "core/dom/AXObjectCache.h"
#include "core/frame/ConsoleTypes.h"
Expand Down Expand Up @@ -155,6 +156,7 @@ class CORE_EXPORT ChromeClient : public HostWindow {

void mouseDidMoveOverElement(const HitTestResult&);
virtual void setToolTip(const String&, TextDirection) = 0;
void clearToolTip();

void print(LocalFrame*);

Expand Down Expand Up @@ -270,6 +272,11 @@ class CORE_EXPORT ChromeClient : public HostWindow {
private:
bool canOpenModalIfDuringPageDismissal(Frame* mainFrame, DialogType, const String& message);
void setToolTip(const HitTestResult&);

LayoutPoint m_lastToolTipPoint;
String m_lastToolTipText;

FRIEND_TEST_ALL_PREFIXES(ChromeClientTest, SetToolTipFlood);
};

} // namespace blink
Expand Down
70 changes: 70 additions & 0 deletions third_party/WebKit/Source/core/page/ChromeClientTest.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright 2015 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "config.h"
#include "core/page/ChromeClient.h"

#include "core/dom/Document.h"
#include "core/layout/HitTestResult.h"
#include "core/loader/EmptyClients.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace blink {

namespace {

class ChromeClientToolTipLogger : public EmptyChromeClient {
public:
void setToolTip(const String& text, TextDirection) override
{
m_toolTipForLastSetToolTip = text;
}

String toolTipForLastSetToolTip() const { return m_toolTipForLastSetToolTip; }
void clearToolTipForLastSetToolTip() { m_toolTipForLastSetToolTip = String(); }

private:
String m_toolTipForLastSetToolTip;
};

} // anonymous namespace

class ChromeClientTest : public testing::Test {
};

TEST_F(ChromeClientTest, SetToolTipFlood)
{
ChromeClientToolTipLogger logger;
ChromeClient* client = &logger;
HitTestResult result(HitTestRequest(HitTestRequest::Move), LayoutPoint(10, 20));
RefPtrWillBeRawPtr<Document> doc = Document::create();
RefPtrWillBeRawPtr<Element> element = HTMLElement::create(HTMLNames::divTag, *doc);
element->setAttribute(HTMLNames::titleAttr, "tooltip");
result.setInnerNode(element.get());

client->setToolTip(result);
EXPECT_EQ("tooltip", logger.toolTipForLastSetToolTip());

// seToolTip(HitTestResult) again in the same condition.
logger.clearToolTipForLastSetToolTip();
client->setToolTip(result);
// setToolTip(String,TextDirection) should not be called.
EXPECT_EQ(String(), logger.toolTipForLastSetToolTip());

// Cancel the tooltip, and setToolTip(HitTestResult) again.
client->clearToolTip();
logger.clearToolTipForLastSetToolTip();
client->setToolTip(result);
// setToolTip(String,TextDirection) should not be called.
EXPECT_EQ(String(), logger.toolTipForLastSetToolTip());

logger.clearToolTipForLastSetToolTip();
element->setAttribute(HTMLNames::titleAttr, "updated");
client->setToolTip(result);
// setToolTip(String,TextDirection) should be called because tooltip string
// is different from the last one.
EXPECT_EQ("updated", logger.toolTipForLastSetToolTip());
}

} // namespace blink

0 comments on commit 7d44e9f

Please sign in to comment.