Bug 69137

Summary: In input field caret is not blinking after context menu hide
Product: WebKit Reporter: chandra shekar vallala <chandra.vallala>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: abarth, ap, cshu, dglazkov, enrica, fishd, kenneth, rniwa, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 70491    
Bug Blocks:    
Attachments:
Description Flags
patch with webkit changes
none
updated the patch for mac/Linux.
rniwa: review-
updated patch
none
WebCore changes
rniwa: review-, webkit-ews: commit-queue-
updated patch
rniwa: review-
patch for chromium port changes. fishd: review-, fishd: commit-queue-

Description chandra shekar vallala 2011-09-30 01:11:01 PDT
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
Comment 1 chandra shekar vallala 2011-09-30 02:47:03 PDT
Created attachment 109272 [details]
patch with webkit changes
Comment 2 Ryosuke Niwa 2011-09-30 09:58:44 PDT
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?
Comment 3 Ryosuke Niwa 2011-09-30 09:59:50 PDT
To other reviewers, please don't r+ this patch for now
Comment 4 chandra shekar vallala 2011-10-03 06:09:20 PDT
Created attachment 109479 [details]
updated the patch for mac/Linux.
Comment 5 chandra shekar vallala 2011-10-03 06:11:53 PDT
(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.
Comment 6 Ryosuke Niwa 2011-10-03 09:25:02 PDT
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.
Comment 7 chandra shekar vallala 2011-10-13 04:00:20 PDT
(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.
Comment 8 Ryosuke Niwa 2011-10-13 10:53:13 PDT
(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.
Comment 9 chandra shekar vallala 2011-10-17 02:01:46 PDT
(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.
Comment 10 chandra shekar vallala 2011-10-17 02:03:11 PDT
Created attachment 111226 [details]
updated patch
Comment 11 Ryosuke Niwa 2011-10-17 11:23:25 PDT
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.
Comment 12 chandra shekar vallala 2011-10-17 23:07:15 PDT
(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.
Comment 13 Ryosuke Niwa 2011-10-17 23:35:06 PDT
(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.
Comment 14 WebKit Review Bot 2011-10-17 23:35:39 PDT
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.
Comment 15 chandra shekar vallala 2011-10-17 23:36:06 PDT
(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.
Comment 16 chandra shekar vallala 2011-10-18 07:16:18 PDT
(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.
Comment 17 Antonio Gomes 2011-10-18 07:35:00 PDT
> 
> > 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.
Comment 18 Ryosuke Niwa 2011-10-18 09:53:14 PDT
(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.
Comment 19 chandra shekar vallala 2011-10-20 00:33:12 PDT
Created attachment 111734 [details]
WebCore changes

This patch has only WebCore changes. Port specific changes will update soon..
Comment 20 Early Warning System Bot 2011-10-20 00:42:03 PDT
Comment on attachment 111734 [details]
WebCore changes

Attachment 111734 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10179356
Comment 21 WebKit Review Bot 2011-10-20 00:45:23 PDT
Comment on attachment 111734 [details]
WebCore changes

Attachment 111734 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10182328
Comment 22 Ryosuke Niwa 2011-10-20 00:54:05 PDT
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.
Comment 23 Ryosuke Niwa 2011-10-20 00:55:14 PDT
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.
Comment 24 chandra shekar vallala 2011-10-20 02:09:03 PDT
(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...
Comment 25 Ryosuke Niwa 2011-10-20 02:20:15 PDT
(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.
Comment 26 chandra shekar vallala 2011-10-20 04:05:44 PDT
Created attachment 111750 [details]
updated patch

Update the WebCore patch.
Comment 27 Ryosuke Niwa 2011-10-20 10:54:02 PDT
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.
Comment 28 chandra shekar vallala 2011-10-21 07:27:26 PDT
Created attachment 111962 [details]
patch for chromium port changes.

Uploaded the patch for chromium port.
Comment 29 WebKit Review Bot 2011-10-21 07:29:00 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 30 Darin Fisher (:fishd, Google) 2011-10-21 07:37:12 PDT
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.
Comment 31 Ryosuke Niwa 2011-10-21 10:26:58 PDT
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.