Send an empty tooltip text when hovering over a different node

Send an empty tooltip text when hovering over a different node

Chromium differs from IE/Edge in the way it handles
tooltip placement when moving the mouse cursor
over different node elements that have the same tooltip
text.

- Chromium: as long that the tooltip text does not change,
the tooltip banner stays stationary when hovering over
different nodes.

- IE/Edge: we the mouse cursor hovers over different node than
before, the tooltip gets hidden regardless whether the its
text has changed or not.

This CL makes Chromium behave similarly to IE.

Note: Although this CL sends an extra "clear tooltip text" IPC message,
it intentionally only happens in one specific situation: when 
mouse cursor is hovering over a node different that before that
*has* the same tooltip text as before, *and* the the text is
non-empty.
It is unlikely that this will flood the IPC channel with more 
tooltip messages.

BUG=142002, 157122, 158320
Review-Url: https://codereview.chromium.org/2592263003
Cr-Commit-Position: refs/heads/master@{#442570}
Committed: https://chromium.googlesource.com/chromium/src/+/1a401559f9ae0048e14d10ce1c980145a198d577

[tonikitoo, 2016-12-22 12:46:01]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 12:46:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Manuel Rego, 2016-12-22 13:35:30]

Description was changed from

==========
Send an empty tooltip when the mouse cursor moves over a different node.

The CL sends an empty tooltip, when one hovers over a new node,
and the previous node contained a tooltip.

Goal here is to hide the previous tooltip. If the new node also contains
a tooltip, ::sendToolTip will be called later with the new text.

Reference bug: https://bugs.webkit.org/show_bug.cgi?id=84375

BUG=142002
==========

to

==========
Send an empty tooltip when the mouse cursor moves over a different node.

The CL sends an empty tooltip, when one hovers over a new node,
and the previous node contained a tooltip.

Goal here is to hide the previous tooltip. If the new node also contains
a tooltip, ::sendToolTip will be called later with the new text.

Reference bug: https://bugs.webkit.org/show_bug.cgi?id=84375

BUG=142002
==========

[commit-bot: I haz the power, 2016-12-22 14:08:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 14:08:48]

Dry run: This issue passed the CQ dry run.

[tonikitoo, 2016-12-22 20:29:26]

Description was changed from

==========
Send an empty tooltip when the mouse cursor moves over a different node.

The CL sends an empty tooltip, when one hovers over a new node,
and the previous node contained a tooltip.

Goal here is to hide the previous tooltip. If the new node also contains
a tooltip, ::sendToolTip will be called later with the new text.

Reference bug: https://bugs.webkit.org/show_bug.cgi?id=84375

BUG=142002
==========

to

==========
Hide tooltips on mouse move

Chromium behaves different from Firefox with regards
to the way tooltips are hidden (or not) when moving
the mouse cursor.

In Firefox, whenever one moves the mouse cursor, the
tooltip gets hidden. On Chromium, a tooltip only hides
in response to a mouse move, if

1) the new target has a "title" attribute value different than
the previous.
2) the mouse cursor is moved over an element without the "title"
attribute set. In this case, an empty string is actually sent from
the Renderer to the Browser, so that it hides the tooltip.

This CL changes Chromium's current behavior by hiding tooltip upon
mouse move events. Once the cursor gets stationary again, and the
"show tooltip" timer times out, the tooltip is shown again, if any.

BUG=142002,560145,157122,158320
==========

[tonikitoo, 2016-12-22 20:31:08]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-22 20:31:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tonikitoo, 2016-12-22 20:36:01]

tonikitoo@igalia.com changed reviewers:
+ oshima@chromium.org, sadrul@chromium.org

[commit-bot: I haz the power, 2016-12-22 21:12:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-22 21:12:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sadrul, 2016-12-23 15:24:23]

The try failures look relevant. Can you please take a look at them?

This changes behaviour on Windows too, right? I don't know that that's something
we want to do. And between matching behaviour with firefox-linux vs. with
chrome-windows, we probably want to go with chrome-windows for chrome-linux,
simply because that's easier to maintain.

[tonikitoo, 2016-12-23 19:31:28]

On 2016/12/23 15:24:23, sadrul wrote:
> The try failures look relevant. Can you please take a look at them?

Right, sticking with this approach would require a change in some expected
results.
 
> This changes behaviour on Windows too, right?

Yes.

> I don't know that that's something
> we want to do. And between matching behaviour with firefox-linux vs. with
> chrome-windows, we probably want to go with chrome-windows for chrome-linux,
> simply because that's easier to maintain.

Ok. Additionally, Firefox hide-tooltip-on-mouse-move behavior is different than
IE11 (I have not tested Edge).

I will try an alternative approach shortly, which is simpler.

[tonikitoo, 2016-12-28 12:08:31]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-28 12:08:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tonikitoo, 2016-12-28 12:11:57]

Description was changed from

==========
Hide tooltips on mouse move

Chromium behaves different from Firefox with regards
to the way tooltips are hidden (or not) when moving
the mouse cursor.

In Firefox, whenever one moves the mouse cursor, the
tooltip gets hidden. On Chromium, a tooltip only hides
in response to a mouse move, if

1) the new target has a "title" attribute value different than
the previous.
2) the mouse cursor is moved over an element without the "title"
attribute set. In this case, an empty string is actually sent from
the Renderer to the Browser, so that it hides the tooltip.

This CL changes Chromium's current behavior by hiding tooltip upon
mouse move events. Once the cursor gets stationary again, and the
"show tooltip" timer times out, the tooltip is shown again, if any.

BUG=142002,560145,157122,158320
==========

to

==========
Send an empty tooltip text when hoverring over a different node

Chromium differs from IE/Edge in the way it handles
tooltip placement when moving the mouse cursor
over different node elements that have the same tooltip
text.

- Chromium: as long that the tooltip text does not change,
the tooltip banner stays stationary when hoverring over
different nodes.

- IE/Edge: we the mouse cursor hovers over different node than
before, the tooltip gets hidden regardless whether the tooltip
text has changed or not.

This CL makes Chromium behave similarly to IE. 

BUG=142002,157122,158320
==========

[tonikitoo, 2016-12-28 12:12:31]

Description was changed from

==========
Send an empty tooltip text when hoverring over a different node

Chromium differs from IE/Edge in the way it handles
tooltip placement when moving the mouse cursor
over different node elements that have the same tooltip
text.

- Chromium: as long that the tooltip text does not change,
the tooltip banner stays stationary when hoverring over
different nodes.

- IE/Edge: we the mouse cursor hovers over different node than
before, the tooltip gets hidden regardless whether the tooltip
text has changed or not.

This CL makes Chromium behave similarly to IE. 

BUG=142002,157122,158320
==========

to

==========
Send an empty tooltip text when hovering over a different node

Chromium differs from IE/Edge in the way it handles
tooltip placement when moving the mouse cursor
over different node elements that have the same tooltip
text.

- Chromium: as long that the tooltip text does not change,
the tooltip banner stays stationary when hovering over
different nodes.

- IE/Edge: we the mouse cursor hovers over different node than
before, the tooltip gets hidden regardless whether the its
text has changed or not.

This CL makes Chromium behave similarly to IE. 

BUG=142002,157122,158320
==========

[tonikitoo, 2016-12-28 12:15:02]

tonikitoo@igalia.com changed reviewers:
+ tkent@chromium.org

[tonikitoo, 2016-12-28 12:31:55]

Description was changed from

==========
Send an empty tooltip text when hovering over a different node

Chromium differs from IE/Edge in the way it handles
tooltip placement when moving the mouse cursor
over different node elements that have the same tooltip
text.

- Chromium: as long that the tooltip text does not change,
the tooltip banner stays stationary when hovering over
different nodes.

- IE/Edge: we the mouse cursor hovers over different node than
before, the tooltip gets hidden regardless whether the its
text has changed or not.

This CL makes Chromium behave similarly to IE. 

BUG=142002,157122,158320
==========

to

==========
Send an empty tooltip text when hovering over a different node

Chromium differs from IE/Edge in the way it handles
tooltip placement when moving the mouse cursor
over different node elements that have the same tooltip
text.

- Chromium: as long that the tooltip text does not change,
the tooltip banner stays stationary when hovering over
different nodes.

- IE/Edge: we the mouse cursor hovers over different node than
before, the tooltip gets hidden regardless whether the its
text has changed or not.

This CL makes Chromium behave similarly to IE.

Note: Although this CL sends an extra "clear tooltip text" IPC message,
it intentionally only happens in one specific situation: when 
mouse cursor is hovering over a node different that before that
*has* the same tooltip text as before, *and* the the text is
non-empty.
It is unlikely that this will flood the IPC channel with more 
tooltip messages.

BUG=142002,157122,158320
==========

[tonikitoo, 2016-12-28 12:32:32]

tkent: PTAL

[commit-bot: I haz the power, 2016-12-28 13:49:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-28 13:49:20]

Dry run: This issue passed the CQ dry run.

[tonikitoo, 2017-01-02 17:35:59]

The CQ bit was checked by tonikitoo@igalia.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-02 17:36:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-02 20:00:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-02 20:00:26]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-01-03 17:04:55]

sadrul@chromium.org changed reviewers:
- sadrul@chromium.org

[sadrul, 2017-01-03 17:04:56]

Since this is now in blink land, I am not a good reviewer for this change
anymore. Removing myself from the reviewer list.

[Stephen Chennney, 2017-01-03 20:38:24]

schenney@chromium.org changed reviewers:
+ schenney@chromium.org

[Stephen Chennney, 2017-01-03 20:38:25]

I'm OK with this, but it's not really my area of specialty.

https://codereview.chromium.org/2592263003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ChromeClient.h (right):

https://codereview.chromium.org/2592263003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ChromeClient.h:363: WeakMember<Node>
m_lastMouseOverNode;
Does this really need to be a weak pointer, rather than a raw pointer? You only
ever use it for comparing the pointer value, never to access, so it seems to me
that a raw pointer would work.

[tonikitoo, 2017-01-04 19:35:12]

On 2017/01/03 20:38:25, Stephen Chennney wrote:
> I'm OK with this, but it's not really my area of specialty.
> 
>
https://codereview.chromium.org/2592263003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/page/ChromeClient.h (right):
> 
>
https://codereview.chromium.org/2592263003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/page/ChromeClient.h:363: WeakMember<Node>
> m_lastMouseOverNode;
> Does this really need to be a weak pointer, rather than a raw pointer? You
only
> ever use it for comparing the pointer value, never to access, so it seems to
me
> that a raw pointer would work.

@schenney, thanks for looking.

Actually, at ChromeClient (and other classes in the same inheritance chain)
"can't contain raw pointers". For instance, this is the compilation error:

../../<path>/src/third_party/WebKit/Source/core/page/ChromeClient.h:84:1: error:
[blink-gc] Class 'ChromeClient' contains invalid fields.
class CORE_EXPORT ChromeClient : public HostWindow {
^
../../<path>/src/third_party/WebKit/Source/core/page/ChromeClient.h:366:3: note:
[blink-gc] Raw pointer field 'm_lastMouseOverNode' to a GC managed class
declared here:
  Node* m_lastMouseOverNode = nullptr;
  ^
1 error generated.
ninja: build stopped: subcommand failed

[oshima, 2017-01-05 22:33:02]

I'm not the owner, but I agree that this is better behavior.

Just FYI. Browser UI has slightly different behavior.
Once tooltip is shown, it gets hidden only when you move the mouse to the
element that does not have tooltip, not when the mouse is moved out.

This may not work well on a page where many of its elements have tooltip, so
this different is probably reasonable.

[tonikitoo, 2017-01-06 01:43:00]

On 2017/01/05 22:33:02, oshima wrote:
> I'm not the owner, but I agree that this is better behavior.
> 
> Just FYI. Browser UI has slightly different behavior.
> Once tooltip is shown, it gets hidden only when you move the mouse to the
> element that does not have tooltip, not when the mouse is moved out.
> 
> This may not work well on a page where many of its elements have tooltip, so
> this different is probably reasonable.

Thanks Oshima. I believe things could evolve to an uniform behavior all over
Chrome (UI and Web content), as we understand better how we want this to work.

In any case, I see this specific CL as a progression towards an better behavior.

[tkent, 2017-01-10 07:01:47]

lgtm

[tonikitoo, 2017-01-10 12:03:47]

The CQ bit was checked by tonikitoo@igalia.com

[commit-bot: I haz the power, 2017-01-10 12:03:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 13:32:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484049827080300,
"parent_rev": "1be376e34def3865b168d419e415ee4b6df63f6e", "commit_rev":
"1a401559f9ae0048e14d10ce1c980145a198d577"}

[commit-bot: I haz the power, 2017-01-10 13:32:49]

Description was changed from

==========
Send an empty tooltip text when hovering over a different node

Chromium differs from IE/Edge in the way it handles
tooltip placement when moving the mouse cursor
over different node elements that have the same tooltip
text.

- Chromium: as long that the tooltip text does not change,
the tooltip banner stays stationary when hovering over
different nodes.

- IE/Edge: we the mouse cursor hovers over different node than
before, the tooltip gets hidden regardless whether the its
text has changed or not.

This CL makes Chromium behave similarly to IE.

Note: Although this CL sends an extra "clear tooltip text" IPC message,
it intentionally only happens in one specific situation: when 
mouse cursor is hovering over a node different that before that
*has* the same tooltip text as before, *and* the the text is
non-empty.
It is unlikely that this will flood the IPC channel with more 
tooltip messages.

BUG=142002,157122,158320
==========

to

==========
Send an empty tooltip text when hovering over a different node

Chromium differs from IE/Edge in the way it handles
tooltip placement when moving the mouse cursor
over different node elements that have the same tooltip
text.

- Chromium: as long that the tooltip text does not change,
the tooltip banner stays stationary when hovering over
different nodes.

- IE/Edge: we the mouse cursor hovers over different node than
before, the tooltip gets hidden regardless whether the its
text has changed or not.

This CL makes Chromium behave similarly to IE.

Note: Although this CL sends an extra "clear tooltip text" IPC message,
it intentionally only happens in one specific situation: when 
mouse cursor is hovering over a node different that before that
*has* the same tooltip text as before, *and* the the text is
non-empty.
It is unlikely that this will flood the IPC channel with more 
tooltip messages.

BUG=142002,157122,158320

Review-Url: https://codereview.chromium.org/2592263003
Cr-Commit-Position: refs/heads/master@{#442570}
Committed:
https://chromium.googlesource.com/chromium/src/+/1a401559f9ae0048e14d10ce1c98...
==========

[commit-bot: I haz the power, 2017-01-10 13:32:50]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1a401559f9ae0048e14d10ce1c98...