Resume cursor blinking on closing context menu

Resume cursor blinking on closing context menu

Currently, we suspend cursor blinking on mouse press and resume it on mouse
release. However, in the case of launching a context menu, we show context
menu immediately on mouse right click press, and the following key events
are consumed on the context menu, so mouse release event does not happen.
Thus, cursor blinking does not get resumed after context menu gets
dismissed.

The rationale for keep suspending caret blinking while showing context
menu, I think, is that once context menu shows, key and mouse events will
be consumed on the context menu on linux. We keep that behavior, and
de-suspend caret blinking when context menu gets closed.

On Linux, we will resume caret blinking in
RenderFrameImpl::OnContextMenuClosed().

On Android, there are two different paths to show context menu:
- paste popup menu (long press on text, on chrome and webview)
- context menu (long press on link, only on chrome)
So we will call WebContents#onContextMenuClosed() from both paths.

BUG=554683
Committed: https://crrev.com/cbfdec91085fba9e23b865c0db6cac2577d3ae44
Cr-Commit-Position: refs/heads/master@{#363360}

[Changwan Ryu, 2015-11-24 01:59:40]

changwan@chromium.org changed reviewers:
+ aelias@chromium.org

[Changwan Ryu, 2015-11-24 01:59:41]

aelias@, could you take an initial look? does this make sense to you?

[aelias_OOO_until_Jul13, 2015-11-25 03:48:16]

aelias@chromium.org changed reviewers:
+ rbyers@chromium.org, sievers@chromium.org, tedchoc@chromium.org

[aelias_OOO_until_Jul13, 2015-11-25 03:48:46]

lgtm, adding OWNERS:
- tedchoc@ for Android browser-side code
- sievers@ for content/renderer
- rbyers@ for public/web

https://codereview.chromium.org/1471993002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/1471993002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1655: mouseEvent.type =
WebInputEvent::MouseDown;
Could you also send the MouseUp event for realism here?

[Changwan Ryu, 2015-11-26 00:01:47]

https://codereview.chromium.org/1471993002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/WebViewTest.cpp (right):

https://codereview.chromium.org/1471993002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/WebViewTest.cpp:1655: mouseEvent.type =
WebInputEvent::MouseDown;
On 2015/11/25 03:48:46, aelias wrote:
> Could you also send the MouseUp event for realism here?

Actually, MouseUp will be consumed by context menu once it shows up so I didn't
add it. I've updated comment to make it clear.

[Rick Byers, 2015-11-27 02:44:49]

Interesting, thanks!  I added some more comments on the bug based on some
experiments.  I believe this patch will fix the issue I'm seeing on Windows too.

WebKit LGTM

[no sievers, 2015-11-30 21:25:00]

https://codereview.chromium.org/1471993002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1471993002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1588:
public void onContextMenuClosed(Menu menu) {
I can't find where this is overriden from.

[Ted C, 2015-11-30 21:30:49]

https://codereview.chromium.org/1471993002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1471993002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1588:
public void onContextMenuClosed(Menu menu) {
Hmm...it is sad that we have to add this at this high of a level, but you can't
add a dismiss listener on the Menu during construction, so I think this will
have to do.

Alternatively, you could listen for a combination of window focus/window
visibility at the view level and gather the same amount of information.  I would
assume you'd lose focus when showing the context menu and either gain it back or
lose visibility entirely depending on the user action.

https://codereview.chromium.org/1471993002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1593:
webContents.onContextMenuClosed();
What happens if the web contents was not the thing that triggered the context
menu?  Could we be incorrectly causing the cursor to blink if something else
triggered one (or does it track whether it was in fact showing a context menu)?

Also, WebView does not have a ChromeActivity, so if you need this to be called
then this will not work like this.

https://codereview.chromium.org/1471993002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1471993002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2571:
if (mWebContents != null) mWebContents.onContextMenuClosed();
What is causing the cursor to pause blinking?  Do we do that any time we long
press something?

https://codereview.chromium.org/1471993002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/LegacyPastePopupMenu.java
(right):

https://codereview.chromium.org/1471993002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/LegacyPastePopupMenu.java:49:
@Override
-4 indent

https://codereview.chromium.org/1471993002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/1471993002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:326:
* Called when context menu gets closed.
based on my previous comment in the Activity, this may be caused when some other
context menu is closed so we might want to say that in the comment here.

[Changwan Ryu, 2015-12-01 05:54:01]

https://codereview.chromium.org/1471993002/diff/40001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1471993002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1588:
public void onContextMenuClosed(Menu menu) {
On 2015/11/30 21:25:00, sievers wrote:
> I can't find where this is overriden from.

This overrides Activity#onContextMenuClosed. Added javadoc.

https://codereview.chromium.org/1471993002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1588:
public void onContextMenuClosed(Menu menu) {
On 2015/11/30 21:30:49, Ted C wrote:
> Hmm...it is sad that we have to add this at this high of a level, but you
can't
> add a dismiss listener on the Menu during construction, so I think this will
> have to do.
> 
> Alternatively, you could listen for a combination of window focus/window
> visibility at the view level and gather the same amount of information.  I
would
> assume you'd lose focus when showing the context menu and either gain it back
or
> lose visibility entirely depending on the user action.

Hmm.. I considered it, but if context menu opens yet another view, then
depending on the timing the original view may not get window focus or visibility
at the right timing (because it was already lost when context menu pops up), and
the callback will only be called when the user comes back to the original view.
Then the timing of callback may be a bit off, although I cannot think of any
disastrous scenario out of it, but I'm afraid that it might introduce
inconsistency of behavior.

https://codereview.chromium.org/1471993002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1593:
webContents.onContextMenuClosed();
On 2015/11/30 21:30:49, Ted C wrote:
> What happens if the web contents was not the thing that triggered the context
> menu?  Could we be incorrectly causing the cursor to blink if something else
> triggered one (or does it track whether it was in fact showing a context
menu)?

Good point! I've added WebContents#onContextMenuOpened() to prevent this case.
Since there can be only one context menu at a time, we won't get context menu
callbacks from other places.

> 
> Also, WebView does not have a ChromeActivity, so if you need this to be called
> then this will not work like this.

AwWebContentsViewDelegate does not trigger ContextMenuHelper, so we won't have
this problem.

https://codereview.chromium.org/1471993002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/1471993002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2571:
if (mWebContents != null) mWebContents.onContextMenuClosed();
On 2015/11/30 21:30:49, Ted C wrote:
> What is causing the cursor to pause blinking?  Do we do that any time we long
> press something?

We handle long press gesture and send context menu event in blink::EventHandler.
In doing so, we simulate mouse right button press behavior (probably in order to
fire contextmenu event - http://www.quirksmode.org/dom/events/contextmenu.html)

https://codereview.chromium.org/1471993002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/input/LegacyPastePopupMenu.java
(right):

https://codereview.chromium.org/1471993002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/input/LegacyPastePopupMenu.java:49:
@Override
On 2015/11/30 21:30:49, Ted C wrote:
> -4 indent

Done.

https://codereview.chromium.org/1471993002/diff/40001/content/public/android/...
File
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java
(right):

https://codereview.chromium.org/1471993002/diff/40001/content/public/android/...
content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:326:
* Called when context menu gets closed.
On 2015/11/30 21:30:49, Ted C wrote:
> based on my previous comment in the Activity, this may be caused when some
other
> context menu is closed so we might want to say that in the comment here.

Done. Also, I've added onContextMenuOpened() to ensure that wrong events cannot
be fired.

[Changwan Ryu, 2015-12-02 02:10:42]

changwan@chromium.org changed reviewers:
+ sgurun@chromium.org

[Changwan Ryu, 2015-12-02 02:10:42]

adding boliu@ for android_webview/ change

[Changwan Ryu, 2015-12-02 02:12:05]

On 2015/12/02 02:10:42, Changwan Ryu wrote:
> adding boliu@ for android_webview/ change

I mean adding sgurun@. Sorry about this, I originally added boliu@ because of
his TODO but he's not available so I had to change reviewers.

[Ted C, 2015-12-02 04:19:27]

lgtm

[sgurun-gerrit only, 2015-12-02 22:22:25]

On 2015/12/02 04:19:27, Ted C wrote:
> lgtm

lgtm

[Changwan Ryu, 2015-12-05 00:19:45]

On 2015/12/02 22:22:25, sgurun wrote:
> On 2015/12/02 04:19:27, Ted C wrote:
> > lgtm
> 
> lgtm

ping? I still need approval for content/renderer/

sievers@, could you take a look?

[no sievers, 2015-12-05 00:51:06]

On 2015/12/05 00:19:45, Changwan Ryu wrote:
> On 2015/12/02 22:22:25, sgurun wrote:
> > On 2015/12/02 04:19:27, Ted C wrote:
> > > lgtm
> > 
> > lgtm
> 
> ping? I still need approval for content/renderer/
> 
> sievers@, could you take a look?

lgtm

[Changwan Ryu, 2015-12-06 06:28:46]

The CQ bit was checked by changwan@chromium.org

[Changwan Ryu, 2015-12-06 06:28:46]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1471993002/#ps60001
(title: "robusted onContextMenuClosed() and fixed nits")

[commit-bot: I haz the power, 2015-12-06 06:28:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1471993002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1471993002/60001

[commit-bot: I haz the power, 2015-12-06 10:06:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-12-06 10:07:06]

Description was changed from

==========
Resume cursor blinking on closing context menu

Currently, we suspend cursor blinking on mouse press and resume it on mouse
release. However, in the case of launching a context menu, we show context
menu immediately on mouse right click press, and the following key events
are consumed on the context menu, so mouse release event does not happen.
Thus, cursor blinking does not get resumed after context menu gets
dismissed.

The rationale for keep suspending caret blinking while showing context
menu, I think, is that once context menu shows, key and mouse events will
be consumed on the context menu on linux. We keep that behavior, and
de-suspend caret blinking when context menu gets closed.

On Linux, we will resume caret blinking in
RenderFrameImpl::OnContextMenuClosed().

On Android, there are two different paths to show context menu:
- paste popup menu (long press on text, on chrome and webview)
- context menu (long press on link, only on chrome)
So we will call WebContents#onContextMenuClosed() from both paths.

BUG=554683
==========

to

==========
Resume cursor blinking on closing context menu

Currently, we suspend cursor blinking on mouse press and resume it on mouse
release. However, in the case of launching a context menu, we show context
menu immediately on mouse right click press, and the following key events
are consumed on the context menu, so mouse release event does not happen.
Thus, cursor blinking does not get resumed after context menu gets
dismissed.

The rationale for keep suspending caret blinking while showing context
menu, I think, is that once context menu shows, key and mouse events will
be consumed on the context menu on linux. We keep that behavior, and
de-suspend caret blinking when context menu gets closed.

On Linux, we will resume caret blinking in
RenderFrameImpl::OnContextMenuClosed().

On Android, there are two different paths to show context menu:
- paste popup menu (long press on text, on chrome and webview)
- context menu (long press on link, only on chrome)
So we will call WebContents#onContextMenuClosed() from both paths.

BUG=554683

Committed: https://crrev.com/cbfdec91085fba9e23b865c0db6cac2577d3ae44
Cr-Commit-Position: refs/heads/master@{#363360}
==========

[commit-bot: I haz the power, 2015-12-06 10:07:07]

Patchset 4 (id:??) landed as
https://crrev.com/cbfdec91085fba9e23b865c0db6cac2577d3ae44
Cr-Commit-Position: refs/heads/master@{#363360}