Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2681803002:
blink: clear tooltips on frame leaves (Closed)
|
Created:
3 years, 10 months ago by Elly Fong-Jones Modified:
3 years, 10 months ago CC:
chromium-reviews, blink-reviews, dtapuska+blinkwatch_chromium.org, Navid Zolghadr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionblink: clear tooltips on frame leaves
If the mouse exits a frame fast enough, EventHandler can receive a
MouseLeave event without any intervening MouseMoved events. In this case, if
there was a tooltip for a previously hovered tooltip, the tooltip does not get
dismissed, which can cause it to draw over other apps or windows. The solution
is to dismiss any active tooltip for a frame when the mouse leaves that frame.
BUG=592085
Review-Url: https://codereview.chromium.org/2681803002
Cr-Commit-Position: refs/heads/master@{#452130}
Committed: https://chromium.googlesource.com/chromium/src/+/a741cc8edf82ff1bf0850f9f8756419d6eaf60d0
Patch Set 1 #
Total comments: 2
Patch Set 2 : move clearToolTip call #Patch Set 3 : add unit test #
Total comments: 1
Patch Set 4 : rebase and nullptr -> WTF::String #
Messages
Total messages: 21 (7 generated)
Description was changed from ========== blink: clear tooltips on frame leaves If the mouse exits a frame fast enough, EventHandler can receive a MouseLeave event without any intervening MouseMoved events. In this case, if there was a tooltip for a previously hovered tooltip, the tooltip does not get dismissed, which can cause it to draw over other apps or windows. The solution is to dismiss any active tooltip for a frame when the mouse leaves that frame. BUG=592085 ========== to ========== blink: clear tooltips on frame leaves If the mouse exits a frame fast enough, EventHandler can receive a MouseLeave event without any intervening MouseMoved events. In this case, if there was a tooltip for a previously hovered tooltip, the tooltip does not get dismissed, which can cause it to draw over other apps or windows. The solution is to dismiss any active tooltip for a frame when the mouse leaves that frame. BUG=592085 ==========
ellyjones@chromium.org changed reviewers: + eae@chromium.org
eae: ptal? :)
Looks reasonable to me but I'm not very familiar with this code. dtapuska or bokan more be more appropriate reviewers.
eae@chromium.org changed reviewers: + bokan@chromium.org, dtapuska@chromium.org
Can we get a test? Also, in the bug it seems like this can happen when we switch tabs, do we get a mouse leave when switching tabs? https://codereview.chromium.org/2681803002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2681803002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:766: page->chromeClient().mouseDidLeave(*m_frame); just call chromeClient().clearToolTip() directly from here, I'd rather not have chromeClient become another "event handler".
On 2017/02/07 22:36:26, bokan wrote: > Can we get a test? Okay, but I think I need a bit of Blink advice. The logical place for this test would be EventHandlerTest, but I'm not sure how to get at the tooltip text so I can ensure it's being cleared. There is another test that deals with tooltip text, but it is for ChromeClient and seems to need a subclass of ChromeClient. Is the best way to write this to create a similar subclass? If so, how do I stick it into the Page? > Also, in the bug it seems like this can happen when we switch tabs, do we get a > mouse leave when switching tabs? I don't see why it couldn't, but I can't seem to make the bug happen, so perhaps there is something else going on there. > https://codereview.chromium.org/2681803002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/input/EventHandler.cpp (right): > > https://codereview.chromium.org/2681803002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/input/EventHandler.cpp:766: > page->chromeClient().mouseDidLeave(*m_frame); > just call chromeClient().clearToolTip() directly from here, I'd rather not have > chromeClient become another "event handler". Done.
https://codereview.chromium.org/2681803002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2681803002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:766: page->chromeClient().mouseDidLeave(*m_frame); On 2017/02/07 22:36:25, bokan wrote: > just call chromeClient().clearToolTip() directly from here, I'd rather not have > chromeClient become another "event handler". Done.
On 2017/02/09 19:20:46, Elly Fong-Jones wrote: > On 2017/02/07 22:36:26, bokan wrote: > > Can we get a test? > > Okay, but I think I need a bit of Blink advice. The logical place for this test > would be EventHandlerTest, but I'm not sure how to get at the tooltip text so I > can ensure it's being cleared. There is another test that deals with tooltip > text, but it is for ChromeClient and seems to need a subclass of ChromeClient. > Is the best way to write this to create a similar subclass? If so, how do I > stick it into the Page? See https://codereview.chromium.org/2680973010/diff/40001/third_party/WebKit/Sour... for an example of how to inject your ChromeClient into the test. I would say just Mock the clearToolTip method and EXPECT_CALL on it (e.g. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/tests/WebF...) > > Also, in the bug it seems like this can happen when we switch tabs, do we get > a > > mouse leave when switching tabs? > > I don't see why it couldn't, but I can't seem to make the bug happen, so perhaps > there is something else going on there. Ok, lets land this and see if the reporter can still repro.
On 2017/02/10 00:32:59, bokan (OOO until Feb 21) wrote: unit test added, ptal :)
Sorry about delay, was OOO. lgtm % comment. https://codereview.chromium.org/2681803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2681803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:475: EXPECT_EQ(nullptr, lastToolTip()); We're comparing a String ref to nullptr here and below. I'm guessing String casts a nullptr to an empty string but it'd be more correct to compare lastToolTip to "".
On 2017/02/21 19:15:43, bokan wrote: > Sorry about delay, was OOO. > > lgtm % comment. > > https://codereview.chromium.org/2681803002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): > > https://codereview.chromium.org/2681803002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/EventHandlerTest.cpp:475: > EXPECT_EQ(nullptr, lastToolTip()); > We're comparing a String ref to nullptr here and below. I'm guessing String > casts a nullptr to an empty string but it'd be more correct to compare > lastToolTip to "". I thought this at first as well, but strangely, it doesn't: [ RUN ] EventHandlerTooltipTest.mouseLeaveClearsTooltip ../../third_party/WebKit/Source/core/input/EventHandlerTest.cpp:491: Failure Value of: lastToolTip() Actual: <null> Expected: "" Which is: 0x144ee2b String::String(UChar*) leaves m_impl in the constructed string as nullptr, and then String::operator<< emits the literal text "<null>" for it. It seems WTF::String() and WTF::String("") are considered distinct, and the null reference turns into the null string, not the empty string.
On 2017/02/22 16:06:34, Elly Fong-Jones wrote: > On 2017/02/21 19:15:43, bokan wrote: > > Sorry about delay, was OOO. > > > > lgtm % comment. > > > > > https://codereview.chromium.org/2681803002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): > > > > > https://codereview.chromium.org/2681803002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/input/EventHandlerTest.cpp:475: > > EXPECT_EQ(nullptr, lastToolTip()); > > We're comparing a String ref to nullptr here and below. I'm guessing String > > casts a nullptr to an empty string but it'd be more correct to compare > > lastToolTip to "". > > I thought this at first as well, but strangely, it doesn't: > > [ RUN ] EventHandlerTooltipTest.mouseLeaveClearsTooltip > ../../third_party/WebKit/Source/core/input/EventHandlerTest.cpp:491: Failure > Value of: lastToolTip() > Actual: <null> > Expected: "" > Which is: 0x144ee2b > > String::String(UChar*) leaves m_impl in the constructed string as nullptr, and > then String::operator<< emits the literal text "<null>" for it. It seems > WTF::String() and WTF::String("") are considered distinct, and the null > reference turns into the null string, not the empty string. Oh, surprising. Ok, could we compare to WTF::String() rather than nullptr?
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/2681803002/#ps60001 (title: "rebase and nullptr -> WTF::String")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487781155284470,
"parent_rev": "fcccfa88dfd370bd07b02169ef1b3d6b0ef47425", "commit_rev":
"a741cc8edf82ff1bf0850f9f8756419d6eaf60d0"}
Message was sent while issue was closed.
Description was changed from ========== blink: clear tooltips on frame leaves If the mouse exits a frame fast enough, EventHandler can receive a MouseLeave event without any intervening MouseMoved events. In this case, if there was a tooltip for a previously hovered tooltip, the tooltip does not get dismissed, which can cause it to draw over other apps or windows. The solution is to dismiss any active tooltip for a frame when the mouse leaves that frame. BUG=592085 ========== to ========== blink: clear tooltips on frame leaves If the mouse exits a frame fast enough, EventHandler can receive a MouseLeave event without any intervening MouseMoved events. In this case, if there was a tooltip for a previously hovered tooltip, the tooltip does not get dismissed, which can cause it to draw over other apps or windows. The solution is to dismiss any active tooltip for a frame when the mouse leaves that frame. BUG=592085 Review-Url: https://codereview.chromium.org/2681803002 Cr-Commit-Position: refs/heads/master@{#452130} Committed: https://chromium.googlesource.com/chromium/src/+/a741cc8edf82ff1bf0850f9f8756... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a741cc8edf82ff1bf0850f9f8756... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
