Bug 112126 – Implement overtype mode for editable content

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

[Sergio Villar Senin]

This bug is a follow-up of this discussion in the mailing list https://lists.webkit.org/pipermail/webkit-dev/2013-March/024097.html

[Ryosuke Niwa]

Be sure to test the implementation with bidirectional text and CJK (namely with various input methods).

[Shezan Baig]

I'm assuming this is to implement just the overtype behavior and not the block-cursor appearance, which can be done at a later stage. Is that right, Sergio?

[Sergio Villar Senin]

(In reply to comment #2)
> I'm assuming this is to implement just the overtype behavior and not the block-cursor appearance, which can be done at a later stage. Is that right, Sergio?

Yes you're right. My plan is to first implement the overtype mode. How we visually expose it to the user is a different story and we might even consider it not WebCore's bussiness as one application might want to a show a different block cursor but some other might decide to decorate the boundaries of the text element or whatever.

[Sergio Villar Senin]

Created attachment 193862 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 193862 [details]
Patch

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

> Source/WebCore/editing/EditorCommand.cpp:1560
> +        { "Overwrite", { executeToggleOverwrite, supportedFromMenuOrKeyBinding, enabledInRichlyEditableText, stateNone, valueNull, notTextInsertion, doNotAllowExecutionWhenDisabled } },

MSDN uses OverWrite. Let us match that capitalization.

> Source/WebCore/editing/InsertTextCommand.cpp:78
> +void InsertTextCommand::setEndingSelectionAfterTrivialReplaceOrOverwrite(const Position& startPosition, const Position& endPosition, bool selectInsertedText)

Override is basically a replace. Also, setEndingSelectionAfterTrivialReplaceOrOverwrite doesn't describe how it's different from ordinary setEndingSelection.

> Source/WebCore/editing/InsertTextCommand.cpp:120
> +    replaceTextInNodePreservingMarkers(textNode, start.offsetInContainerNode(), count, text);

Why are we preserving markers? That doesn't make much sense. e.g. all spellchecking markers, etc… will remain there.

> Source/WebCore/editing/InsertTextCommand.cpp:124
> +    if (endPosition.isNull())
> +        return false;

This is no-op since textNode is guaranteed to be not-null when Position is constructed.

> Source/WebCore/editing/InsertTextCommand.cpp:150
> +    } else {
> +        if (document()->frame()->editor()->isOverwriteModeEnabled())

else if also we need a curly brackets around two line statements here: (if & return occupy two physical lines)

> LayoutTests/editing/execCommand/overtype-support.html:12
> +if (document.queryCommandSupported("Overwrite"))
> +  testFailed("'Overwrite' command exposed to JavaScript");
> +else
> +  testPassed("'Overwrite' command NOT exposed to JavaScript");

Nit: Wrong indentation. use 4 spaces. We should also test queryCommandEnabled and queryCommandState.

> LayoutTests/editing/execCommand/overtype-support.html:19
> +  if (internals.isOverwriteModeEnabled(document)) {
> +    testFailed("'Overwrite' mode not disabled by default");
> +  } else {

I don't think testing the default state is useful in that it's not some intrinsic property of the overwrite mode.
Conceivably, some ports may want to preserve overwrite state throughout page loads in which case this is just testing that DRT/WTR resets states.

> LayoutTests/editing/execCommand/overtype-support.html:28
> +    internals.toggleOverwriteModeEnabled(document);
> +    if (!internals.isOverwriteModeEnabled(document)) {
> +      testFailed("Unable to toggle 'Overwrite' mode");
> +    } else {
> +      testPassed("'Overwrite' mode successfully enabled");
> +      internals.toggleOverwriteModeEnabled(document);
> +      if (internals.isOverwriteModeEnabled(document))
> +        testFailed("toggling 'Overwrite' mode twice does not restore the original disabled state");

I don't think these test cases are really useful since they're simply testing internals method return the right values.

> LayoutTests/editing/execCommand/overtype.html:23
> +if (!window.internals) {
> +  log("FAILED: this test requires the 'internals' object.");
> +} else {
> +  Markup.description('This is a test for Overwrite mode');
> +

Nit: wrong indentation. Use 4 spaces.

[Ryosuke Niwa]

Comment on attachment 193862 [details]
Patch

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

>> Source/WebCore/editing/InsertTextCommand.cpp:120
>> +    replaceTextInNodePreservingMarkers(textNode, start.offsetInContainerNode(), count, text);
> 
> Why are we preserving markers? That doesn't make much sense. e.g. all spellchecking markers, etc… will remain there.

r- because of this.

[Sergio Villar Senin]

Created attachment 194013 [details]
New patch addressing Ryosuke's requests

[Build Bot]

Comment on attachment 194013 [details]
New patch addressing Ryosuke's requests

Attachment 194013 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17210632

New failing tests:
editing/execCommand/enabling-and-selection-2.html

[Build Bot]

Comment on attachment 194013 [details]
New patch addressing Ryosuke's requests

Attachment 194013 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17286080

New failing tests:
editing/execCommand/enabling-and-selection-2.html

[Build Bot]

Comment on attachment 194013 [details]
New patch addressing Ryosuke's requests

Attachment 194013 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17228307

New failing tests:
editing/execCommand/enabling-and-selection-2.html

[Sergio Villar Senin]

(In reply to comment #10)
> (From update of attachment 194013 [details])
> Attachment 194013 [details] did not pass mac-ews (mac):
> Output: http://webkit-commit-queue.appspot.com/results/17228307
> 
> New failing tests:
> editing/execCommand/enabling-and-selection-2.html

I guess that only needs a specific test result for the mac? I don't have access to the exact error.

[Ryosuke Niwa]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 194013 [details] [details])
> > Attachment 194013 [details] [details] did not pass mac-ews (mac):
> > Output: http://webkit-commit-queue.appspot.com/results/17228307
> > 
> > New failing tests:
> > editing/execCommand/enabling-and-selection-2.html
> 
> I guess that only needs a specific test result for the mac? I don't have access to the exact error.

Re-upload the patch? I've fixed EWS so it should attach zip now.

[Sergio Villar Senin]

Created attachment 194580 [details]
Patch

[Build Bot]

Comment on attachment 194580 [details]
Patch

Attachment 194580 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17303103

[Build Bot]

Created attachment 194711 [details]
Archive of layout-test-results from webkit-ews-05 for mac-future

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-future  Platform: Mac OS X 10.8.2

[Sergio Villar Senin]

(In reply to comment #15)
> Created an attachment (id=194711) [details]
> Archive of layout-test-results from webkit-ews-05 for mac-future
> 
> The attached test failures were seen while running run-webkit-tests on the mac-ews.
> Bot: webkit-ews-05  Port: mac-future  Platform: Mac OS X 10.8.2

So this is the failure, 

PASS whenEnabled('OverWrite') is 'richly editable'
FAIL whenEnabled('OverWrite') should be richly editable (of type string). Was 0 (of type number).

which means that isCommandEnabled() returns always FALSE in the mac.

[Sergio Villar Senin]

Created attachment 194823 [details]
Patch

Added the new command to mac's webView, which is required by mac's TestRunner::isCommandEnabled

[Ryosuke Niwa]

Comment on attachment 194823 [details]
Patch

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

> LayoutTests/editing/execCommand/overtype-support.html:12
> +if (document.queryCommandSupported("OverWrite"))
> +    testFailed("'OverWrite' command exposed to JavaScript");
> +else
> +    testPassed("'OverWrite' command not exposed to JavaScript");

It's probably better to use shouldBe instead as in:
shouldBe('document.queryCommandSupported("OverWrite")', 'false');

> LayoutTests/editing/execCommand/overtype-support.html:14
> +if (document.queryCommandState("OverWrite") == false)

Nit: Use === or simply !document.queryCommandState("OverWrite") per WebKit style.
But really, we should be using shouldBe instead.

> LayoutTests/editing/execCommand/overtype.html:21
> +if (!window.internals) {
> +  log("FAILED: this test requires the 'internals' object.");
> +} else {

Nit: No curly brackets around a single line statement.

> LayoutTests/editing/execCommand/overtype.html:22
> +  Markup.description('This is a test for Overwrite mode');

Nit: Please indent by 4 spaces, not 2 spaces.

> LayoutTests/editing/execCommand/overtype.html:50
> +  element.innerHTML="webkit";

Nit: spaces around =.

> LayoutTests/editing/execCommand/overtype.html:51
> +  selection.setPosition(element, 0);

There's a standard equivalent collapse(element, 0).

> LayoutTests/editing/execCommand/overtype.html:55
> +    moveSelectionForwardByCharacterCommand();
> +  for (i = 0; i < 2; i++)
> +    extendSelectionForwardByCharacterCommand();

Nit: Indent by 4 spaces.

> LayoutTests/editing/execCommand/overtype.html:61
> +  element.innerHTML="ç©ä»·é¢æ";

Why don't we use entity reference here?

> LayoutTests/editing/execCommand/overtype.html:66
> +  document.execCommand("InsertText", false, 'æ¬æ¬æ¬æ¬');

Ditto.

> LayoutTests/editing/execCommand/overtype.html:69
> +  element.innerHTML="<div id=\"rtl-div\" style=\"direction: rtl;\">1234ש×××:</div>"

Ditto.

> LayoutTests/editing/execCommand/overtype.html:78
> +  // Do not forget to disable Ovewrite mode
> +  internals.toggleOverwriteModeEnabled(document);

It seems like this should be automatically reset by the test runner. Tests shouldn't have to do this manually. r- because of this.

[Sergio Villar Senin]

Created attachment 195060 [details]
Patch

[Sergio Villar Senin]

Committed r146907: <http://trac.webkit.org/changeset/146907>