Skip to content

Commit

Permalink
Hide tooltips while scrolling.
Browse files Browse the repository at this point in the history
BUG=586852
TEST=FrameViewTest.HideTooltipWhenScrollPositionChanges

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

Cr-Commit-Position: refs/heads/master@{#378446}
(cherry picked from commit 933d8eb)

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

Cr-Commit-Position: refs/branch-heads/2661@{crosswalk-project#182}
Cr-Branched-From: ef6f6ae-refs/heads/master@{#378081}
  • Loading branch information
tdresser committed Mar 10, 2016
1 parent 45b1944 commit 43512ed
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 4 deletions.
3 changes: 3 additions & 0 deletions third_party/WebKit/Source/core/frame/FrameView.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1590,6 +1590,9 @@ void FrameView::scrollPositionChanged()
document->enqueueScrollEventForNode(document);

m_frame->eventHandler().dispatchFakeMouseMoveEventSoon();
Page* page = frame().page();
if (page)
page->chromeClient().clearToolTip();

if (LayoutView* layoutView = document->layoutView()) {
if (layoutView->usesCompositing())
Expand Down
21 changes: 17 additions & 4 deletions third_party/WebKit/Source/core/frame/FrameViewTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,6 @@
#include "testing/gtest/include/gtest/gtest.h"
#include "wtf/OwnPtr.h"

// This test ensures that FrameView informs the ChromeClient of changes to the
// paint artifact so that they can be shown to the user (e.g. via the
// compositor).

using testing::_;
using testing::AnyNumber;

Expand All @@ -41,6 +37,7 @@ class MockChromeClient : public EmptyChromeClient {
}
MOCK_METHOD1(didPaint, void(const PaintArtifact&));
MOCK_METHOD2(attachRootGraphicsLayer, void(GraphicsLayer*, LocalFrame* localRoot));
MOCK_METHOD2(setToolTip, void(const String&, TextDirection));

void scheduleAnimation(Widget*) override { m_hasScheduledAnimation = true; }
bool m_hasScheduledAnimation;
Expand All @@ -52,6 +49,11 @@ class FrameViewTestBase : public testing::Test {
: m_chromeClient(adoptPtrWillBeNoop(new MockChromeClient))
{ }

~FrameViewTestBase()
{
testing::Mock::VerifyAndClearExpectations(&chromeClient());
}

void SetUp() override
{
Page::PageClients clients;
Expand Down Expand Up @@ -103,6 +105,9 @@ class FrameViewSlimmingPaintV2Test : public FrameViewTestBase {
RuntimeEnabledFeatures::Backup m_featuresBackup;
};

// These tests ensure that FrameView informs the ChromeClient of changes to the
// paint artifact so that they can be shown to the user (e.g. via the
// compositor).
TEST_F(FrameViewSlimmingPaintV2Test, PaintOnce)
{
EXPECT_CALL(chromeClient(), didPaint(_));
Expand Down Expand Up @@ -144,5 +149,13 @@ TEST_F(FrameViewTest, SetPaintInvalidationOutOfUpdateAllLifecyclePhases)
EXPECT_FALSE(chromeClient().m_hasScheduledAnimation);
}

// If we don't hide the tooltip on scroll, it can negatively impact scrolling
// performance. See crbug.com/586852 for details.
TEST_F(FrameViewTest, HideTooltipWhenScrollPositionChanges)
{
EXPECT_CALL(chromeClient(), setToolTip(String(), _));
document().view()->scrollTo(DoublePoint(1, 1));
}

} // namespace
} // namespace blink
4 changes: 4 additions & 0 deletions ui/views/corewm/tooltip_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,10 @@ void TooltipController::OnMouseEvent(ui::MouseEvent* event) {
// Hide the tooltip for click, release, drag, wheel events.
if (tooltip_->IsVisible())
tooltip_->Hide();

// Don't reshow the tooltip during scroll.
if (tooltip_timer_.IsRunning())
tooltip_timer_.Reset();
break;
default:
break;
Expand Down

0 comments on commit 43512ed

Please sign in to comment.