Bug 114819 – Show a block cursor in overtype mode

• Component: HTML Editing
• Status: RESOLVED FIXED

[Sergio Villar Senin]

I've been taking a look at how to properly implement this. It's not enough with adjusting the caret size and position to cover the character located in the next offset because it does not get on well with bidi text. So rniwa suggested on the mailing list (see https://lists.webkit.org/pipermail/webkit-dev/2013-April/024650.html) to internally use a selection as it already deals with all the bidi text complexities.

Having an internal 1-char long selection is not a problem. The thing is how to properly show it to the user. We have two options I'd say:
1) modify the caret to cover the same area than the internal selection.
2) show the selection instead of the caret

Regarding 1) it's really complicated because the actual size and position of the selection is computed at rendering time if I'm not wrong (the code iterates over the RenderObjects covered by the selection to build the final LayoutRect), so it is not possible to get their position and size to be used to modify the caret.

I've explored option 2) as well. I think we can show the selection without exposing it to the JS world by setting the Frame into the internal FrameSelection object. This way the selection will be shown to the user but the one exposed to JS will remain the one linked to the Frame object.

Opinions, suggestions, corrections?

[Ryosuke Niwa]

We already have an abstraction around FrameSelection when we expose to JavaScript (DOMSelection? can't remember the name) so I expect the second approach to work better.

[Shezan Baig]

I'm curious how the internal structure will look like if we have block-cursor *and* user-selected text at the same time.  Will there be two FrameSelection objects attached to the same Frame, or will FrameSelection be modified to handle this situation?

[Ryosuke Niwa]

(In reply to comment #2)
> I'm curious how the internal structure will look like if we have block-cursor *and* user-selected text at the same time.  Will there be two FrameSelection objects attached to the same Frame, or will FrameSelection be modified to handle this situation?

You can't because collapsed and non-collapsed selection (caret) cannot co-exist.

[Ryosuke Niwa]

collapsed (caret)* and non-collapsed selection

[Shezan Baig]

(In reply to comment #3)
> You can't because collapsed and non-collapsed selection (caret) cannot co-exist.


This might present a UX problem for us.  One of our requirements is the ability to change the color of the block cursor, so it will have a different color than the selection.  This is used to indicate which side of the selection the caret extent is at, so that the user knows exactly how the selection will be modified when using Ctrl+Left/Right.

[Ryosuke Niwa]

We don't support that. I don't think we want to add such a feature to WebKit unless it's a platform convention somewhere.  It'll add significant complexity to our codebase to get that right.

[Sergio Villar Senin]

Created attachment 199026 [details]
Patch

[Shezan Baig]

Can we make block-cursor independent of the overtype mode setting?

[Build Bot]

Comment on attachment 199026 [details]
Patch

Attachment 199026 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/141450

[Sergio Villar Senin]

(In reply to comment #8)
> Can we make block-cursor independent of the overtype mode setting?

What do you mean, to have it always enabled? I guess it could make sense for specific projects but not for upstream (all the browsers/editors show the 1-pixel caret in insert mode).

[Build Bot]

Comment on attachment 199026 [details]
Patch

Attachment 199026 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/199181

[Sergio Villar Senin]

(In reply to comment #11)
> (From update of attachment 199026 [details])
> Attachment 199026 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/199181

I'll need some help from windows guys. Not sure where they store the list of exported symbols.

[Build Bot]

Comment on attachment 199026 [details]
Patch

Attachment 199026 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/59585

[Sergio Villar Senin]

Build errors are due to the fact that I am not 100% sure how to export symbols for the Mac/Win platform, that could be addressed later and it isn't incompatible with a preliminary review.

[Ryosuke Niwa]

Comment on attachment 199026 [details]
Patch

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

> Source/WebCore/editing/Editor.cpp:3132
> +        m_overwriteModeSelection = adoptPtr(new FrameSelection(frame(), true));

I'm not keen on the fact we're going to create another FrameSelection.
Can't we add a single Position to FrameSelection and change updateAppearance to paint the selection depending on the mode?

> Source/WebCore/editing/FrameSelection.cpp:108
> +FrameSelection::FrameSelection(Frame* frame, bool isOverwriteModeSelection)

Please use enum instead.

> Source/WebCore/editing/FrameSelection.cpp:345
> +        m_frame->editor()->updateOverwriteModeSelection();

This is going to fire a second set of selectionchange event, etc... r- because of this.

> Source/WebCore/editing/FrameSelection.cpp:1775
> +    // In overtype mode, the internal FrameSelection will paint the selection pretending to be a caret.
> +    if (m_frame->editor()->isOverwriteModeEnabled() != m_isOverwriteModeSelection)
> +        return;
> +

Oh no :( circular dependencies.

[Sergio Villar Senin]

(In reply to comment #15)
> (From update of attachment 199026 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199026&action=review
> 
> > Source/WebCore/editing/Editor.cpp:3132
> > +        m_overwriteModeSelection = adoptPtr(new FrameSelection(frame(), true));
> 
> I'm not keen on the fact we're going to create another FrameSelection.
> Can't we add a single Position to FrameSelection and change updateAppearance to paint the selection depending on the mode?

I was not completely happy either, but you comments inspired me :) another approach which will fix the issues commented in your review.

[Sergio Villar Senin]

Created attachment 199652 [details]
Patch

[Build Bot]

Comment on attachment 199652 [details]
Patch

Attachment 199652 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/224084

[Build Bot]

Comment on attachment 199652 [details]
Patch

Attachment 199652 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/56773

[Sergio Villar Senin]

Created attachment 199653 [details]
Patch

Attempt to fix Mac's build failures

[Build Bot]

Comment on attachment 199653 [details]
Patch

Attachment 199653 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/139540

[Sergio Villar Senin]

Created attachment 199666 [details]
Patch

Make the Win EWS happy

[Sergio Villar Senin]

pinging reviewers now that we have the OK from the EWSs

[Ryosuke Niwa]

Comment on attachment 199666 [details]
Patch

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

New approach is much saner than the original patch but it still introduces undesirable circular dependencies and makes updateAppearance update the caret visibility, which we don't want.

> Source/WebCore/ChangeLog:10
> +        The 1-pixel long caret will be replaced in overtype mode by a

Caret & selection is the same thing. Caret = collapsed selection.
We need to revise this change log entry.

> Source/WebCore/editing/Editor.cpp:3130
> +    frame()->selection()->setCaretVisible(!m_overwriteModeEnabled);

This seems wrong. We might want to update the appearance but we shouldn't be updating the caret visibility like this.
FrameSelection talking back to editor to get isOverwriteModeEnabled is also bad.
We probably want to add another member variable to FrameSelection saying that we want to show a block caret whenever possible.

> Source/WebCore/editing/FrameSelection.cpp:1765
> +    bool shouldFakeCaretVisibility = m_frame->editor()->isOverwriteModeEnabled() && m_selection.isCaret();
> +    if (shouldFakeCaretVisibility) {

I don't like this variable name. "shouldFakeCaretVisibility" doesn't tell us why it's fake or in what sense it is fake.
It's also strange that this is the special case instead of a block caret inside a line being a special case.

> Source/WebCore/editing/FrameSelection.cpp:1767
> +        shouldFakeCaretVisibility = forwardPosition.isNotNull();

And this flag is even updated here!

> Source/WebCore/editing/FrameSelection.cpp:1768
> +        setCaretVisibility(shouldFakeCaretVisibility ? Hidden : Visible);

updateAppearance shouldn't be updating caret visibility. You do realize that setCaretVisibility calls updateAppearance, right?
r- because of this.

[Sergio Villar Senin]

(In reply to comment #24)
> (From update of attachment 199666 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199666&action=review

> FrameSelection talking back to editor to get isOverwriteModeEnabled is also bad.
> We probably want to add another member variable to FrameSelection saying that we want to show a block caret whenever possible.

Will do that.
 
> > Source/WebCore/editing/FrameSelection.cpp:1765
> > +    bool shouldFakeCaretVisibility = m_frame->editor()->isOverwriteModeEnabled() && m_selection.isCaret();
> > +    if (shouldFakeCaretVisibility) {
> 
> I don't like this variable name. "shouldFakeCaretVisibility" doesn't tell us why it's fake or in what sense it is fake.
> It's also strange that this is the special case instead of a block caret inside a line being a special case.
> > Source/WebCore/editing/FrameSelection.cpp:1767
> > +        shouldFakeCaretVisibility = forwardPosition.isNotNull();
> 
> And this flag is even updated here!

I don't exactly get what you mean in your sentence about the special case but anyway, as you might have noticed, this whole if block basically checks that we can extend the selection forward. That's done in order not to hide the caret if we're at the end of a line and we do not need to do that if not in overwrite mode.

> > Source/WebCore/editing/FrameSelection.cpp:1768
> > +        setCaretVisibility(shouldFakeCaretVisibility ? Hidden : Visible);
> 
> updateAppearance shouldn't be updating caret visibility. You do realize that setCaretVisibility calls updateAppearance, right?
> r- because of this.

True, terrible mistake I'll change that, weird that it didn't cause a stack overflow, some code should be breaking the loop.

[Sergio Villar Senin]

Created attachment 200115 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 200115 [details]
Patch

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

> Source/WebCore/editing/FrameSelection.cpp:1761
> +        m_caretPaint = forwardPosition.isNull();

I don't think we should update this value. m_caretPaint is only useful inside m_caretBlinkTimer's fired but we'll never start this timer below if the block caret is shown.

> Source/WebCore/editing/FrameSelection.cpp:2076
> +#if ENABLE(TEXT_CARET)
> +    m_frame->document()->updateLayoutIgnorePendingStylesheets();
> +#else
> +    m_frame->document()->updateStyleIfNeeded();
> +#endif

Why? We can always call updateLayoutIgnorePendingStylesheets since nobody is going to call this function when the text caret is disabled.

> LayoutTests/editing/selection/caret-overtype-mode-expected.txt:8
> +ABC
> +ABC
> +ש××ABC
> +ש××ABC
> +ש××
> +ש××

Why are we not seeing any PASSes?

> LayoutTests/editing/selection/caret-overtype-mode.html:36
> +        shouldBeTrueQuiet(getSelection().isCollapsed.toString());

Why quiet?

> LayoutTests/editing/selection/caret-overtype-mode.html:43
> +            shouldBeTrueQuiet(leftPositionValues[i].toString() + expected[i] + leftPositionValues[i+1].toString());
> +        shouldBeTrueQuiet(widthValues[i].toString() + " > 1");

Why are we using Quiet? Why aren't we printing out results?

> LayoutTests/editing/selection/caret-overtype-mode.html:46
> +    // At the end of the line we must have a 1-pixel long classical caret

Include this inside debug() so that we can see it in the expected result.

> LayoutTests/editing/selection/caret-overtype-mode.html:47
> +    getSelection().collapse(element, element.length);

Better yet, include this inside evalAndLog so that we can see it in the expected result.
If we've done that, we don't need the comment above at all.

> LayoutTests/editing/selection/caret-overtype-mode.html:49
> +    shouldBe(internals.selectionBounds().width.toString(), "0", true);
> +    shouldBe(internals.absoluteCaretBounds().width.toString(), "1", true);

You should do 
shouldBe("internals.selectionBounds().width", "0", true);
shouldBe("internals.absoluteCaretBounds().width", "1", true);
instead.

[Sergio Villar Senin]

(In reply to comment #27)
> (From update of attachment 200115 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200115&action=review
> 
> > Source/WebCore/editing/FrameSelection.cpp:1761
> > +        m_caretPaint = forwardPosition.isNull();
> 
> I don't think we should update this value. m_caretPaint is only useful inside m_caretBlinkTimer's fired but we'll never start this timer below if the block caret is shown.

AFAIK that's not totally correct, the last painting phase in the RenderBlock invokes the FrameSelection::paintCaret() method. If you don't like the current approach I guess that another possibility could be to store the forwardPosition (updating it likely in setSelection() when in overtype mode) and add the check both in updateAppearance and paintCaret.

> > Source/WebCore/editing/FrameSelection.cpp:2076
> > +#if ENABLE(TEXT_CARET)
> > +    m_frame->document()->updateLayoutIgnorePendingStylesheets();
> > +#else
> > +    m_frame->document()->updateStyleIfNeeded();
> > +#endif
> 
> Why? We can always call updateLayoutIgnorePendingStylesheets since nobody is going to call this function when the text caret is disabled.

Well, in theory nothing prevents a JS method to call toggleOverwriteMode in a rich editable content right?

> > LayoutTests/editing/selection/caret-overtype-mode-expected.txt:8
> > +ABC
> > +ABC
> > +ש××ABC
> > +ש××ABC
> > +ש××
> > +ש××
> 
> Why are we not seeing any PASSes?

Replying here to this and the following questions.

I just used the quiet versions of shouldBeXXX functions because I wanted a test case common for all the platforms. Font rendering would be different for each platform and so the sizes of the fonts. Without the quiet version we would have to provide specific results for each platform.

Note that IMHO the test should focus on verifying that we are using the block cursor, not sure that the specific widths of each letter are that important. 

> > LayoutTests/editing/selection/caret-overtype-mode.html:49
> > +    shouldBe(internals.selectionBounds().width.toString(), "0", true);
> > +    shouldBe(internals.absoluteCaretBounds().width.toString(), "1", true);
> 
> You should do 
> shouldBe("internals.selectionBounds().width", "0", true);
> shouldBe("internals.absoluteCaretBounds().width", "1", true);
> instead.

OK

[Ryosuke Niwa]

Comment on attachment 200115 [details]
Patch

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

>>> Source/WebCore/editing/FrameSelection.cpp:1761
>>> +        m_caretPaint = forwardPosition.isNull();
>> 
>> I don't think we should update this value. m_caretPaint is only useful inside m_caretBlinkTimer's fired but we'll never start this timer below if the block caret is shown.
> 
> AFAIK that's not totally correct, the last painting phase in the RenderBlock invokes the FrameSelection::paintCaret() method. If you don't like the current approach I guess that another possibility could be to store the forwardPosition (updating it likely in setSelection() when in overtype mode) and add the check both in updateAppearance and paintCaret.

Ugh… dependencies :(  Okay, this looks reasonable then.

>>> Source/WebCore/editing/FrameSelection.cpp:2076
>>> +#endif
>> 
>> Why? We can always call updateLayoutIgnorePendingStylesheets since nobody is going to call this function when the text caret is disabled.
> 
> Well, in theory nothing prevents a JS method to call toggleOverwriteMode in a rich editable content right?

We’re not exposing it to JavaScript except internals, and we don’t care about the performance there
so it should be always safe to call updateLayoutIgnorePendingStylesheets.

[Sergio Villar Senin]

Created attachment 200134 [details]
Patch (second approach)

So this is the other approach I mentioned to you as an alternative to modifying m_caretPaint. I personally prefer the other one because I'm generally not in favor of adding more stuff to an object state due to the burden of having to maintain it.

Using m_caretPaint to prevent the caret painting (as in the other patch) looks good to me because it seems the attribute is meant to avoid caret painting (not sure if it was originally designed only for the caret blinking) but I guess it should be safe enough.

Anyway posting this here just to show you another approach, I'll let the expert decide :-).

[Sergio Villar Senin]

Comment on attachment 200134 [details]
Patch (second approach)

Setting it as obsolete as agreed on IRC, the other approach is the preferred one.

[Sergio Villar Senin]

Created attachment 200207 [details]
Patch

Added a missing initialization. Also replaced every occurrence of "block caret" by "block cursor".

Apart from that the test results are now far more verbose and self-explanatory.

[Ryosuke Niwa]

Comment on attachment 200207 [details]
Patch

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

> LayoutTests/editing/selection/block-cursor-overtype-mode.html:36
> +    for (var i=0; i < textNode.length; i++) {

Space around =.

> LayoutTests/editing/selection/block-cursor-overtype-mode.html:43
> +            if (expected[i-1] == ">")

Wrong indentation. 4 spaces.

> LayoutTests/editing/selection/block-cursor-overtype-mode.html:45
> +	    else

Tab

> LayoutTests/editing/selection/block-cursor-overtype-mode.html:47
> +	}

Tab

> LayoutTests/editing/selection/block-cursor-overtype-mode.html:50
> +	shouldBeTrue("blockCursor.width > 1");

Tab

[Sergio Villar Senin]

Committed r149432: <http://trac.webkit.org/changeset/149432>