Use a block cursor in overtype mode

Use a block cursor in overtype mode

Overtype mode will use block cursor instead of a caret to
highlight the next character to be replaced. It will fully cover
the next character to be replaced (except at the end of a line
where the usual blinking caret will be shown).

This new block cursor is internally implemented as a selection
(not exposed to JavaScript) because the selection code knows how
to deal with bidi text.

R=
BUG=237584
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=152662

[svillar, 2013-05-02 19:20:21]

The patch landed in WK http://trac.webkit.org/changeset/149432

[dglazkov, 2013-05-06 16:35:45]

The change looks mechanically good, but I am not the right person to look at
this. I added some peeps who might be good reviewers for editing code.

[esprehn, 2013-05-06 16:42:10]

https://codereview.chromium.org/14859008/diff/1/Source/core/editing/FrameSele...
File Source/core/editing/FrameSelection.cpp (right):

https://codereview.chromium.org/14859008/diff/1/Source/core/editing/FrameSele...
Source/core/editing/FrameSelection.cpp:1743: // the FrameSelection will paint a
blinking caret as usual).
Why doesn't the block blink? I seem to recall editors supporting both modes, I
take it that not blinking in overtype is the most common?

[svillar, 2013-05-06 16:59:43]

On 2013/05/06 16:42:10, esprehn wrote:
>
https://codereview.chromium.org/14859008/diff/1/Source/core/editing/FrameSele...
> File Source/core/editing/FrameSelection.cpp (right):
> 
>
https://codereview.chromium.org/14859008/diff/1/Source/core/editing/FrameSele...
> Source/core/editing/FrameSelection.cpp:1743: // the FrameSelection will paint
a
> blinking caret as usual).
> Why doesn't the block blink? I seem to recall editors supporting both modes, I
> take it that not blinking in overtype is the most common?

I don't really have an answer for this last question.

This is an initial patch to add the block cursor to the overtype mode, so I just
didn't add blinking in order not to complicate the patch too much as by doing
so, it will likely require some extra refactorings to share code with the caret
painting code (note that the block cursor is painted as a selection).

In any case, I think blinking could be achieved with some JS if required by
using ::selection{} along with a timer.

[leviw_travelin_and_unemployed, 2013-05-06 18:40:37]

I agree that the code appears functionally sound, but I'm unsure of how
comfortable I am with the actual behavior. I'd like to actually build and try
this out before making up my mind.

[ojan, 2013-05-07 01:21:19]

How do you turn on overwrite mode in Chrome?

[svillar, 2013-05-07 07:41:02]

On 2013/05/07 01:21:19, ojan wrote:
> How do you turn on overwrite mode in Chrome?

When I discussed this feature with rniwa we agreed that the command to toggle
overtype mode one would be only available through a keybinding or a context menu
option. I've only enabled it in the GTK port for the moment (see r147916).

I Chrome is interested in enabling the feature I can provide a patch to enable
it with a keybinding.

[svillar, 2013-05-13 11:37:36]

On 2013/05/07 07:41:02, svillar wrote:
> On 2013/05/07 01:21:19, ojan wrote:
> > How do you turn on overwrite mode in Chrome?
> 
> When I discussed this feature with rniwa we agreed that the command to toggle
> overtype mode one would be only available through a keybinding or a context
menu
> option. I've only enabled it in the GTK port for the moment (see r147916).
> 
> I Chrome is interested in enabling the feature I can provide a patch to enable
> it with a keybinding.

Added the insert keybinding in issue 14842018

[svillar, 2013-05-20 17:08:47]

ping reviewers. As mentioned above, the patch for
https://codereview.chromium.org/14842018/ enables overtype mode in chromium so
you could actually test this change.

[svillar, 2013-05-22 17:06:57]

On 2013/05/20 17:08:47, svillar wrote:
> ping reviewers. As mentioned above, the patch for
> https://codereview.chromium.org/14842018/ enables overtype mode in chromium so
> you could actually test this change.

So the keybinding patch has finally landed, you could try the overtype mode now
just by hitting the "Insert key" in any richly editable web content.

[esprehn, 2013-06-12 19:25:30]

https://codereview.chromium.org/14859008/diff/1/LayoutTests/editing/selection...
File LayoutTests/editing/selection/block-cursor-overtype-mode-expected.txt
(right):

https://codereview.chromium.org/14859008/diff/1/LayoutTests/editing/selection...
LayoutTests/editing/selection/block-cursor-overtype-mode-expected.txt:10:
getSelection().collapse(textNode, 0)
It'd be nicer if these tests were broken into separate files. Like
overtype-rtl.html and overtype.html, one giant test is really hard to debug.

https://codereview.chromium.org/14859008/diff/1/Source/core/editing/Editor.cpp
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/14859008/diff/1/Source/core/editing/Editor.cp...
Source/core/editing/Editor.cpp:2918: m_overwriteModeEnabled =
!m_overwriteModeEnabled;
I'd prefer we didn't store the overwrite mode state in two places. Can you
eliminate the boolean in Editor? The isOvertypeMode() method can just return
selection()->isBlockCursor() or something.

https://codereview.chromium.org/14859008/diff/1/Source/core/testing/Internals...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/14859008/diff/1/Source/core/testing/Internals...
Source/core/testing/Internals.cpp:1974: return ClientRect::create();
return 0, the return value doesn't matter anyway, this is going to throw.

[svillar, 2013-06-13 08:53:06]

https://codereview.chromium.org/14859008/diff/1/Source/core/editing/Editor.cpp
File Source/core/editing/Editor.cpp (right):

https://codereview.chromium.org/14859008/diff/1/Source/core/editing/Editor.cp...
Source/core/editing/Editor.cpp:2918: m_overwriteModeEnabled =
!m_overwriteModeEnabled;
On 2013/06/12 19:25:30, esprehn wrote:
> I'd prefer we didn't store the overwrite mode state in two places. Can you
> eliminate the boolean in Editor? The isOvertypeMode() method can just return
> selection()->isBlockCursor() or something.

So there is a reason for having these two states which are actually different
things. The boolean in the editor is the one used to determine how the editing
code will behave, inserting new characters or overwriting the existing ones.

We cannot directly use it in the selection to decide whether or not to paint a
block cursor because sometimes you don't want to do it even if we're in overtype
mode. This happens for example when the cursor is at the end of the line, in
that case we want to paint a caret even in overtype mode.

So the boolean in the editor will be true, instructing the code to work in
overtype mode, but the boolean in the selection will be false, and thus a caret
(and not a block cursor) will be painted.

https://codereview.chromium.org/14859008/diff/1/Source/core/testing/Internals...
File Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/14859008/diff/1/Source/core/testing/Internals...
Source/core/testing/Internals.cpp:1974: return ClientRect::create();
On 2013/06/12 19:25:30, esprehn wrote:
> return 0, the return value doesn't matter anyway, this is going to throw.

Good point

[esprehn, 2013-06-18 18:36:03]

LGTM.

[commit-bot: I haz the power, 2013-06-18 18:36:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/svillar@igalia.com/14859008/17001

[commit-bot: I haz the power, 2013-06-18 20:20:43]

Change committed as 152662