Bug 38564 – Caret keeps blinking during forward-delete

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

Steps to reproduce:
1. Type something in one of the text fields in this page
2. Move the caret to the beginning of the field
3. Hold down the (forward) delete key

The caret does not stop blinking as it should. Compare with (backwards) delete or ordinary typing.

[Van Lam]

Created attachment 105823 [details]
Proposed fix

[Darin Adler]

Comment on attachment 105823 [details]
Proposed fix

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

r=me, but I think this could be slightly improved coding-style-wise

I also would like to see us come up with a way to test.

> Source/WebCore/editing/FrameSelection.cpp:1604
> +    bool shouldStopBlinkingDueToTypingCommand = (m_frame && (lastEditCommand = m_frame->editor()->lastEditCommand())) ?
> +        lastEditCommand->shouldStopCaretBlinking() : false;

The last part of this should also be written using && rather than ? :

Also, this is hard to read with the assignment in the middle of it. I think it would be clearer like this:

    EditCommand* lastEditCommand = m_frame ? m_frame->editor()->lastEditCommand() : 0;
    bool shouldStopBlinkingDueToTypingCommand = lastEditCommand && lastEditCommand->shouldStopCaretBlinking();

> Source/WebCore/editing/TypingCommand.h:73
> +    bool shouldStopCaretBlinking() const { return true; }

This override could, and should, be private rather than public. It would be a programming mistake to call this on a TypingCommand*.

[Darin Adler]

Comment on attachment 105823 [details]
Proposed fix

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

>> Source/WebCore/editing/TypingCommand.h:73
>> +    bool shouldStopCaretBlinking() const { return true; }
> 
> This override could, and should, be private rather than public. It would be a programming mistake to call this on a TypingCommand*.

Also, WebKit coding style is to always explicitly say virtual on a virtual function override like this one.

[Van Lam]

Created attachment 105880 [details]
Updated patch

Thanks for the review, Darin. After thinking about how to test this change, I'm unsure on how the blinking caret is exposed such that we can test it. Any ideas anyone?

[Eric Seidel (no email)]

Comment on attachment 105823 [details]
Proposed fix

Cleared Darin Adler's review+ from obsolete attachment 105823 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Ryosuke Niwa]

(In reply to comment #4)
> Thanks for the review, Darin. After thinking about how to test this change, I'm unsure on how the blinking caret is exposed such that we can test it. Any ideas anyone?

Unfortunately, there isn't a way to test this. But the patch looks seems valuable as is so let's just land this.

[WebKit Review Bot]

Comment on attachment 105880 [details]
Updated patch

Clearing flags on attachment: 105880

Committed r102413: <http://trac.webkit.org/changeset/102413>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

A manual test would be possible; automated tests are not the only valuable ones. Please, someone, make a manual test for the manual tests directory.