Bug 38564 - Caret keeps blinking during forward-delete
Summary: Caret keeps blinking during forward-delete
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-04 22:36 PDT by mitz
Modified: 2011-12-09 11:36 PST (History)
5 users (show)

See Also:


Attachments
Proposed fix (3.70 KB, patch)
2011-08-31 13:42 PDT, Van Lam
no flags Details | Formatted Diff | Diff
Updated patch (3.90 KB, patch)
2011-08-31 18:58 PDT, Van Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2010-05-04 22:36:39 PDT
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.
Comment 1 Van Lam 2011-08-31 13:42:21 PDT
Created attachment 105823 [details]
Proposed fix
Comment 2 Darin Adler 2011-08-31 16:51:30 PDT
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*.
Comment 3 Darin Adler 2011-08-31 16:51:55 PDT
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.
Comment 4 Van Lam 2011-08-31 18:58:16 PDT
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?
Comment 5 Eric Seidel (no email) 2011-09-06 15:28:30 PDT
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.
Comment 6 Ryosuke Niwa 2011-12-08 17:35:21 PST
(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.
Comment 7 WebKit Review Bot 2011-12-08 18:28:42 PST
Comment on attachment 105880 [details]
Updated patch

Clearing flags on attachment: 105880

Committed r102413: <http://trac.webkit.org/changeset/102413>
Comment 8 WebKit Review Bot 2011-12-08 18:28:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Darin Adler 2011-12-09 11:36:44 PST
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.