Caret stops blinking after context menu shown

• Component: HTML Editing
• Status: RESOLVED FIXED

[Jonathan Hammer]

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.

[Ryosuke Niwa]

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.

[Jonathan Hammer]

(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)?

[Ryosuke Niwa]

Oh, you probably have to add some method to internals to expose whether FrameSelection::m_caretBlinkTimer is active or not.

[Jonathan Hammer]

(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!

[Jonathan Hammer]

Created attachment 355081 [details]
Patch

Proposed patch.

[Ryosuke Niwa]

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.

[Jonathan Hammer]

Created attachment 355100 [details]
Patch

Revised patch attached

[Ryosuke Niwa]

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.

[WebKit Commit Bot]

Comment on attachment 355100 [details]
Patch

Clearing flags on attachment: 355100

Committed r238345: <https://trac.webkit.org/changeset/238345>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46149260>