Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2457523003:
Support 'insertReplacementText' for spellcheck (Closed)
DescriptionSupport 'insertReplacementText' for spellcheck
In this patch, we implement the insertReplacementText input event for
spellcheck. We will not cancel the selection when the before event canceled.
This is all still behind the InputEvent experimental RuntimeEnabledFeature.
BUG=652403
Committed: https://crrev.com/64f23d2d80977f018ecdcad5d34b78e24f98e980
Cr-Commit-Position: refs/heads/master@{#433794}
Patch Set 1 #
Total comments: 15
Patch Set 2 : fix for chongz's nit #
Total comments: 12
Patch Set 3 : fix for nit from Yosi_UTC9, Xiaocheng #
Total comments: 2
Patch Set 4 : fix for yosin's comments #
Total comments: 5
Patch Set 5 : dtapuska's comment addressed #
Total comments: 8
Patch Set 6 : change test #
Total comments: 1
Patch Set 7 : fix range #
Total comments: 7
Patch Set 8 : yosin@ comment addressed #
Total comments: 15
Patch Set 9 : yosin@ comment addressed #
Total comments: 2
Patch Set 10 : yosin comment addressed #Patch Set 11 : add DeleteByCut to dispatchBeforeInputDataTransfer DCHECK #Patch Set 12 : add null check #
Total comments: 2
Patch Set 13 : yosin@ comment#57 #58 addressed #Messages
Total messages: 67 (24 generated)
Description was changed from ========== insertReplacementText insertReplacementText insertReplacementText BUG= ========== to ========== Support 'insertReplacementText' for spellcheck In this patch, we implement the insertReplacementText input event for spellcheck. Cause this before event is cancelable, we will cancel the selection when the before event canceled. BUG=652403 ==========
chaopeng@chromium.org changed reviewers: + chongz@chromium.org
Please take a first look. Thank you.
The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for the patch! Here are some nits. https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:13: test(function () { Can you add ``` assert_not_equals(window.eventSender, undefined, 'This test requires eventSender.'); ``` in case we are running the test from somewhere else or manually? https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:17: lastEvent = event; I'm not sure but why would we need |lastEvent|? https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/clipboard/DataObject.cpp (right): https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/clipboard/DataObject.cpp:67: dataObject->add(data, "text/plain"); Can we import |mimeTypeTextPlain| from "ClipboardMimeTypes.h" and do ``` dataObject->setData(mimeTypeTextPlain, data); ``` https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/clipboard/DataTransfer.h (right): https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/clipboard/DataTransfer.h:60: // Whether this transfer is serving a drag-drop or copy-paste request. Can we update the comment to briefly describe what |InsertReplacementText| does? e.g. Spellcheck, auto-correct or similar. https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2095: DispatchEventResult dispatchBeforeInputPlainText( Sorry for the confusion. I was imagining we could merge this into |dispatchBeforeInputDataTransfer()|, so we only have one method |dispatchBeforeInputDataTransfer()|, which would dispatch |dataTransfer| on rich text area, and dispatch |data| on plain text area. e.g. ``` DispatchEventResult dispatchBeforeInputDataTransfer( Element* target, InputEvent::InputType inputType, DataTransfer* dataTransfer, const RangeVector* ranges); ``` And we can get string data from |dataTransfer.getData(mimeTypeTextPlain)|. Or do you have other concerns? Note: We can pass |Element* target| so we don't have to call |target->toNode()|. https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/Editor.cpp:1308: InputEvent::InputType::InsertFromPaste); Can you add ``` TODO(chongz): Support |InsertTranspose|. ``` Thanks! https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:831: frame().selection().selection().toNormalizedEphemeralRange())); I think you can just do ``` new RangeVector(1, m_frame->selection().firstRange()); ``` See https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/I... https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:838: Can you add the following check after dispatching 'beforeinput'? ``` // 'beforeinput' event handler may destroy target frame. if (m_frame->document()->frame() != m_frame) return; ``` The reason is JS handlers may destroy frame (e.g. iframe), thus the code after would crash. See https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/E... https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:841: frame().selection().setSelection(SelectionInDOMTree::Builder() Thinking more about this I think we shouldn't reset selection. For example if JS canceled 'beforeinput' and handled itself (replace text and adjust selection), resetting selection here will make their life harder. https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:853: frame().editor().replaceSelectionWithText( Actually can we dispatch 'beforeinput' right before this line? I think 'beforeinput' should only cover the DOM mutation part, and shouldn't affect other stuff like style or selection. https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/w... File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right): https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/w... third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:978: InputEvent::InputType::InsertFromPaste); I cannot find the usage of this method, but we probably should use |InputEvent::InputType::InsertReplacementText|?
PTAL. Thank you. https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/clipboard/DataObject.cpp (right): https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/clipboard/DataObject.cpp:67: dataObject->add(data, "text/plain"); On 2016/10/27 16:41:24, chongz wrote: > Can we import |mimeTypeTextPlain| from "ClipboardMimeTypes.h" and do > ``` > dataObject->setData(mimeTypeTextPlain, data); > ``` Do we have any better head file? Cause this is not related to Clipboard. https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:853: frame().editor().replaceSelectionWithText( On 2016/10/27 16:41:25, chongz wrote: > Actually can we dispatch 'beforeinput' right before this line? I think > 'beforeinput' should only cover the DOM mutation part, and shouldn't affect > other stuff like style or selection. frame().document()->updateStyleAndLayoutIgnorePendingStylesheets(); must call just before replaceSelectionWithText or selection() will fail.
chongz@chromium.org changed reviewers: + yosin@chromium.org
Hi yosin@, chaopeng@ also started working on InputEvent, PTAL, thanks! https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/clipboard/DataObject.cpp (right): https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/clipboard/DataObject.cpp:67: dataObject->add(data, "text/plain"); On 2016/10/28 00:33:08, chaopeng wrote: > On 2016/10/27 16:41:24, chongz wrote: > > Can we import |mimeTypeTextPlain| from "ClipboardMimeTypes.h" and do > > ``` > > dataObject->setData(mimeTypeTextPlain, data); > > ``` > > Do we have any better head file? Cause this is not related to Clipboard. Will see if yosin@ has some suggestions. https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2457523003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:853: frame().editor().replaceSelectionWithText( On 2016/10/28 00:33:08, chaopeng wrote: > On 2016/10/27 16:41:25, chongz wrote: > > Actually can we dispatch 'beforeinput' right before this line? I think > > 'beforeinput' should only cover the DOM mutation part, and shouldn't affect > > other stuff like style or selection. > > frame().document()->updateStyleAndLayoutIgnorePendingStylesheets(); must call > just before replaceSelectionWithText or selection() will fail. I see, I'm not sure how |updateStyleAndLayoutIgnorePendingStylesheets()| works, but maybe yosin@ could give some suggestions. https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2092: inputType == InputEvent::InputType::InsertReplacementText); |inputType| could also be |InsertFromDrop|.
chaopeng@chromium.org changed reviewers: + xiaochengh@chromium.org
xiaochengh@ PTAL, we found frame().document()->updateStyleAndLayoutIgnorePendingStylesheets(); must call just before replaceSelectionWithText or selection() will fail. Do you have any suggestion. Thank you.
Yes, Editor::replaceSelectionFoo() requires clean layout. Same holds for VisibleSelection::toNormalizedEphemeralRange() and basically all functions of SpellChecker. This is due to an on-going refactoring work of eliminating layout update calls that are deep in the call stack. Please call Document::updateStyleAndLayoutIgnorePendingStylesheets() if you introduce a new call site. If the call site is outside testing code, please also add the following comments so that we can track all the editing-related layout update calls: // TODO(editing-dev): The use of updateStyleAndLayoutIgnorePendingStylesheets // needs to be audited. See http://crbug.com/590369 for more details. https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:1797: Please call |document->updateStyleAndLayoutIgnorePendingStylesheet()| here, because selection normalization requires clean layout. https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:1802: document->updateStyleAndLayoutIgnorePendingStylesheets(); I don't think we should update layout here. https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:1828: Please call document->updateStyleAndLayoutIgnorePendingStylesheet() here, as SpellChecker::replaceMisspelledRange() requires clean layout.
https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:42: assert_equals(editable.innerText, "apple"); Could you utilize assert_selection()? https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2092: inputType == InputEvent::InputType::InsertReplacementText); Could you add "<< inputType" for ease of debugging? https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:215: if (event->pastingFragment()) { We prefer early return style. https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:835: bool cancel = dispatchBeforeInputDataTransfer( nit: s/bool/const bool/ https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:847: if (!cancel) { We prefer early-return style. https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:1795: DCHECK(document); Please throw an exception rather than DCHECK(). ClusterFuzz sometime includes a method in Internals.idl. https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:1826: DCHECK(document); Please throw an exception rather than DCHECK(). ClusterFuzz sometime includes a method in Internals.idl.
https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:42: assert_equals(editable.innerText, "apple"); On 2016/10/28 04:06:14, Yosi_UTC9 wrote: > Could you utilize assert_selection()? Do you mean utilize assert_selection in testharness.js or in this test? And for this line, we cannot assert_selection cause we cancel the selection after finish the replacement.
On 2016/10/28 14:28:31, chaopeng wrote: > https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html > (right): > > https://codereview.chromium.org/2457523003/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:42: > assert_equals(editable.innerText, "apple"); > On 2016/10/28 04:06:14, Yosi_UTC9 wrote: > > Could you utilize assert_selection()? > > Do you mean utilize assert_selection in testharness.js or in this test? And for > this line, we cannot assert_selection cause we cancel the selection after finish > the replacement. Updated, PTAL. Thank you.
chaopeng@chromium.org changed reviewers: + aelias@chromium.org, dtapuska@chromium.org, rbyers@chromium.org
aelias@chromium.org: Please review changes in Web rbyers@chromium.org: Please review changes in Core
Description was changed from ========== Support 'insertReplacementText' for spellcheck In this patch, we implement the insertReplacementText input event for spellcheck. Cause this before event is cancelable, we will cancel the selection when the before event canceled. BUG=652403 ========== to ========== Support 'insertReplacementText' for spellcheck In this patch, we implement the insertReplacementText input event for spellcheck. We will not cancel the selection when the before event canceled. BUG=652403 ==========
This seems pointless and counterproductive because it doesn't cover Android IME-caused spellcheck which is probably like 95% of all spellcheck processed by Chrome. Nor is there any way to support Android IME spellcheck because they simply appear as normal composition replacement with no signal as to the reason. So I oppose adding this new event, I'll reply on the https://github.com/w3c/input-events/issues/31
OK, after a bit more time to explore the problem space, I've withdrawn my spec-level concern on this particular point (see history on github). Source/web lgtm
https://codereview.chromium.org/2457523003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): https://codereview.chromium.org/2457523003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:10: function assert_selection(expected) { Please use editing/assert_selection.js https://codereview.chromium.org/2457523003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:15: internals.setSpellCheckingEnabled(true); Move this inside after checking |windows.internals|
https://codereview.chromium.org/2457523003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): https://codereview.chromium.org/2457523003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:47: console.log(selection.toString()); // selection is empty Hi Yosin, I changed the test to assert selection. It works for div but not for textarea. Here the selection is empty not matter I use `|appla^` or `^appla|`. Can you please take a look. Thank you.
On 2016/10/28 15:11:24, chaopeng wrote: > mailto:aelias@chromium.org: Please review changes in Web > > mailto:rbyers@chromium.org: Please review changes in Core Sorry I haven't been following the design details here closely enough. dtapuska@ can you review core/ and if you and yosin@ are happy I'll stamp it for OWNERS.
https://codereview.chromium.org/2457523003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): https://codereview.chromium.org/2457523003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:47: console.log(selection.toString()); // selection is empty On 2016/11/02 20:45:25, chaopeng wrote: > Hi Yosin, I changed the test to assert selection. It works for div but not for > textarea. Here the selection is empty not matter I use `|appla^` or `^appla|`. > Can you please take a look. Thank you. I'm not exactly sure how |assert_selection()| works, but AFAIK selection doesn't work for textarea/input, and you will have to use something like |textarea.selectionStart/End|, and maybe that's the cause? https://developer.mozilla.org/en-US/docs/Web/API/HTMLTextAreaElement
https://codereview.chromium.org/2457523003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): https://codereview.chromium.org/2457523003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:47: console.log(selection.toString()); // selection is empty On 2016/11/03 at 15:48:34, chongz wrote: > On 2016/11/02 20:45:25, chaopeng wrote: > > Hi Yosin, I changed the test to assert selection. It works for div but not for > > textarea. Here the selection is empty not matter I use `|appla^` or `^appla|`. > > Can you please take a look. Thank you. > > I'm not exactly sure how |assert_selection()| works, but AFAIK selection doesn't work for textarea/input, and you will have to use something like |textarea.selectionStart/End|, and maybe that's the cause? > > https://developer.mozilla.org/en-US/docs/Web/API/HTMLTextAreaElement I'm now updating assert_selection() to support INPUT/TEXTAREA. Stay tuned!
https://codereview.chromium.org/2457523003/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): https://codereview.chromium.org/2457523003/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:47: console.log(selection.toString()); // selection is empty On 2016/11/08 at 04:34:27, Yosi_UTC9 wrote: > On 2016/11/03 at 15:48:34, chongz wrote: > > On 2016/11/02 20:45:25, chaopeng wrote: > > > Hi Yosin, I changed the test to assert selection. It works for div but not for > > > textarea. Here the selection is empty not matter I use `|appla^` or `^appla|`. > > > Can you please take a look. Thank you. > > > > I'm not exactly sure how |assert_selection()| works, but AFAIK selection doesn't work for textarea/input, and you will have to use something like |textarea.selectionStart/End|, and maybe that's the cause? > > > > https://developer.mozilla.org/en-US/docs/Web/API/HTMLTextAreaElement > > I'm now updating assert_selection() to support INPUT/TEXTAREA. Stay tuned! assert_selection() with TEXTAREA http://crrev.com/2487093002
lgtm % one not https://codereview.chromium.org/2457523003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2457523003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2104: // TODO(chongz): Pass appreciate |ranges| after it's defined on spec. Should this comment indicate // Pass appropriate? not // Pass appreciate ?
On 2016/11/09 05:25:55, Yosi_UTC9 wrote: > https://codereview.chromium.org/2457523003/diff/60001/third_party/WebKit/Layo... > File > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html > (right): > > https://codereview.chromium.org/2457523003/diff/60001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:47: > console.log(selection.toString()); // selection is empty > On 2016/11/08 at 04:34:27, Yosi_UTC9 wrote: > > On 2016/11/03 at 15:48:34, chongz wrote: > > > On 2016/11/02 20:45:25, chaopeng wrote: > > > > Hi Yosin, I changed the test to assert selection. It works for div but not > for > > > > textarea. Here the selection is empty not matter I use `|appla^` or > `^appla|`. > > > > Can you please take a look. Thank you. > > > > > > I'm not exactly sure how |assert_selection()| works, but AFAIK selection > doesn't work for textarea/input, and you will have to use something like > |textarea.selectionStart/End|, and maybe that's the cause? > > > > > > https://developer.mozilla.org/en-US/docs/Web/API/HTMLTextAreaElement > > > > I'm now updating assert_selection() to support INPUT/TEXTAREA. Stay tuned! > > assert_selection() with TEXTAREA http://crrev.com/2487093002 yosin@ PTAL Thank you.
https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:18: var document = selection.document; nit: It is better to use |let| or |const| instead of |var|. https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2093: << "Unsupported inputType: " << (int)inputType; nit: s/(int)inputType/static_cast<int>(inputType)/ For ease of debugging, it is better to have |operator<<(std::ostream&, InputType)| See "core/editing/commands/EditorCommandNames.h" how to implement |operator<<()| w/o hassle. ;-) https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:828: RangeVector* ranges = new RangeVector(1, frame().selection().firstRange()); Is this work if |FrameSelection::firstRange()| returns null? Since |FrameSelection::setSelection()| can invoke JS, DOM tree can be gone away here. https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:1798: void Internals::setSpellingMarker(Document* document, It is better that |setSepllingMarker| to take |Range| instead of obtaining range for marker from selection for increase capability of API and modulality. Also, I recommend to have |setMarker(range, markerType)| to offer more capability. xiaochengh@, WDYT? I think spell checker has tests to verify removing marker when typing correct the spell. |setMarker()| might help.
https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:828: RangeVector* ranges = new RangeVector(1, frame().selection().firstRange()); On 2016/11/10 at 01:49:52, Yosi_UTC9 wrote: > Is this work if |FrameSelection::firstRange()| returns null? > > Since |FrameSelection::setSelection()| can invoke JS, DOM tree can be gone away here. This seems to be a bug that we introduced. When hoisting layout updates, we did not check whether DOM is gone after FS::setSelection(), and just called updateSAL directly. https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:847: if (cancel) Let's move this return to be before updateStyleAndLayout... so that layout is updated less frequently. https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:1798: void Internals::setSpellingMarker(Document* document, On 2016/11/10 at 01:49:52, Yosi_UTC9 wrote: > It is better that |setSepllingMarker| to take |Range| instead of obtaining range for marker from selection for increase capability of API and modulality. > > Also, I recommend to have |setMarker(range, markerType)| to offer more capability. > > xiaochengh@, WDYT? I think spell checker has tests to verify removing marker when typing correct the spell. |setMarker()| might help. Agreed. In fact, that would help us a lot by simplifying the following two types of tests: 1. Verification of stale marker removal 2. Verification of marker appearance So, we would appreciate a lot if we could have setMarker(range, markerType) in this patch :)
https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/2457523003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:1798: void Internals::setSpellingMarker(Document* document, On 2016/11/10 at 02:35:53, Xiaocheng wrote: > On 2016/11/10 at 01:49:52, Yosi_UTC9 wrote: > > It is better that |setSepllingMarker| to take |Range| instead of obtaining range for marker from selection for increase capability of API and modulality. > > > > Also, I recommend to have |setMarker(range, markerType)| to offer more capability. > > > > xiaochengh@, WDYT? I think spell checker has tests to verify removing marker when typing correct the spell. |setMarker()| might help. > > Agreed. In fact, that would help us a lot by simplifying the following two types of tests: > > 1. Verification of stale marker removal > 2. Verification of marker appearance > > So, we would appreciate a lot if we could have setMarker(range, markerType) in this patch :) As usual, it is better to have another patch containing Internals changes. ;-)
https://codereview.chromium.org/2457523003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): https://codereview.chromium.org/2457523003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:48: internals.setMarker(document, selection.getRangeAt(0), 'Spelling'); yosin@ selection.getRangeAt(0) is not working for textarea. can you take a look at it.
On 2016/11/15 at 20:02:59, chaopeng wrote: > https://codereview.chromium.org/2457523003/diff/100001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): > > https://codereview.chromium.org/2457523003/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:48: internals.setMarker(document, selection.getRangeAt(0), 'Spelling'); > yosin@ selection.getRangeAt(0) is not working for textarea. can you take a look at it. Selections in form control elements (<input>, <textarea>) cannot be manipulated directly because it's hidden in the shadow tree under the element. We need to break into the shadow tree to get the selection range: const shadowRoot = internals.shadowRoot(textarea); const selection = shadowRoot.getSelection(); const range = selection.getRangeAt(0); ...
On 2016/11/16 01:05:27, Xiaocheng wrote: > On 2016/11/15 at 20:02:59, chaopeng wrote: > > > https://codereview.chromium.org/2457523003/diff/100001/third_party/WebKit/Lay... > > File > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html > (right): > > > > > https://codereview.chromium.org/2457523003/diff/100001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:48: > internals.setMarker(document, selection.getRangeAt(0), 'Spelling'); > > yosin@ selection.getRangeAt(0) is not working for textarea. can you take a > look at it. > > Selections in form control elements (<input>, <textarea>) cannot be manipulated > directly because it's hidden in the shadow tree under the element. > > We need to break into the shadow tree to get the selection range: > > const shadowRoot = internals.shadowRoot(textarea); > const selection = shadowRoot.getSelection(); > const range = selection.getRangeAt(0); > ... It works. Thank you Xiaocheng@. yosin@ Xiaocheng@ PTAL. Thank you.
https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:8: assert_not_equals(window.eventSender, Please get rid of this statement since this test case doesn't use |eventSender|. https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:22: const handler = function(event) { nit: s/const handler/function handler(event)/ https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:23: assert_equals(event.inputType, "insertReplacementText"); nit: Please use single-quote since other parts of script use single-quote. https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:26: editable.removeEventListener('beforeinput', handler); Q: Do we really need to remove event listener? https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:37: assert_not_equals(window.eventSender, Please get rid of this statement since this test case doesn't use |eventSender|. https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:55: const handler = function(event) { nit: s/const handler/function handler(event)/ https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:59: textarea.removeEventListener('beforeinput', handler); Q: Do we really need to remove event listener?
On 2016/11/16 04:14:46, Yosi_UTC9 wrote: > https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html > (right): > > https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:8: > assert_not_equals(window.eventSender, > Please get rid of this statement since this test case doesn't use |eventSender|. > > https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:22: > const handler = function(event) { > nit: s/const handler/function handler(event)/ > > https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:23: > assert_equals(event.inputType, "insertReplacementText"); > nit: Please use single-quote since other parts of script use single-quote. > > https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:26: > editable.removeEventListener('beforeinput', handler); > Q: Do we really need to remove event listener? > > https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:37: > assert_not_equals(window.eventSender, > Please get rid of this statement since this test case doesn't use |eventSender|. > > https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:55: > const handler = function(event) { > nit: s/const handler/function handler(event)/ > > https://codereview.chromium.org/2457523003/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:59: > textarea.removeEventListener('beforeinput', handler); > Q: Do we really need to remove event listener? Updated, PTAL
https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:7: test(function () { nit: s/function ()/() =>/ https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:19: function handler(event) { Since we refer |handler| once, it should be an closure argument of |addEventListener()|. https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:23: }; nit: no need to have semi-colon(;) https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:29: ); nit: Should be at end of L28. https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:32: test(function () { nit: s/function ()/() =>/ https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:41: const textarea = document.getElementById('ta'); Since there is only one element, how about removing "id" attribute and using document.querySelector('textarea')? In this way, sample and expectation are simpler. https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:48: function handler(event) { Since we refer |handler| once, it should be an closure argument of |addEventListener()|. https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:52: }; nit: no need to have semi-colon(;) https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:58: ); nit: Should be at end of L57 https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:60: nit: Get rid of an extra blank line. https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/Editor.cpp:1307: // TODO(chongz) Support |InsertTranspose| nit: Please write TODO comment more descriptive with 5W1H in mind, e.g. TODO(chongz): Once we add |InsertTranspose| in |InputEvent::InputType|, we should use it instead of |InsertFromPaste|. or TODO(chongz): We should use |InsertTranspose| instead of |InsertFromPaste| once we introduce it in |InputEvent::InputType|. BTW, I could have another patch to introduce |InsertTranspose|, it is very simple patch and easy to review, and not put TODO. ;-) https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:822: nit: Please don't add an extra blank line. https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:827: Element* target = frame().editor().findEventTargetFromSelection(); nit: s/Element*/Element* const/ https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:828: RangeVector* ranges = new RangeVector(1, frame().selection().firstRange()); nit: s/RangeVector*/RangeVector* const/ https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:829: DataTransfer* dataTransfer = DataTransfer::create( nit: s/DataTransfer*/DataTransfer* const/
Patchset #9 (id:160001) has been deleted
On 2016/11/17 04:06:48, Yosi_UTC9 wrote: > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html > (right): > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:7: > test(function () { > nit: s/function ()/() =>/ > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:19: > function handler(event) { > Since we refer |handler| once, it should be an closure argument of > |addEventListener()|. > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:23: > }; > nit: no need to have semi-colon(;) > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:29: > ); > nit: Should be at end of L28. > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:32: > test(function () { > nit: s/function ()/() =>/ > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:41: > const textarea = document.getElementById('ta'); > Since there is only one element, how about removing "id" attribute and using > document.querySelector('textarea')? > In this way, sample and expectation are simpler. > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:48: > function handler(event) { > Since we refer |handler| once, it should be an closure argument of > |addEventListener()|. > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:52: > }; > nit: no need to have semi-colon(;) > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:58: > ); > nit: Should be at end of L57 > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:60: > > nit: Get rid of an extra blank line. > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/Editor.cpp (right): > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/Editor.cpp:1307: // TODO(chongz) Support > |InsertTranspose| > nit: Please write TODO comment more descriptive with 5W1H in mind, e.g. > > TODO(chongz): Once we add |InsertTranspose| in |InputEvent::InputType|, we > should use it instead of |InsertFromPaste|. > or > TODO(chongz): We should use |InsertTranspose| instead of |InsertFromPaste| once > we introduce it in |InputEvent::InputType|. > > BTW, I could have another patch to introduce |InsertTranspose|, it is very > simple patch and easy to review, and not put TODO. ;-) > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:822: > nit: Please don't add an extra blank line. > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:827: Element* > target = frame().editor().findEventTargetFromSelection(); > nit: s/Element*/Element* const/ > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:828: > RangeVector* ranges = new RangeVector(1, frame().selection().firstRange()); > nit: s/RangeVector*/RangeVector* const/ > > https://codereview.chromium.org/2457523003/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:829: > DataTransfer* dataTransfer = DataTransfer::create( > nit: s/DataTransfer*/DataTransfer* const/ Updated, PTAL. Thank you.
Patchset #10 (id:200001) has been deleted
lgtm w/ small nits https://codereview.chromium.org/2457523003/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html (right): https://codereview.chromium.org/2457523003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:9: undefined, nit: align arguments to "(", like: assert_not_equals(window.internals, undefined, 'This test requires internals.'); https://codereview.chromium.org/2457523003/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-spellcheck.html:31: assert_not_equals(window.internals, nit: align arguments to "(", like: assert_not_equals(window.internals, undefined, 'This test requires internals.');
rbyers@ PTAL
Description was changed from ========== Support 'insertReplacementText' for spellcheck In this patch, we implement the insertReplacementText input event for spellcheck. We will not cancel the selection when the before event canceled. BUG=652403 ========== to ========== Support 'insertReplacementText' for spellcheck In this patch, we implement the insertReplacementText input event for spellcheck. We will not cancel the selection when the before event canceled. This is all still behind the InputEvent experimental RuntimeEnabledFeature. BUG=652403 ==========
On 2016/11/21 01:56:49, chaopeng wrote: > rbyers@ PTAL dtapuska@, aelias@ and yosin@ are experts here. RS LGTM I added a note to the CL description to clarify that this isn't shipping anything new yet (still behind the flag), look good?
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, dtapuska@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2457523003/#ps220001 (title: "yosin comment addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, dtapuska@chromium.org, rbyers@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2457523003/#ps240001 (title: "add DeleteByCut to dispatchBeforeInputDataTransfer DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm w/ small nit. https://codereview.chromium.org/2457523003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/2457523003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:2103: String data = dataTransfer->getData(mimeTypeTextPlain); nit: s/String/const String&/ to avoid copy in ctor.
https://codereview.chromium.org/2457523003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2457523003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:840: if (frame().document()->frame() != m_frame) This if-condition always true. frame() == m_frame. We need to save current document before dispatch, e.g. Document& currentDocument = frame().document(); const bool canceled = dispatchBeforeInputDataTransfer(....); if (currentDocument() != frame.document()) return;
lgtm Go ahead!
The CQ bit was checked by chaopeng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, dtapuska@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/2457523003/#ps280001 (title: "yosin@ comment#57 #58 addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1479781041859700,
"parent_rev": "c0b6822795af09f40dc00b4bc238c63df20ea3e1", "commit_rev":
"45d3d67713d9a74ba07b4b435e0feac48b2157d3"}
Message was sent while issue was closed.
Description was changed from ========== Support 'insertReplacementText' for spellcheck In this patch, we implement the insertReplacementText input event for spellcheck. We will not cancel the selection when the before event canceled. This is all still behind the InputEvent experimental RuntimeEnabledFeature. BUG=652403 ========== to ========== Support 'insertReplacementText' for spellcheck In this patch, we implement the insertReplacementText input event for spellcheck. We will not cancel the selection when the before event canceled. This is all still behind the InputEvent experimental RuntimeEnabledFeature. BUG=652403 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Support 'insertReplacementText' for spellcheck In this patch, we implement the insertReplacementText input event for spellcheck. We will not cancel the selection when the before event canceled. This is all still behind the InputEvent experimental RuntimeEnabledFeature. BUG=652403 ========== to ========== Support 'insertReplacementText' for spellcheck In this patch, we implement the insertReplacementText input event for spellcheck. We will not cancel the selection when the before event canceled. This is all still behind the InputEvent experimental RuntimeEnabledFeature. BUG=652403 Committed: https://crrev.com/64f23d2d80977f018ecdcad5d34b78e24f98e980 Cr-Commit-Position: refs/heads/master@{#433794} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/64f23d2d80977f018ecdcad5d34b78e24f98e980 Cr-Commit-Position: refs/heads/master@{#433794} |
