Bug 69137 – In input field caret is not blinking after context menu hide

• Component: WebKit Misc.
• Status: NEW

[chandra shekar vallala]

On right click, mouse down event handler suspends the caret blink and opens the context menu. Mouse up event is not comming to renderer when context menu is opened. So caret blink suspend has not resumed.

To match the native application behavior, caretBlinkingSuspended is set to false on close of context menu.

Original issue is reported at http://code.google.com/p/chromium/issues/detail?id=98543

[chandra shekar vallala]

Created attachment 109272 [details]
patch with webkit changes

[Ryosuke Niwa]

Comment on attachment 109272 [details]
patch with webkit changes 

Why doesn't this problem reproduce on Mac? From what you've told me, Mac and Linux have the same behavior, right?

[Ryosuke Niwa]

To other reviewers, please don't r+ this patch for now

[chandra shekar vallala]

Created attachment 109479 [details]
updated the patch for mac/Linux.

[chandra shekar vallala]

(In reply to comment #2)
> (From update of attachment 109272 [details])
> Why doesn't this problem reproduce on Mac? From what you've told me, Mac and Linux have the same behavior, right?

Yes, Linux/Mac both have the same behavior since context menu come up before the mouse up. I updated the patch for mac and Linux.

[Ryosuke Niwa]

Comment on attachment 109479 [details]
updated the patch for mac/Linux.

This bug reproduces on Safari as well. We need to fix it in cross-platform code then.

[chandra shekar vallala]

(In reply to comment #6)
> (From update of attachment 109479 [details])
> This bug reproduces on Safari as well. We need to fix it in cross-platform code then.


To match the native app behaviour [caret should blink after the context menu hide which is same across all platforms] we should resume the caret blink after the menu hide.  Currently there is no way in which WebCore knows whether the context menu widget is hidden or not and it can even more troublesome in multi-process ports like Chromium or WebKit2 since widget is handled in different process. 

Instead we could resume caret after menu is dismissed which should be handled at WebKit port level rather than in WebCore. Also there might be usecases where action on menu will not dismiss the menu widget in which case with the WebCore solution we may move into an unwanted state of caret blinking even when menu is present.

My understanding is root cause of the bug is because of the way we handle the contextmenu creation/destroy which is defined WebKit port layer for each platform. So IMHO solution should also be in WebKit port layer rather than in WebCore.

[Ryosuke Niwa]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 109479 [details] [details])
> > This bug reproduces on Safari as well. We need to fix it in cross-platform code then.
> 
> To match the native app behaviour [caret should blink after the context menu hide which is same across all platforms] we should resume the caret blink after the menu hide.  Currently there is no way in which WebCore knows whether the context menu widget is hidden or not and it can even more troublesome in multi-process ports like Chromium or WebKit2 since widget is handled in different process.

So are you saying that ContextMenuController doesn't get notified at all? That's hard to believe. If anything, we should be adding some method in ContextMenuController to notify when the context menu hides.

> Instead we could resume caret after menu is dismissed which should be handled at WebKit port level rather than in WebCore.

That'll be a wrong abstraction layer to deal with this. Suspension of caret blinking is done in EventHandler, and resuming it in WebKit layer is strange to say the least.

> My understanding is root cause of the bug is because of the way we handle the contextmenu creation/destroy which is defined WebKit port layer for each platform.

Then we should add some notification mechanism so that we can notify either EventHandler or ContextMenuController when the context menu disappears. We should definitely NOT resume caret blinking in WebKit layer. In fact, I'd like to make setCaretBlinkingSuspended only accessible from EventHandler.

[chandra shekar vallala]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > (From update of attachment 109479 [details] [details] [details])
> > > This bug reproduces on Safari as well. We need to fix it in cross-platform code then.
> > 
> > To match the native app behaviour [caret should blink after the context menu hide which is same across all platforms] we should resume the caret blink after the menu hide.  Currently there is no way in which WebCore knows whether the context menu widget is hidden or not and it can even more troublesome in multi-process ports like Chromium or WebKit2 since widget is handled in different process.
> 
> So are you saying that ContextMenuController doesn't get notified at all? That's hard to believe. If anything, we should be adding some method in ContextMenuController to notify when the context menu hides.

Yes, ContextMenuController doesn't get notified from browser. So I added a contextMenuHidden function to controller. This should get called when menu hides.

> 
> > Instead we could resume caret after menu is dismissed which should be handled at WebKit port level rather than in WebCore.
> 
> That'll be a wrong abstraction layer to deal with this. Suspension of caret blinking is done in EventHandler, and resuming it in WebKit layer is strange to say the least.

Yes, your right. But I am not sure of adding a new function in EventHandler to resume/suspend the caret. EventHandler has a function called lostMouseCapture to do the same. The function name doesn't match to this case. I added a new one in EventHandler.

> 
> > My understanding is root cause of the bug is because of the way we handle the contextmenu creation/destroy which is defined WebKit port layer for each platform.
> 
> Then we should add some notification mechanism so that we can notify either EventHandler or ContextMenuController when the context menu disappears. We should definitely NOT resume caret blinking in WebKit layer. In fact, I'd like to make setCaretBlinkingSuspended only accessible from EventHandler.

To make the behavior same across all platforms, we should suspend/resume the caret on context menu show/hide respectively.

[chandra shekar vallala]

Created attachment 111226 [details]
updated patch

[Ryosuke Niwa]

Comment on attachment 111226 [details]
updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111226&action=review

> Source/WebCore/page/ContextMenuController.cpp:98
> +void ContextMenuController::contextMenuHidden()

"hidden" doesn't sound right to me. It's not like context menu is temporarily hidden from the screen. Dismissed or closed will be better.

> Source/WebCore/page/ContextMenuController.cpp:108
> +    // Resume the caret blinking which suspended in context menu show.

This comment repeats the code.

> Source/WebCore/page/ContextMenuController.cpp:109
> +    frame->eventHandler()->suspendCaretBlinking(false);

This function name seems too general to me. I would name to respondToContextMenuDismiss or something like that to avoid tying it to caret blinking.

> Source/WebCore/page/ContextMenuController.cpp:173
> +    Frame* frame = m_hitTestResult.innerNonSharedNode()->document()->frame();
> +    // Suspend the caret blinking to match native application behavior.
> +    frame->eventHandler()->suspendCaretBlinking(true);

This is not right. We've already suspended caret blinking on keydown. We shouldn't be suspending caret blinking here again.

> Source/WebCore/page/EventHandler.cpp:718
> +void EventHandler::suspendCaretBlinking(bool shouldSuspend)

This function must be renamed to reflect the fact it's called when context menu is closed. It also shouldn't take a bool.

[chandra shekar vallala]

(In reply to comment #11)
> (From update of attachment 111226 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111226&action=review
> 
> > Source/WebCore/page/ContextMenuController.cpp:98
> > +void ContextMenuController::contextMenuHidden()
> 
> "hidden" doesn't sound right to me. It's not like context menu is temporarily hidden from the screen. Dismissed or closed will be better.

I agree.

> > Source/WebCore/page/ContextMenuController.cpp:108
> > +    // Resume the caret blinking which suspended in context menu show.
> 
> This comment repeats the code.
> 
> > Source/WebCore/page/ContextMenuController.cpp:109
> > +    frame->eventHandler()->suspendCaretBlinking(false);
> 
> This function name seems too general to me. I would name to respondToContextMenuDismiss or something like that to avoid tying it to 
This function can be general to accept true/false.
caret blinking.
> 
> > Source/WebCore/page/ContextMenuController.cpp:173
> > +    Frame* frame = m_hitTestResult.innerNonSharedNode()->document()->frame();
> > +    // Suspend the caret blinking to match native application behavior.
> > +    frame->eventHandler()->suspendCaretBlinking(true);
> 
> This is not right. We've already suspended caret blinking on keydown. We shouldn't be suspending caret blinking here again.

In chromium win port, context menu created in mouse up where we resumes the caret blink. As a result of this caret blinks though context menu shown. This behavior differs with IE and native application in windows. To fix this its better to suspends the caret blink in showContextMenu.

> 
> > Source/WebCore/page/EventHandler.cpp:718
> > +void EventHandler::suspendCaretBlinking(bool shouldSuspend)
> 
> This function must be renamed to reflect the fact it's called when context menu is closed. It also shouldn't take a bool.

This function can call with either true or false as I explained it above to fix the issue in chromium win port. If we don't want to alter the behavior in chromium win port, this can be done.

[Ryosuke Niwa]

(In reply to comment #12)
> > This function name seems too general to me. I would name to respondToContextMenuDismiss or something like that to avoid tying it to 
> This function can be general to accept true/false.
> caret blinking.

I don't think that's a good idea. I don't want WebKit code start suspend/resume caret blinking at random timing. We should keep encapsulate the timing inside WebCore.

> > This is not right. We've already suspended caret blinking on keydown. We shouldn't be suspending caret blinking here again.
> 
> In chromium win port, context menu created in mouse up where we resumes the caret blink. As a result of this caret blinks though context menu shown. This behavior differs with IE and native application in windows. To fix this its better to suspends the caret blink in showContextMenu.

Are you saying that the context menu is created on mouse up? Are you saying that Chromium Win's current behavior matches native Windows apps' behaviors?

Then where is such a logic implemented? I'd think that we should move this logic to EventHandler and make it depend on editing behavior.

> > This function must be renamed to reflect the fact it's called when context menu is closed. It also shouldn't take a bool.
> 
> This function can call with either true or false as I explained it above to fix the issue in chromium win port. If we don't want to alter the behavior in chromium win port, this can be done.

I repeat myself that I don't want to provide such a generic method to WebKit layer.

[WebKit Review Bot]

Attachment 111226 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/page/ContextMenuController.cpp:104:  Declaration has space between type name and * in Frame *frame  [whitespace/declaration] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chandra shekar vallala]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 111226 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=111226&action=review
> > 
> > > Source/WebCore/page/ContextMenuController.cpp:98
> > > +void ContextMenuController::contextMenuHidden()
> > 
> > "hidden" doesn't sound right to me. It's not like context menu is temporarily hidden from the screen. Dismissed or closed will be better.
> 
> I agree.
> 
> > > Source/WebCore/page/ContextMenuController.cpp:108
> > > +    // Resume the caret blinking which suspended in context menu show.
> > 
> > This comment repeats the code.
> > 
> > > Source/WebCore/page/ContextMenuController.cpp:109
> > > +    frame->eventHandler()->suspendCaretBlinking(false);
> > 
> > This function name seems too general to me. I would name to respondToContextMenuDismiss or something like that to avoid tying it to 
> This function can be general to accept true/false.
> caret blinking.
Oops wrongly added. Please ignore the above statement.
> > 
> > > Source/WebCore/page/ContextMenuController.cpp:173
> > > +    Frame* frame = m_hitTestResult.innerNonSharedNode()->document()->frame();
> > > +    // Suspend the caret blinking to match native application behavior.
> > > +    frame->eventHandler()->suspendCaretBlinking(true);
> > 
> > This is not right. We've already suspended caret blinking on keydown. We shouldn't be suspending caret blinking here again.
> 
> In chromium win port, context menu created in mouse up where we resumes the caret blink. As a result of this caret blinks though context menu shown. This behavior differs with IE and native application in windows. To fix this its better to suspends the caret blink in showContextMenu.
> 
> > 
> > > Source/WebCore/page/EventHandler.cpp:718
> > > +void EventHandler::suspendCaretBlinking(bool shouldSuspend)
> > 
> > This function must be renamed to reflect the fact it's called when context menu is closed. It also shouldn't take a bool.
> 
> This function can call with either true or false as I explained it above to fix the issue in chromium win port. If we don't want to alter the behavior in chromium win port, this can be done.

[chandra shekar vallala]

(In reply to comment #13)
> (In reply to comment #12)
> > > This function name seems too general to me. I would name to respondToContextMenuDismiss or something like that to avoid tying it to 
> > This function can be general to accept true/false.
> > caret blinking.
> 
> I don't think that's a good idea. I don't want WebKit code start suspend/resume caret blinking at random timing. We should keep encapsulate the timing inside WebCore.
> 
> > > This is not right. We've already suspended caret blinking on keydown. We shouldn't be suspending caret blinking here again.
> > 
> > In chromium win port, context menu created in mouse up where we resumes the caret blink. As a result of this caret blinks though context menu shown. This behavior differs with IE and native application in windows. To fix this its better to suspends the caret blink in showContextMenu.
> 
> Are you saying that the context menu is created on mouse up? Are you saying that Chromium Win's current behavior matches native Windows apps' behaviors?

Yes. Context menu event handled in mouse up for chromium-win. The logic is at Source/WebKit/chromium/src/WebViewImpl.cpp:577. The current behavior[caret blinks after context menu open in chrome-win. ] doesn't match with native win's app. 

> 
> Then where is such a logic implemented? I'd think that we should move this logic to EventHandler and make it depend on editing behavior.
> 
> > > This function must be renamed to reflect the fact it's called when context menu is closed. It also shouldn't take a bool.
> > 
> > This function can call with either true or false as I explained it above to fix the issue in chromium win port. If we don't want to alter the behavior in chromium win port, this can be done.
> 
> I repeat myself that I don't want to provide such a generic method to WebKit layer.

[Antonio Gomes]

> 
> > Are you saying that the context menu is created on mouse up? Are you saying that Chromium Win's current behavior matches native Windows apps' behaviors?
> 
> Yes. Context menu event handled in mouse up for chromium-win. The logic is at Source/WebKit/chromium/src/WebViewImpl.cpp:577. The current behavior[caret blinks after context menu open in chrome-win. ] doesn't match with native win's app. 
> 
> > 
> > Then where is such a logic implemented? I'd think that we should move this logic to EventHandler and make it depend on editing behavior.

Definitively sounds like a editing behavior.

[Ryosuke Niwa]

(In reply to comment #16)
> (In reply to comment #13)
> > Are you saying that the context menu is created on mouse up? Are you saying that Chromium Win's current behavior matches native Windows apps' behaviors?
> 
> Yes. Context menu event handled in mouse up for chromium-win. The logic is at Source/WebKit/chromium/src/WebViewImpl.cpp:577. The current behavior[caret blinks after context menu open in chrome-win. ] doesn't match with native win's app. 

I see. It sounds like we should refactor code so that the timing at which context menu is shown can be instructed by EventHandler.

Alternatively, you can add new methods to EventHandler that WebKit layer calls when a context menu is shown or hidden. This approach is less desirable since that would mean that each port will still end up implementing their own timing for showing context menu.

Regardless, an important thing here is that you'd have to make sure you make appropriate changes to all ports.

[chandra shekar vallala]

Created attachment 111734 [details]
WebCore changes

This patch has only WebCore changes. Port specific changes will update soon..

[Early Warning System Bot]

Comment on attachment 111734 [details]
WebCore changes

Attachment 111734 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10179356

[WebKit Review Bot]

Comment on attachment 111734 [details]
WebCore changes

Attachment 111734 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10182328

[Ryosuke Niwa]

Comment on attachment 111734 [details]
WebCore changes

View in context: https://bugs.webkit.org/attachment.cgi?id=111734&action=review

> Source/WebCore/page/EventHandler.cpp:2262
> +void respondToContextMenuShown()

We probably need to suspend caret blinking before showing context menu, so I'd rename this function to willShowContextMenu.

Also, you're missing EventHandler::.

> Source/WebCore/page/EventHandler.cpp:2267
> +void respondToContextMenuDismissed()

Nit: s/respondToContextMenuDismissed/respondToContextMenuDismissal/.

Ditto about missing EventHandler:: r- because of this.

[Ryosuke Niwa]

By the way since your patch doesn't actually fix this bug, you should file a new bug and make this bug depend it then upload your patch on that new bug.

[chandra shekar vallala]

(In reply to comment #23)
> By the way since your patch doesn't actually fix this bug, you should file a new bug and make this bug depend it then upload your patch on that new(In reply to comment #22)
> (From update of attachment 111734 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111734&action=review
> 
> > Source/WebCore/page/EventHandler.cpp:2262
> > +void respondToContextMenuShown()
> 
> We probably need to suspend caret blinking before showing context menu, so I'd rename this function to willShowContextMenu.

We are not sure of showing the context menu on every right click since there can be default prevent for context. If we call willShowContextMenu before context menu show from webkit layer there might be no context menu close notify from browser. The issue will exist again ....

> 
> Also, you're missing EventHandler::.
> 
> > Source/WebCore/page/EventHandler.cpp:2267
> > +void respondToContextMenuDismissed()
> 
> Nit: s/respondToContextMenuDismissed/respondToContextMenuDismissal/.
> 
> Ditto about missing EventHandler:: r- because of this.

Sorry, I uploaded a wrong patch...

[Ryosuke Niwa]

(In reply to comment #24)
> We are not sure of showing the context menu on every right click since there can be default prevent for context. If we call willShowContextMenu before context menu show from webkit layer there might be no context menu close notify from browser. The issue will exist again

That's the whole point of this function, right? WebKit layer or whatever callers of this function ensures that context menu is about to be shown. We shouldn't be calling this function if the context menu can be prevented to be shown.

[chandra shekar vallala]

Created attachment 111750 [details]
updated patch

Update the WebCore patch.

[Ryosuke Niwa]

Comment on attachment 111750 [details]
updated patch

Please post this patch on the bug 70491. Also your patch needs to modify each port's code to call these functions. Furthermore, you should be removing the code to suspend caret blinking on mousedown in EventHandler.

[chandra shekar vallala]

Created attachment 111962 [details]
patch for chromium port changes.

Uploaded the patch for chromium port.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 111962 [details]
patch for chromium port changes.

View in context: https://bugs.webkit.org/attachment.cgi?id=111962&action=review

> Source/WebKit/chromium/public/WebView.h:328
> +    virtual void contextMenuClosed() { }

nit: please use a name like didCloseContextMenu to be more consistent with existing notification-style methods.

[Ryosuke Niwa]

As I've repeatedly stated, please upload your patch to add WebKit/WebCore notification API to the bug 70491. This bug should only receive patches that actually fixes the bug you stated.