Showing a context menu for a selection of text inside of a contenteditable element stops the caret from blinking even *after* the context menu is dismissed. The only way to restart the blinking after the menu is dismissed is to click somewhere else inside the contenteditable element. If you don’t click and instead use the keyboard to change the selection, the caret will remain unblinking. FrameSelection::setCaretBlinkingSuspended(true) is called from EventHandler::handleMousePressEvent(). The balancing setCaretBlinkingSuspended(false) call is found in EventHandler::handleMouseReleaseEvent. The reason it is broken in the case of context menus is because handleMouseReleaseEvent is never called after the context menu is shown. My proposed fix is to modify EventHandler::sendContextMenuEvent and have it call setCaretBlinkingSuspended(false) before showing the menu. I can create a patch, but wanted to ask first if it is feasible to create a layout regression test to cover this bug/fix? I can’t think of an obvious way to do it.
Yes, you should be able to send eventSender.mouseDown(2) or eventSender.keyDown('menu') to trigger a context menu. It may only work in WebKitLegacy / WebKit1 though... so you might need to run the test with -1.
(In reply to Ryosuke Niwa from comment #1) > Yes, you should be able to send eventSender.mouseDown(2) or > eventSender.keyDown('menu') to trigger a context menu. It may only work in > WebKitLegacy / WebKit1 though... so you might need to run the test with -1. Sorry I should have clarified: my question is how would the test be able to determine if the caret was blinking or not (after the menu is dismissed)?
Oh, you probably have to add some method to internals to expose whether FrameSelection::m_caretBlinkTimer is active or not.
(In reply to Ryosuke Niwa from comment #3) > Oh, you probably have to add some method to internals to expose whether > FrameSelection::m_caretBlinkTimer is active or not. Ah, got it. Didn’t know about internals until just now. I’ll look into it. Thanks!
Created attachment 355081 [details] Patch Proposed patch.
Comment on attachment 355081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355081&action=review Thanks for the fix! The code change looks good but there are a few nits to fix. > Source/WebCore/page/EventHandler.cpp:2883 > + Nit: Whitespace. > LayoutTests/fast/events/contextmenu-dismiss-blink-caret.html:3 > + <div id="contenteditable" contenteditable>This tests whether the caret continues to blink after the context menu is dismissed.</div> Please add an instruction to run this test manually in the browser. This is important so that QA folks, etc... can run this test manually to verify the fix, and also in the future this test ever gets broken, to be able to bisect using a browser.
Created attachment 355100 [details] Patch Revised patch attached
Comment on attachment 355100 [details] Patch Let's land this. In the future, please also set cq? so that someone who is a committer can cq+ the patch to land it.
Comment on attachment 355100 [details] Patch Clearing flags on attachment: 355100 Committed r238345: <https://trac.webkit.org/changeset/238345>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46149260>