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.
Created attachment 105823 [details] Proposed fix
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 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.
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 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.
(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 on attachment 105880 [details] Updated patch Clearing flags on attachment: 105880 Committed r102413: <http://trac.webkit.org/changeset/102413>
All reviewed patches have been landed. Closing bug.
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.