This bug is a follow-up of this discussion in the mailing list https://lists.webkit.org/pipermail/webkit-dev/2013-March/024097.html
Be sure to test the implementation with bidirectional text and CJK (namely with various input methods).
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?
(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.
Created attachment 193862 [details] Patch
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.
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.
Created attachment 194013 [details] New patch addressing Ryosuke's requests
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
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
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
(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.
(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.
Created attachment 194580 [details] Patch
Comment on attachment 194580 [details] Patch Attachment 194580 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17303103
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
(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.
Created attachment 194823 [details] Patch Added the new command to mac's webView, which is required by mac's TestRunner::isCommandEnabled
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.
Created attachment 195060 [details] Patch
Committed r146907: <http://trac.webkit.org/changeset/146907>