Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1752933002:
[InputEvent] Fire 'beforeinput' during typing, pressing hot keys and IME composition (Closed)
DescriptionFire 'beforeinput' during typing, pressing hot keys and IME composition
Support |inputType| and |data| field.
'beforeinput' will be fired when:
1. Typing without IME
2. Pressing hot keys like Ctrl-Z
3. Typing with IME, will be fire before compositionUpdate
'beforeinput' will not be fired by Document.execCommand().
SPEC=http://w3c.github.io/editing/input-events.html#event-type-beforeinput
BUG=585875
Committed: https://crrev.com/232c62972c919927922d08050749280f11ce4f2b
Cr-Commit-Position: refs/heads/master@{#388519}
Patch Set 1 #
Total comments: 10
Patch Set 2 : dtapuska's review, also add layout tests #
Total comments: 8
Patch Set 3 : Yosi's review #
Total comments: 18
Patch Set 4 : Rebase on WebEditingCommandType & Yosin's review 2 #
Total comments: 32
Patch Set 5 : Yosin's review 3 #
Total comments: 27
Patch Set 6 : ojan's review #
Total comments: 1
Patch Set 7 : Use 'foo == enum' instead of static_cast<> #Patch Set 8 : Rebase #Messages
Total messages: 44 (14 generated)
chongz@chromium.org changed reviewers: + dtapuska@chromium.org
Hi dtapuska@, PTAL, thanks! https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp (right): https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp:66: RefPtrWillBeRawPtr<InputEvent> beforeInputEvent = InputEvent::createBeforeInput("insertText", evt->keyEvent()->text()); Will replace "insertText" with constant command name. https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodController.cpp:163: RefPtrWillBeRawPtr<InputEvent> beforeInputEvent = InputEvent::createBeforeInput("insertText", text); https://developer.mozilla.org/en-US/docs/Web/API/Document/execCommand#Commands doesn't have something like 'replaceText', or should it just be 'insertText' with a range? https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodController.cpp:272: RefPtrWillBeRawPtr<InputEvent> beforeInputEvent = InputEvent::createBeforeInput("replaceContent", text); 'replaceContent' is mentioned here http://w3c.github.io/editing/input-events.html#widl-InputEvent-inputType Not sure which one I should use. https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1786: RefPtrWillBeRawPtr<InputEvent> beforeInputEvent = InputEvent::createBeforeInput("executeCommand", ""); Didn't find a way to get command name from Editor::Command, should I add a name field to Editor::Command?
https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp (right): https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp:66: RefPtrWillBeRawPtr<InputEvent> beforeInputEvent = InputEvent::createBeforeInput("insertText", evt->keyEvent()->text()); On 2016/03/01 20:41:10, chongz wrote: > Will replace "insertText" with constant command name. you can likely store the atomic string in a static I think. https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp:69: return true; I'm not certain returning true here is correct. As that indicates the keyboard event was default handled doesn't it? I guess this might be ok; have to think about it. https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodController.cpp:160: if (RuntimeEnabledFeatures::inputEventEnabled()) { This seems to be a repeated pattern in this class. Can we use an anonymous function? https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/InputMethodController.cpp:272: RefPtrWillBeRawPtr<InputEvent> beforeInputEvent = InputEvent::createBeforeInput("replaceContent", text); On 2016/03/01 20:41:10, chongz wrote: > 'replaceContent' is mentioned here > http://w3c.github.io/editing/input-events.html#widl-InputEvent-inputType > Not sure which one I should use. Let us leave it with replace text for now and see what the spec comes up as. https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1786: RefPtrWillBeRawPtr<InputEvent> beforeInputEvent = InputEvent::createBeforeInput("executeCommand", ""); On 2016/03/01 20:41:10, chongz wrote: > Didn't find a way to get command name from Editor::Command, should I add a name > field to Editor::Command? Ya I think you'd want to load it from the EditorInternalCommand class..
Description was changed from ========== Fire 'beforeinput' during typing, pressing hot keys and IME composition Support |inputType| and |data| field. 'beforeinput' will be fired when: 1. Typing without IME 2. Pressing hot keys like Ctrl-C/V 3. Typing with IME, will be fire before composition events 'beforeinput' will not be fired by Document.execCommand(). SPEC=http://w3c.github.io/editing/input-events.html#event-type-beforeinput BUG= ========== to ========== Fire 'beforeinput' during typing, pressing hot keys and IME composition Support |inputType| and |data| field. 'beforeinput' will be fired when: 1. Typing without IME 2. Pressing hot keys like Ctrl-C/V 3. Typing with IME, will be fire before compositionUpdate 'beforeinput' will not be fired by Document.execCommand(). SPEC=http://w3c.github.io/editing/input-events.html#event-type-beforeinput BUG= ==========
chongz@chromium.org changed reviewers: + ojan@chromium.org
PTAL, thanks! https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp (right): https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp:69: return true; On 2016/03/01 21:49:51, dtapuska wrote: > I'm not certain returning true here is correct. As that indicates the keyboard > event was default handled doesn't it? I guess this might be ok; have to think > about it. To my understanding the return value here should be 'defaultHandled() || defaultPrevented()', otherwise preventing beforeinput for space will cause scroll I guess? https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:1678: RefPtrWillBeRawPtr<InputEvent> beforeInputEvent = InputEvent::createBeforeInput(kInputTypeReplaceText, data); I'm not so sure about the inputType we want to use here? https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1780: return true; I assume we don't want to fallback to other default actions if, for example, Ctrl-V's beforeinput got canceled?
Description was changed from ========== Fire 'beforeinput' during typing, pressing hot keys and IME composition Support |inputType| and |data| field. 'beforeinput' will be fired when: 1. Typing without IME 2. Pressing hot keys like Ctrl-C/V 3. Typing with IME, will be fire before compositionUpdate 'beforeinput' will not be fired by Document.execCommand(). SPEC=http://w3c.github.io/editing/input-events.html#event-type-beforeinput BUG= ========== to ========== Fire 'beforeinput' during typing, pressing hot keys and IME composition Support |inputType| and |data| field. 'beforeinput' will be fired when: 1. Typing without IME 2. Pressing hot keys like Ctrl-C/V 3. Typing with IME, will be fire before compositionUpdate 'beforeinput' will not be fired by Document.execCommand(). SPEC=http://w3c.github.io/editing/input-events.html#event-type-beforeinput BUG=585875 ==========
On 2016/03/04 19:46:50, chongz wrote: > PTAL, thanks! > > https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp (right): > > https://codereview.chromium.org/1752933002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/EditorKeyBindings.cpp:69: return true; > On 2016/03/01 21:49:51, dtapuska wrote: > > I'm not certain returning true here is correct. As that indicates the keyboard > > event was default handled doesn't it? I guess this might be ok; have to think > > about it. > > To my understanding the return value here should be 'defaultHandled() || > defaultPrevented()', otherwise preventing beforeinput for space will cause > scroll I guess? > > https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): > > https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/EditingUtilities.cpp:1678: > RefPtrWillBeRawPtr<InputEvent> beforeInputEvent = > InputEvent::createBeforeInput(kInputTypeReplaceText, data); > I'm not so sure about the inputType we want to use here? > > https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): > > https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1780: return > true; > I assume we don't want to fallback to other default actions if, for example, > Ctrl-V's beforeinput got canceled? Hi Ojan, can you take a look at this CL please? Or can you point to someone else to review? Much appreciated!
yosin@chromium.org changed reviewers: + yosin@chromium.org
https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html (right): https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html:11: nit: please remove an extra blank line. https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html:24: assert_equals(lastData, data, modifiers.toString() + "+" + key + " should produce data:" + data); nit: It is better to use template string to improve readability: `${modifiers.toString()}+${key} should produce data: ${data}` https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html:39: nit: please remove an extra blank line. https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:1658: static const char* kInputTypeInsertText = "insertText"; Could you move |kInputTypeXXX| into "InputEvent.h"? https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:76: int idForUserMetrics; I think we should have a field to specify whether "this" command needs to dispatch "beforeinput" or not. Since we should not dispatch "beforeInput" for "moveLeft", "moveUp", etc. Having |enum class InputType { None, InsertCharacter, ReplaceText, ...}| then |InputType m_inputType|? https://codereview.chromium.org/1752933002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1782: frame().document()->updateLayoutIgnorePendingStylesheets(); Could you check whether |frame().document()| is null or not? "beforeInput" event handler may destroy |frame()|.
Hi Yosi, thank you so much for the review! I've updated CL as per your comments, PTAL, thanks! https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1545: { "MoveBackward", 53, executeMoveBackward, supportedFromMenuOrKeyBinding, enabledInEditableTextOrCaretBrowsing, stateNone, valueNull, notTextInsertion, doNotFireInputEvent, doNotAllowExecutionWhenDisabled }, Currently set |doNotFireInputEvent| to commands "Move*", "Print", "Scroll*", "Select*", is there any other commands that shouldn't fire input events? https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:13: String InputTypeToString(InputEvent::InputType inputType) Cannot do |InputType m_inputType| because I need to create InputEvent from |commandName|, or should I enum all the possible command names?
https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:296: // If text is empty, then delete the old composition here. If text is non-empty, InsertTextCommand::input We also dispatch "beforeInput" with "deleteComposedCharacter" in |InputMethodController::extendSelectionAndDelete()|. Note: Blink doesn't have information about "forward delete" or "backward delete". https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1545: { "MoveBackward", 53, executeMoveBackward, supportedFromMenuOrKeyBinding, enabledInEditableTextOrCaretBrowsing, stateNone, valueNull, notTextInsertion, doNotFireInputEvent, doNotAllowExecutionWhenDisabled }, On 2016/03/15 at 19:19:10, chongz wrote: > Currently set |doNotFireInputEvent| to commands "Move*", "Print", "Scroll*", "Select*", is there any other commands that shouldn't fire input events? Entries having |shouldFireInputEvent| should have |supportedFromMenuOrKeyBinding|. We don't need to fire "beforeInput" from |document.execCommand()|. https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1783: if (m_command->shouldFireInputEvent) { We should check this command is invoked by user action, e.g. |m_source == CommandFromMenuOrKeyBinding|. https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:13: String InputTypeToString(InputEvent::InputType inputType) On 2016/03/15 at 19:19:10, chongz wrote: > Cannot do |InputType m_inputType| because I need to create InputEvent from |commandName|, or should I enum all the possible command names? |inputType| doesn't hold a command name rather it holds a type of input the spec. So, we can define it as |enum class|.
https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:13: String InputTypeToString(InputEvent::InputType inputType) On 2016/03/16 at 01:46:51, Yosi_UTC9 wrote: > On 2016/03/15 at 19:19:10, chongz wrote: > > Cannot do |InputType m_inputType| because I need to create InputEvent from |commandName|, or should I enum all the possible command names? > > |inputType| doesn't hold a command name rather it holds a type of input the spec. So, we can define it as |enum class|. |inputType| is still in discussion, https://github.com/w3c/editing/issues/110
Thanks for the review! But I got a few questions that I want to clarify before implementing... https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:296: // If text is empty, then delete the old composition here. If text is non-empty, InsertTextCommand::input On 2016/03/16 01:46:51, Yosi_UTC9 wrote: > We also dispatch "beforeInput" with "deleteComposedCharacter" in > |InputMethodController::extendSelectionAndDelete()|. > Note: Blink doesn't have information about "forward delete" or "backward > delete". Acknowledged. https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1545: { "MoveBackward", 53, executeMoveBackward, supportedFromMenuOrKeyBinding, enabledInEditableTextOrCaretBrowsing, stateNone, valueNull, notTextInsertion, doNotFireInputEvent, doNotAllowExecutionWhenDisabled }, On 2016/03/16 01:46:51, Yosi_UTC9 wrote: > On 2016/03/15 at 19:19:10, chongz wrote: > > Currently set |doNotFireInputEvent| to commands "Move*", "Print", "Scroll*", > "Select*", is there any other commands that shouldn't fire input events? > > Entries having |shouldFireInputEvent| should have > |supportedFromMenuOrKeyBinding|. > We don't need to fire "beforeInput" from |document.execCommand()|. > I'm not so sure if it's necessary, I guess we can just filter the source in |Editor::Command::execute|? e.g. "InsertText" is |supported| and we want to fire "beforeinput" if it's from keybinding right? https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1783: if (m_command->shouldFireInputEvent) { On 2016/03/16 01:46:51, Yosi_UTC9 wrote: > We should check this command is invoked by user action, e.g. |m_source == > CommandFromMenuOrKeyBinding|. Acknowledged. https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:13: String InputTypeToString(InputEvent::InputType inputType) On 2016/03/16 01:51:10, Yosi_UTC9 wrote: > On 2016/03/16 at 01:46:51, Yosi_UTC9 wrote: > > On 2016/03/15 at 19:19:10, chongz wrote: > > > Cannot do |InputType m_inputType| because I need to create InputEvent from > |commandName|, or should I enum all the possible command names? > > > > |inputType| doesn't hold a command name rather it holds a type of input the > spec. So, we can define it as |enum class|. > > |inputType| is still in discussion, https://github.com/w3c/editing/issues/110 Ok, I also found a list here and I'm not sure which one should I follow. https://github.com/w3c/editing/issues/79 Maybe I'll start with "insertText", "replaceContent", "deleteContent", "Undo", "Redo" first? To map a command to an InputType do you want me to 1. Set Fire/DoNotFire in EditorCommand like now and build a commandName-to-InputType map in InputEvent.cpp? Or 2. Just store InputType in EditorCommand to replace Fire/DoNotFire? Thanks!
https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1545: { "MoveBackward", 53, executeMoveBackward, supportedFromMenuOrKeyBinding, enabledInEditableTextOrCaretBrowsing, stateNone, valueNull, notTextInsertion, doNotFireInputEvent, doNotAllowExecutionWhenDisabled }, On 2016/03/16 at 21:51:44, chongz wrote: > On 2016/03/16 01:46:51, Yosi_UTC9 wrote: > > On 2016/03/15 at 19:19:10, chongz wrote: > > > Currently set |doNotFireInputEvent| to commands "Move*", "Print", "Scroll*", > > "Select*", is there any other commands that shouldn't fire input events? > > > > Entries having |shouldFireInputEvent| should have > > |supportedFromMenuOrKeyBinding|. > > We don't need to fire "beforeInput" from |document.execCommand()|. > > > > I'm not so sure if it's necessary, I guess we can just filter the source in |Editor::Command::execute|? > e.g. "InsertText" is |supported| and we want to fire "beforeinput" if it's from keybinding right? Yes. We can check Editor::Command::m_source in execute(). https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:13: String InputTypeToString(InputEvent::InputType inputType) On 2016/03/16 at 21:51:44, chongz wrote: > On 2016/03/16 01:51:10, Yosi_UTC9 wrote: > > On 2016/03/16 at 01:46:51, Yosi_UTC9 wrote: > > > On 2016/03/15 at 19:19:10, chongz wrote: > > > > Cannot do |InputType m_inputType| because I need to create InputEvent from > > |commandName|, or should I enum all the possible command names? > > > > > > |inputType| doesn't hold a command name rather it holds a type of input the > > spec. So, we can define it as |enum class|. > > > > |inputType| is still in discussion, https://github.com/w3c/editing/issues/110 > > Ok, I also found a list here and I'm not sure which one should I follow. > https://github.com/w3c/editing/issues/79 > > Maybe I'll start with "insertText", "replaceContent", "deleteContent", "Undo", "Redo" first? Make sense. Start with well defined one then expand in future. > To map a command to an InputType do you want me to > 1. Set Fire/DoNotFire in EditorCommand like now and build a commandName-to-InputType map in InputEvent.cpp? > Or 2. Just store InputType in EditorCommand to replace Fire/DoNotFire? Let's use |enum class InputType|, then having string representation in |InputEvent| constructor. See http://crbug.com/251275, Using strings for actions is error prone Pseudo code can be: static const char* const kInputTypeStrings[] = { "insertText", "replaceText", ... }; const auto& it = std::begin(kInputTypeStrings) + static_cast<size_t>(inputType); if (it < std::begin(kInputTypeStrings) || it <= std::end(kInputString)) { ASSERT_NOT_REACHED(); return ""; } return *it;
https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:13: String InputTypeToString(InputEvent::InputType inputType) On 2016/03/17 at 01:52:23, Yosi_UTC9 wrote: > On 2016/03/16 at 21:51:44, chongz wrote: > > On 2016/03/16 01:51:10, Yosi_UTC9 wrote: > > > On 2016/03/16 at 01:46:51, Yosi_UTC9 wrote: > > > > On 2016/03/15 at 19:19:10, chongz wrote: > > > > > Cannot do |InputType m_inputType| because I need to create InputEvent from > > > |commandName|, or should I enum all the possible command names? > > > > > > > > |inputType| doesn't hold a command name rather it holds a type of input the > > > spec. So, we can define it as |enum class|. > > > > > > |inputType| is still in discussion, https://github.com/w3c/editing/issues/110 > > > > Ok, I also found a list here and I'm not sure which one should I follow. > > https://github.com/w3c/editing/issues/79 > > > > Maybe I'll start with "insertText", "replaceContent", "deleteContent", "Undo", "Redo" first? > Make sense. Start with well defined one then expand in future. > > > To map a command to an InputType do you want me to > > 1. Set Fire/DoNotFire in EditorCommand like now and build a commandName-to-InputType map in InputEvent.cpp? > > Or 2. Just store InputType in EditorCommand to replace Fire/DoNotFire? > > Let's use |enum class InputType|, then having string representation in |InputEvent| constructor. > See http://crbug.com/251275, Using strings for actions is error prone > > Pseudo code can be: > > > static const char* const kInputTypeStrings[] = { "insertText", "replaceText", ... }; > const auto& it = std::begin(kInputTypeStrings) + static_cast<size_t>(inputType); > if (it < std::begin(kInputTypeStrings) || it <= std::end(kInputString)) { > ASSERT_NOT_REACHED(); > return ""; > } > return *it; Using strings for code logic is definitely bad and the bug you point to is really bad! But that's not what this code is doing, right? The problem with having the string representation in the InputEvent constructor is that it's generated from IDL and you'd now need a custom constructor. This switch is *only* used to fill a string field in the input argument to the constructor, so I don't think that's problematic. It just happens to be that the constructor takes a dictionary argument instead of a string directly. That said, I'm missing why we need an enum or separate string map at all. Can we use the command name from the CommandMap in EditorCommand? I think we should make the command names in EditorCommand match the ones we need to expose to web authors, otherwise we have to maintain two maps, which is error-prone. As far as firing or not firing, isn't that already controlled by the shouldFireInputEvent value in the CommandMap? In every case we would fire the input event, we should also be firing the beforeInput event (the opposite isn't always true because the author can preventDefault).
https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:13: String InputTypeToString(InputEvent::InputType inputType) On 2016/03/17 at 18:12:59, ojan wrote: > On 2016/03/17 at 01:52:23, Yosi_UTC9 wrote: > > On 2016/03/16 at 21:51:44, chongz wrote: > > > On 2016/03/16 01:51:10, Yosi_UTC9 wrote: > > > > On 2016/03/16 at 01:46:51, Yosi_UTC9 wrote: > > > > > On 2016/03/15 at 19:19:10, chongz wrote: > > > > > > Cannot do |InputType m_inputType| because I need to create InputEvent from > > > > |commandName|, or should I enum all the possible command names? > > > > > > > > > > |inputType| doesn't hold a command name rather it holds a type of input the > > > > spec. So, we can define it as |enum class|. > > > > > > > > |inputType| is still in discussion, https://github.com/w3c/editing/issues/110 > > > > > > Ok, I also found a list here and I'm not sure which one should I follow. > > > https://github.com/w3c/editing/issues/79 > > > > > > Maybe I'll start with "insertText", "replaceContent", "deleteContent", "Undo", "Redo" first? > > Make sense. Start with well defined one then expand in future. > > > > > To map a command to an InputType do you want me to > > > 1. Set Fire/DoNotFire in EditorCommand like now and build a commandName-to-InputType map in InputEvent.cpp? > > > Or 2. Just store InputType in EditorCommand to replace Fire/DoNotFire? > > > > Let's use |enum class InputType|, then having string representation in |InputEvent| constructor. > > See http://crbug.com/251275, Using strings for actions is error prone > > > > Pseudo code can be: > > > > > > static const char* const kInputTypeStrings[] = { "insertText", "replaceText", ... }; > > const auto& it = std::begin(kInputTypeStrings) + static_cast<size_t>(inputType); > > if (it < std::begin(kInputTypeStrings) || it <= std::end(kInputString)) { > > ASSERT_NOT_REACHED(); > > return ""; > > } > > return *it; > > Using strings for code logic is definitely bad and the bug you point to is really bad! But that's not what this code is doing, right? The problem with having the string representation in the InputEvent constructor is that it's generated from IDL and you'd now need a custom constructor. This switch is *only* used to fill a string field in the input argument to the constructor, so I don't think that's problematic. It just happens to be that the constructor takes a dictionary argument instead of a string directly. > > That said, I'm missing why we need an enum or separate string map at all. Can we use the command name from the CommandMap in EditorCommand? I think we should make the command names in EditorCommand match the ones we need to expose to web authors, otherwise we have to maintain two maps, which is error-prone. > > As far as firing or not firing, isn't that already controlled by the shouldFireInputEvent value in the CommandMap? In every case we would fire the input event, we should also be firing the beforeInput event (the opposite isn't always true because the author can preventDefault). Oh, I see, you added shouldFireInputEvent. For now you have this only dispatching beforeInput, do you plan to change the input event firing to use this logic as well? I think it's important that the two use the same code so we don't have subtle bugs of cases where they don't match.
On 2016/03/17 18:21:10, ojan wrote: > https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/events/InputEvent.cpp (right): > > https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/events/InputEvent.cpp:13: String > InputTypeToString(InputEvent::InputType inputType) > On 2016/03/17 at 18:12:59, ojan wrote: > > On 2016/03/17 at 01:52:23, Yosi_UTC9 wrote: > > > On 2016/03/16 at 21:51:44, chongz wrote: > > > > On 2016/03/16 01:51:10, Yosi_UTC9 wrote: > > > > > On 2016/03/16 at 01:46:51, Yosi_UTC9 wrote: > > > > > > On 2016/03/15 at 19:19:10, chongz wrote: > > > > > > > Cannot do |InputType m_inputType| because I need to create > InputEvent from > > > > > |commandName|, or should I enum all the possible command names? > > > > > > > > > > > > |inputType| doesn't hold a command name rather it holds a type of > input the > > > > > spec. So, we can define it as |enum class|. > > > > > > > > > > |inputType| is still in discussion, > https://github.com/w3c/editing/issues/110 > > > > > > > > Ok, I also found a list here and I'm not sure which one should I follow. > > > > https://github.com/w3c/editing/issues/79 > > > > > > > > Maybe I'll start with "insertText", "replaceContent", "deleteContent", > "Undo", "Redo" first? > > > Make sense. Start with well defined one then expand in future. > > > > > > > To map a command to an InputType do you want me to > > > > 1. Set Fire/DoNotFire in EditorCommand like now and build a > commandName-to-InputType map in InputEvent.cpp? > > > > Or 2. Just store InputType in EditorCommand to replace Fire/DoNotFire? > > > > > > Let's use |enum class InputType|, then having string representation in > |InputEvent| constructor. > > > See http://crbug.com/251275, Using strings for actions is error prone > > > > > > Pseudo code can be: > > > > > > > > > static const char* const kInputTypeStrings[] = { "insertText", > "replaceText", ... }; > > > const auto& it = std::begin(kInputTypeStrings) + > static_cast<size_t>(inputType); > > > if (it < std::begin(kInputTypeStrings) || it <= std::end(kInputString)) { > > > ASSERT_NOT_REACHED(); > > > return ""; > > > } > > > return *it; > > > > Using strings for code logic is definitely bad and the bug you point to is > really bad! But that's not what this code is doing, right? The problem with > having the string representation in the InputEvent constructor is that it's > generated from IDL and you'd now need a custom constructor. This switch is > *only* used to fill a string field in the input argument to the constructor, so > I don't think that's problematic. It just happens to be that the constructor > takes a dictionary argument instead of a string directly. > > > > That said, I'm missing why we need an enum or separate string map at all. Can > we use the command name from the CommandMap in EditorCommand? I think we should > make the command names in EditorCommand match the ones we need to expose to web > authors, otherwise we have to maintain two maps, which is error-prone. > > > > As far as firing or not firing, isn't that already controlled by the > shouldFireInputEvent value in the CommandMap? In every case we would fire the > input event, we should also be firing the beforeInput event (the opposite isn't > always true because the author can preventDefault). > > Oh, I see, you added shouldFireInputEvent. For now you have this only > dispatching beforeInput, do you plan to change the input event firing to use > this logic as well? I think it's important that the two use the same code so we > don't have subtle bugs of cases where they don't match. I think Yosi has some good points about using an enumeration. I've asked Chong to make a patch for introducing the enumeration in the web/* space. Since ideally the original problem is the editing commands being passed into the APIs as WebStrings. Using an enumeration will make this code much simpler because the shouldFireInputEvent can really just be a case statement based on the type of the event. In fact a number of function pointers in this Editor class would collapse the same with a little bit of cleanup. But nonetheless Chong is going to make an eumeration that current matches the "idForUserMetrics" field (since ultimately that is an enum field hand coded).
On 2016/03/17 18:26:33, dtapuska wrote: > On 2016/03/17 18:21:10, ojan wrote: > > > https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/events/InputEvent.cpp (right): > > > > > https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/events/InputEvent.cpp:13: String > > InputTypeToString(InputEvent::InputType inputType) > > On 2016/03/17 at 18:12:59, ojan wrote: > > > On 2016/03/17 at 01:52:23, Yosi_UTC9 wrote: > > > > On 2016/03/16 at 21:51:44, chongz wrote: > > > > > On 2016/03/16 01:51:10, Yosi_UTC9 wrote: > > > > > > On 2016/03/16 at 01:46:51, Yosi_UTC9 wrote: > > > > > > > On 2016/03/15 at 19:19:10, chongz wrote: > > > > > > > > Cannot do |InputType m_inputType| because I need to create > > InputEvent from > > > > > > |commandName|, or should I enum all the possible command names? > > > > > > > > > > > > > > |inputType| doesn't hold a command name rather it holds a type of > > input the > > > > > > spec. So, we can define it as |enum class|. > > > > > > > > > > > > |inputType| is still in discussion, > > https://github.com/w3c/editing/issues/110 > > > > > > > > > > Ok, I also found a list here and I'm not sure which one should I follow. > > > > > > https://github.com/w3c/editing/issues/79 > > > > > > > > > > Maybe I'll start with "insertText", "replaceContent", "deleteContent", > > "Undo", "Redo" first? > > > > Make sense. Start with well defined one then expand in future. > > > > > > > > > To map a command to an InputType do you want me to > > > > > 1. Set Fire/DoNotFire in EditorCommand like now and build a > > commandName-to-InputType map in InputEvent.cpp? > > > > > Or 2. Just store InputType in EditorCommand to replace Fire/DoNotFire? > > > > > > > > Let's use |enum class InputType|, then having string representation in > > |InputEvent| constructor. > > > > See http://crbug.com/251275, Using strings for actions is error prone > > > > > > > > Pseudo code can be: > > > > > > > > > > > > static const char* const kInputTypeStrings[] = { "insertText", > > "replaceText", ... }; > > > > const auto& it = std::begin(kInputTypeStrings) + > > static_cast<size_t>(inputType); > > > > if (it < std::begin(kInputTypeStrings) || it <= std::end(kInputString)) { > > > > ASSERT_NOT_REACHED(); > > > > return ""; > > > > } > > > > return *it; > > > > > > Using strings for code logic is definitely bad and the bug you point to is > > really bad! But that's not what this code is doing, right? The problem with > > having the string representation in the InputEvent constructor is that it's > > generated from IDL and you'd now need a custom constructor. This switch is > > *only* used to fill a string field in the input argument to the constructor, > so > > I don't think that's problematic. It just happens to be that the constructor > > takes a dictionary argument instead of a string directly. > > > > > > That said, I'm missing why we need an enum or separate string map at all. > Can > > we use the command name from the CommandMap in EditorCommand? I think we > should > > make the command names in EditorCommand match the ones we need to expose to > web > > authors, otherwise we have to maintain two maps, which is error-prone. > > > > > > As far as firing or not firing, isn't that already controlled by the > > shouldFireInputEvent value in the CommandMap? In every case we would fire the > > input event, we should also be firing the beforeInput event (the opposite > isn't > > always true because the author can preventDefault). > > > > Oh, I see, you added shouldFireInputEvent. For now you have this only > > dispatching beforeInput, do you plan to change the input event firing to use > > this logic as well? I think it's important that the two use the same code so > we > > don't have subtle bugs of cases where they don't match. > > I think Yosi has some good points about using an enumeration. I've asked Chong > to make a patch for introducing the enumeration in the web/* space. Since > ideally the original problem is the editing commands being passed into the APIs > as WebStrings. Using an enumeration will make this code much simpler because the > shouldFireInputEvent can really just be a case statement based on the type of > the event. In fact a number of function pointers in this Editor class would > collapse the same with a little bit of cleanup. But nonetheless Chong is going > to make an eumeration that current matches the "idForUserMetrics" field (since > ultimately that is an enum field hand coded). Err; I should indicate he's doing this as a standalone patch that this patch will become dependent on.
I see. That's fine w me. Please loop in esprehn on discussons around things in web. He's cleaning up that layer as part of the Onion Soup effort, so will likely have opinions on what the right API surface to expose and where the code should live.
Hi yosin@, I've rebased on the new |WebEditingCommandType| enum and updated as per your comments in last patch. PTAL, thanks! https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:296: // If text is empty, then delete the old composition here. If text is non-empty, InsertTextCommand::input On 2016/03/16 21:51:44, chongz wrote: > On 2016/03/16 01:46:51, Yosi_UTC9 wrote: > > We also dispatch "beforeInput" with "deleteComposedCharacter" in > > |InputMethodController::extendSelectionAndDelete()|. > > Note: Blink doesn't have information about "forward delete" or "backward > > delete". > > Acknowledged. Done. But I tried several keyboard layouts and Chinese IME and cannot figure out when will |InputMethodController::extendSelectionAndDelete()| get called? https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:1783: if (m_command->shouldFireInputEvent) { On 2016/03/16 21:51:44, chongz wrote: > On 2016/03/16 01:46:51, Yosi_UTC9 wrote: > > We should check this command is invoked by user action, e.g. |m_source == > > CommandFromMenuOrKeyBinding|. > > Acknowledged. Done. https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:13: String InputTypeToString(InputEvent::InputType inputType) On 2016/03/17 18:21:10, ojan wrote: > On 2016/03/17 at 18:12:59, ojan wrote: > > On 2016/03/17 at 01:52:23, Yosi_UTC9 wrote: > > > On 2016/03/16 at 21:51:44, chongz wrote: > > > > On 2016/03/16 01:51:10, Yosi_UTC9 wrote: > > > > > On 2016/03/16 at 01:46:51, Yosi_UTC9 wrote: > > > > > > On 2016/03/15 at 19:19:10, chongz wrote: > > > > > > > Cannot do |InputType m_inputType| because I need to create > InputEvent from > > > > > |commandName|, or should I enum all the possible command names? > > > > > > > > > > > > |inputType| doesn't hold a command name rather it holds a type of > input the > > > > > spec. So, we can define it as |enum class|. > > > > > > > > > > |inputType| is still in discussion, > https://github.com/w3c/editing/issues/110 > > > > > > > > Ok, I also found a list here and I'm not sure which one should I follow. > > > > https://github.com/w3c/editing/issues/79 > > > > > > > > Maybe I'll start with "insertText", "replaceContent", "deleteContent", > "Undo", "Redo" first? > > > Make sense. Start with well defined one then expand in future. > > > > > > > To map a command to an InputType do you want me to > > > > 1. Set Fire/DoNotFire in EditorCommand like now and build a > commandName-to-InputType map in InputEvent.cpp? > > > > Or 2. Just store InputType in EditorCommand to replace Fire/DoNotFire? > > > > > > Let's use |enum class InputType|, then having string representation in > |InputEvent| constructor. > > > See http://crbug.com/251275, Using strings for actions is error prone > > > > > > Pseudo code can be: > > > > > > > > > static const char* const kInputTypeStrings[] = { "insertText", > "replaceText", ... }; > > > const auto& it = std::begin(kInputTypeStrings) + > static_cast<size_t>(inputType); > > > if (it < std::begin(kInputTypeStrings) || it <= std::end(kInputString)) { > > > ASSERT_NOT_REACHED(); > > > return ""; > > > } > > > return *it; > > > > Using strings for code logic is definitely bad and the bug you point to is > really bad! But that's not what this code is doing, right? The problem with > having the string representation in the InputEvent constructor is that it's > generated from IDL and you'd now need a custom constructor. This switch is > *only* used to fill a string field in the input argument to the constructor, so > I don't think that's problematic. It just happens to be that the constructor > takes a dictionary argument instead of a string directly. > > > > That said, I'm missing why we need an enum or separate string map at all. Can > we use the command name from the CommandMap in EditorCommand? I think we should > make the command names in EditorCommand match the ones we need to expose to web > authors, otherwise we have to maintain two maps, which is error-prone. > > > > As far as firing or not firing, isn't that already controlled by the > shouldFireInputEvent value in the CommandMap? In every case we would fire the > input event, we should also be firing the beforeInput event (the opposite isn't > always true because the author can preventDefault). > > Oh, I see, you added shouldFireInputEvent. For now you have this only > dispatching beforeInput, do you plan to change the input event firing to use > this logic as well? I think it's important that the two use the same code so we > don't have subtle bugs of cases where they don't match. Done. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:471: dispatchBeforeInputFromComposition(frame().document()->focusedElement(), InputEvent::InputType::DeleteComposedCharacter, "backward"); Not sure when will this code path get called...? https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:34: return String(it->stringName); Should I construct a local String table from |kInputTypeStringNameMap| so we don't need to allocate every time? Or is there any cleaner way to do it? https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:70: inputEventInit.setInputType(InputTypeToString(inputType)); I have to convert |inputType| to String here and then convert back to |inputType| in |InputEvent::InputEvent(const AtomicString& type, const InputEventInit& initializer)| since |InputEventInit.inputType| is String. Adding another constructor that takes individual arguments instead of |InputEventInit| looks ugly... Is there a better way to fix it? https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.h (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.h:55: InputType m_inputType; Changed to enum.
https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html:12: var lastData = ""; Please use single-quote in script fragment since other parts of this script use single-quote. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html:14: txt.addEventListener('beforeinput', function(e) { nit: Please avoid to use single-letter variable name. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html:22: assert_equals(lastData, data, "${modifiers.toString()}+${key} should produce data: ${data}"); I think you mean `${modifiers.toString()}....`. Test harness's template string can't use local variables. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html:35: }, "Testing beforeinput data"); Please use single-quote in script fragment since other parts of this script use single-quote. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-inputtype.html (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-inputtype.html:12: var lastInputType = ""; Please use single-quote in script fragment since other parts of this script use single-quote. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-inputtype.html:14: txt.addEventListener('beforeinput', function(e) { nit: Please avoid to use single-letter variable name. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-inputtype.html:47: }, "Testing beforeinput inputType"); Please use single-quote in script fragment since other parts of this script use single-quote. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-order-typing-command.html (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-order-typing-command.html:22: txt.addEventListener(eventType, function(e) { nit: Please avoid to use single-letter variable name. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-order-typing-command.html:38: }, "Testing beforeinput order on typing and command"); Please use single-quote in script fragment since other parts of this script use single-quote. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:1736: if (RuntimeEnabledFeatures::inputEventEnabled()) { Could you use early return style to reduce indentation and diff when we get rid of runtime flag. # We just remove this if-statement when we get rid of runtime flag. if (!RuntimeEnabledFeatures::inputEventEnabled()) return DispatchEventResult::NotCanceled; if (!target) return DispatchEventResult::NotCanceled; InputEvent* beforeInputEvent = InputEvent::createBeforeInputTyping(InputEvent::InputType::InsertText, data); return target->dispatchEvent(beforeInputEvent); https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:471: dispatchBeforeInputFromComposition(frame().document()->focusedElement(), InputEvent::InputType::DeleteComposedCharacter, "backward"); On 2016/04/13 at 00:34:28, chongz wrote: > Not sure when will this code path get called...? Backspace emulation? Or, Thai keyboard? I remember that do-while loop introduced for deleting character in complex script. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:34: return String(it->stringName); On 2016/04/13 at 00:34:28, chongz wrote: > Should I construct a local String table from |kInputTypeStringNameMap| so we don't need to allocate every time? Or is there any cleaner way to do it? How about using AtomicString? https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:35: return ""; s/""/emptyString()/ https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:38: InputEvent::InputType InputTypeFromString(const String& stringName) s/InputTypeFromString/inputTypeFromString/ or |convertInputTypeToString()| Functions in Blink should start with lower case letter.
Description was changed from ========== Fire 'beforeinput' during typing, pressing hot keys and IME composition Support |inputType| and |data| field. 'beforeinput' will be fired when: 1. Typing without IME 2. Pressing hot keys like Ctrl-C/V 3. Typing with IME, will be fire before compositionUpdate 'beforeinput' will not be fired by Document.execCommand(). SPEC=http://w3c.github.io/editing/input-events.html#event-type-beforeinput BUG=585875 ========== to ========== Fire 'beforeinput' during typing, pressing hot keys and IME composition Support |inputType| and |data| field. 'beforeinput' will be fired when: 1. Typing without IME 2. Pressing hot keys like Ctrl-Z 3. Typing with IME, will be fire before compositionUpdate 'beforeinput' will not be fired by Document.execCommand(). SPEC=http://w3c.github.io/editing/input-events.html#event-type-beforeinput BUG=585875 ==========
Hi Yosin, PTAL, thanks! https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html:12: var lastData = ""; On 2016/04/13 06:13:08, Yosi_UTC9 wrote: > Please use single-quote in script fragment since other parts of this script use > single-quote. Done. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html:14: txt.addEventListener('beforeinput', function(e) { On 2016/04/13 06:13:08, Yosi_UTC9 wrote: > nit: Please avoid to use single-letter variable name. Done. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html:22: assert_equals(lastData, data, "${modifiers.toString()}+${key} should produce data: ${data}"); On 2016/04/13 06:13:08, Yosi_UTC9 wrote: > I think you mean `${modifiers.toString()}....`. > Test harness's template string can't use local variables. Done. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html:35: }, "Testing beforeinput data"); On 2016/04/13 06:13:08, Yosi_UTC9 wrote: > Please use single-quote in script fragment since other parts of this script use > single-quote. Done. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-inputtype.html (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-inputtype.html:12: var lastInputType = ""; On 2016/04/13 06:13:08, Yosi_UTC9 wrote: > Please use single-quote in script fragment since other parts of this script use > single-quote. Done. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-inputtype.html:14: txt.addEventListener('beforeinput', function(e) { On 2016/04/13 06:13:09, Yosi_UTC9 wrote: > nit: Please avoid to use single-letter variable name. Done. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-inputtype.html:47: }, "Testing beforeinput inputType"); On 2016/04/13 06:13:09, Yosi_UTC9 wrote: > Please use single-quote in script fragment since other parts of this script use > single-quote. Done. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-order-typing-command.html (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-order-typing-command.html:22: txt.addEventListener(eventType, function(e) { On 2016/04/13 06:13:09, Yosi_UTC9 wrote: > nit: Please avoid to use single-letter variable name. Done. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-order-typing-command.html:38: }, "Testing beforeinput order on typing and command"); On 2016/04/13 06:13:09, Yosi_UTC9 wrote: > Please use single-quote in script fragment since other parts of this script use > single-quote. Done. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/EditingUtilities.cpp (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/EditingUtilities.cpp:1736: if (RuntimeEnabledFeatures::inputEventEnabled()) { On 2016/04/13 06:13:09, Yosi_UTC9 wrote: > Could you use early return style to reduce indentation and diff when we get rid > of runtime flag. > > # We just remove this if-statement when we get rid of runtime flag. > if (!RuntimeEnabledFeatures::inputEventEnabled()) > return DispatchEventResult::NotCanceled; > if (!target) > return DispatchEventResult::NotCanceled; > InputEvent* beforeInputEvent = > InputEvent::createBeforeInputTyping(InputEvent::InputType::InsertText, data); > return target->dispatchEvent(beforeInputEvent); Done. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:471: dispatchBeforeInputFromComposition(frame().document()->focusedElement(), InputEvent::InputType::DeleteComposedCharacter, "backward"); On 2016/04/13 06:13:09, Yosi_UTC9 wrote: > On 2016/04/13 at 00:34:28, chongz wrote: > > Not sure when will this code path get called...? > > Backspace emulation? Or, Thai keyboard? > I remember that do-while loop introduced for deleting character in complex > script. Actually it will always go into here by pressing backspace on Android's soft keyboard (outside composition), so I think the event should be 'deleteContent'. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:34: return String(it->stringName); On 2016/04/13 06:13:09, Yosi_UTC9 wrote: > On 2016/04/13 at 00:34:28, chongz wrote: > > Should I construct a local String table from |kInputTypeStringNameMap| so we > don't need to allocate every time? Or is there any cleaner way to do it? > > How about using AtomicString? Done. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:35: return ""; On 2016/04/13 06:13:09, Yosi_UTC9 wrote: > s/""/emptyString()/ Done. https://codereview.chromium.org/1752933002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:38: InputEvent::InputType InputTypeFromString(const String& stringName) On 2016/04/13 06:13:09, Yosi_UTC9 wrote: > s/InputTypeFromString/inputTypeFromString/ or |convertInputTypeToString()| > Functions in Blink should start with lower case letter. Done. https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:295: dispatchBeforeInputFromComposition(target, InputEvent::InputType::InsertText, text); Based on discussion on GitHub DeleteComposedCharacter should be fired here. Will figure out a way to do it.
lgtm Thanks so much!
Hi ojan@, can you take a look at this CL please? I still need an owner for "third_party/WebKit/Source/core/events/*", thanks!
Very close. Just a few more things (mostly adding TODOs). https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html:30: // Shift+foo doesn't work with eventSender, but actually |data| is correct according to manual tests. Make this a TODO to fix this? https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-order-typing-command.html (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-order-typing-command.html:16: // Pressing Ctrl+z, eventSender.keyDown won't send 'keydown' and 'keyup' for Ctrl. Make this a TODO? https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:175: // According to spec 'beforeinput' should not be sent along with CompositionEnd, so Nit: two spaces before CompositionEnd https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:177: // https://w3c.github.io/uievents/#h-events-composition-event-input-events I believe the point of this part of the spec is that we currently have bugs where we modify the content after compositionEnd. We should fix those bugs and then we don't need to dispatch the event. "Since there are no DOM updates associated with the compositionend event, beforeinput and input events should not be sent at that time." The insertTextForConfirmedComposition line is actually a violation of the spec. We either need to fix this or get the spec changed. I think we had decided in January that if Chrome is able to change our behavior here that would be preferable. Gary might be able to confirm. In either case, for the purposes of this patch, I'm OK with this as is, but you should file and bug to fix this and definitely have a TODO here. https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:471: // TODO(chongz): According to spec |data| should be "forward" or "backward". Shouldn't data be the text that's going to be deleted? https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:138: return InputEvent::InputType::InsertText; I think InsertText and InsertHTML should be two separate values. All of the above are InserHTML except InsertBacktab and InsertText. Although, I'm not sure exactly what InsertLineBreak, InsertNewline and InsertNewlineInQuotedContent (i.e. whether they insert HTML or just raw text). https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:24: {InputEvent::InputType::Redo, "redo"}, We will need the spec to make sure to list all these cases if it doesn't already. That doesn't need to block committing the code of course. https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:42: if (codePointCompareIgnoringASCIICase(stringName, entry.stringName) == 0) Why do we need to ignore case? https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:58: m_inputType = convertStringToInputType(initializer.inputType()); TODO: ojan https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:70: inputEventInit.setInputType(convertInputTypeToString(inputType)); It seems pretty silly for us to convert the enum to a string just to immediately convert it back to an enum in the create call. But, I think for now we're stuck with it. Ideally the dictionary processing code would be out in the bindings so that the code in core can just use the enums. In either case, I don't think that should block this patch. Can you add a TODO though and I'll start a thread with the architecture team? https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:90: InputEvent* InputEvent::createBeforeInputEditorCommand(InputType inputType, const String& data) This looks the same as createBeforeInputTyping. Can they be one function? Even better, make a single function for all three of these that takes an enum that controls whether the event is cancelable since that's the only thing that's different between the three. https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.idl (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.idl:12: readonly attribute DOMString data; Nit: We don't usually put more than one space between "readonly" and "attribute".
Hi ojan, I've updated as per your comments, PTAL, thanks! https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-data.html:30: // Shift+foo doesn't work with eventSender, but actually |data| is correct according to manual tests. On 2016/04/16 00:11:45, ojan wrote: > Make this a TODO to fix this? Done. https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-order-typing-command.html (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/before-input-order-typing-command.html:16: // Pressing Ctrl+z, eventSender.keyDown won't send 'keydown' and 'keyup' for Ctrl. On 2016/04/16 00:11:45, ojan wrote: > Make this a TODO? Actually I realized this should be the correct behavior since it's sending a single key 'z' with modifier 'ctrl'. I can add methods to send separate 'keydown'/'keyup' for 'ctrl' but I'm not sure it's worth it for the purpose of this test. https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:175: // According to spec 'beforeinput' should not be sent along with CompositionEnd, so On 2016/04/16 00:11:45, ojan wrote: > Nit: two spaces before CompositionEnd Done. https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:177: // https://w3c.github.io/uievents/#h-events-composition-event-input-events On 2016/04/16 00:11:45, ojan wrote: > I believe the point of this part of the spec is that we currently have bugs > where we modify the content after compositionEnd. We should fix those bugs and > then we don't need to dispatch the event. > > "Since there are no DOM updates associated with the compositionend event, > beforeinput and input events should not be sent at that time." > > The insertTextForConfirmedComposition line is actually a violation of the spec. > We either need to fix this or get the spec changed. I think we had decided in > January that if Chrome is able to change our behavior here that would be > preferable. Gary might be able to confirm. > > In either case, for the purposes of this patch, I'm OK with this as is, but you > should file and bug to fix this and definitely have a TODO here. Yes we shouldn't modify DOM after 'compositionend', will fix it. Tracking bug https://crbug.com/575294 https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:471: // TODO(chongz): According to spec |data| should be "forward" or "backward". On 2016/04/16 00:11:45, ojan wrote: > Shouldn't data be the text that's going to be deleted? Spec says: ``` 6. If the user indicated deletion of content that causes the caret to move using a keyboard, speech, or similar method, the inputType must be deleteContent 1. Set the value of data to be "forward" or "backward", depending on whether the caret would move forward or backward in connection with the deletion of content. ``` If that's actually a bug I can file one. http://w3c.github.io/editing/input-events.html#widl-InputEvent-inputType https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:138: return InputEvent::InputType::InsertText; On 2016/04/16 00:11:45, ojan wrote: > I think InsertText and InsertHTML should be two separate values. All of the > above are InserHTML except InsertBacktab and InsertText. Although, I'm not sure > exactly what InsertLineBreak, InsertNewline and InsertNewlineInQuotedContent > (i.e. whether they insert HTML or just raw text). The spec (http://w3c.github.io/editing/input-events.html) haven't mentioned |InsertHTML| yet... Will only support |InsertText| in this patch and doing some research about |InsertHTML|. https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:24: {InputEvent::InputType::Redo, "redo"}, On 2016/04/16 00:11:46, ojan wrote: > We will need the spec to make sure to list all these cases if it doesn't > already. That doesn't need to block committing the code of course. Posted question in https://github.com/w3c/editing/issues/79, will see if we could get a list. https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:42: if (codePointCompareIgnoringASCIICase(stringName, entry.stringName) == 0) On 2016/04/16 00:11:46, ojan wrote: > Why do we need to ignore case? I was trying to match the style of |execCommand()| where they ignore case, but yes it would probably be better to not do that for new APIs. https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:58: m_inputType = convertStringToInputType(initializer.inputType()); On 2016/04/16 00:11:45, ojan wrote: > TODO: ojan Done. https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:70: inputEventInit.setInputType(convertInputTypeToString(inputType)); On 2016/04/16 00:11:46, ojan wrote: > It seems pretty silly for us to convert the enum to a string just to immediately > convert it back to an enum in the create call. But, I think for now we're stuck > with it. Ideally the dictionary processing code would be out in the bindings so > that the code in core can just use the enums. In either case, I don't think that > should block this patch. Can you add a TODO though and I'll start a thread with > the architecture team? Done. https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.cpp:90: InputEvent* InputEvent::createBeforeInputEditorCommand(InputType inputType, const String& data) On 2016/04/16 00:11:46, ojan wrote: > This looks the same as createBeforeInputTyping. Can they be one function? Even > better, make a single function for all three of these that takes an enum that > controls whether the event is cancelable since that's the only thing that's > different between the three. Done. https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/events/InputEvent.idl (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/events/InputEvent.idl:12: readonly attribute DOMString data; On 2016/04/16 00:11:46, ojan wrote: > Nit: We don't usually put more than one space between "readonly" and > "attribute". Done.
https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/commands/EditorCommand.cpp:138: return InputEvent::InputType::InsertText; On 2016/04/18 22:54:47, chongz wrote: > On 2016/04/16 00:11:45, ojan wrote: > > I think InsertText and InsertHTML should be two separate values. All of the > > above are InserHTML except InsertBacktab and InsertText. Although, I'm not > sure > > exactly what InsertLineBreak, InsertNewline and InsertNewlineInQuotedContent > > (i.e. whether they insert HTML or just raw text). > > The spec (http://w3c.github.io/editing/input-events.html) haven't mentioned > |InsertHTML| yet... Will only support |InsertText| in this patch and doing some > research about |InsertHTML|. Actually according to https://github.com/w3c/editing/issues/124 |replaceContent| (or probably better rename to |insertContent|?) looks very similar to |insertHTML|, if they are the same thing I will add support in another CL.
lgtm https://codereview.chromium.org/1752933002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/events/InputEvent.cpp (right): https://codereview.chromium.org/1752933002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/events/InputEvent.cpp:71: inputEventInit.setCancelable(static_cast<bool>(cancelable)); Typically we avoid casting unless really necessary. More common pattern here would be setCancelable(cancelable == IsCancelable);
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org, ojan@chromium.org Link to the patchset: https://codereview.chromium.org/1752933002/#ps120001 (title: "Use 'foo == enum' instead of static_cast<>")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1752933002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752933002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org, ojan@chromium.org Link to the patchset: https://codereview.chromium.org/1752933002/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1752933002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1752933002/140001
Message was sent while issue was closed.
Description was changed from ========== Fire 'beforeinput' during typing, pressing hot keys and IME composition Support |inputType| and |data| field. 'beforeinput' will be fired when: 1. Typing without IME 2. Pressing hot keys like Ctrl-Z 3. Typing with IME, will be fire before compositionUpdate 'beforeinput' will not be fired by Document.execCommand(). SPEC=http://w3c.github.io/editing/input-events.html#event-type-beforeinput BUG=585875 ========== to ========== Fire 'beforeinput' during typing, pressing hot keys and IME composition Support |inputType| and |data| field. 'beforeinput' will be fired when: 1. Typing without IME 2. Pressing hot keys like Ctrl-Z 3. Typing with IME, will be fire before compositionUpdate 'beforeinput' will not be fired by Document.execCommand(). SPEC=http://w3c.github.io/editing/input-events.html#event-type-beforeinput BUG=585875 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/InputMethodController.cpp (right): https://codereview.chromium.org/1752933002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/InputMethodController.cpp:471: // TODO(chongz): According to spec |data| should be "forward" or "backward". On 2016/04/18 at 22:54:47, chongz wrote: > On 2016/04/16 00:11:45, ojan wrote: > > Shouldn't data be the text that's going to be deleted? > > Spec says: > ``` > 6. If the user indicated deletion of content that causes the caret to move using a keyboard, speech, or similar method, the inputType must be deleteContent > 1. Set the value of data to be "forward" or "backward", depending on whether the caret would move forward or backward in connection with the deletion of content. > ``` > If that's actually a bug I can file one. > http://w3c.github.io/editing/input-events.html#widl-InputEvent-inputType Whoops. I forgot to respond to this. That behavior seems crazy to me. data should always be the thing getting inserted, not some magical field that contains arbitrary values. So, IMO, the forward/backward part of this spec is definitely something that should be changed. Instead of reusing the data field for this purpose, there should just be two different edit types (forwardDeleteContent and backwardDeleteContent or something like that). I'm also unsure about the replaceContent value of data being a documentFragment. It's weird for it to sometimes be a string and sometimes a documentFragment. But, maybe we should run that by more people on our team first. This might be a good thing to discuss with the architecture team (platform-architecture-dev@chromium), and then we file a bug on the spec.
Message was sent while issue was closed.
Description was changed from ========== Fire 'beforeinput' during typing, pressing hot keys and IME composition Support |inputType| and |data| field. 'beforeinput' will be fired when: 1. Typing without IME 2. Pressing hot keys like Ctrl-Z 3. Typing with IME, will be fire before compositionUpdate 'beforeinput' will not be fired by Document.execCommand(). SPEC=http://w3c.github.io/editing/input-events.html#event-type-beforeinput BUG=585875 ========== to ========== Fire 'beforeinput' during typing, pressing hot keys and IME composition Support |inputType| and |data| field. 'beforeinput' will be fired when: 1. Typing without IME 2. Pressing hot keys like Ctrl-Z 3. Typing with IME, will be fire before compositionUpdate 'beforeinput' will not be fired by Document.execCommand(). SPEC=http://w3c.github.io/editing/input-events.html#event-type-beforeinput BUG=585875 Committed: https://crrev.com/232c62972c919927922d08050749280f11ce4f2b Cr-Commit-Position: refs/heads/master@{#388519} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/232c62972c919927922d08050749280f11ce4f2b Cr-Commit-Position: refs/heads/master@{#388519}
Message was sent while issue was closed.
Description was changed from ========== Fire 'beforeinput' during typing, pressing hot keys and IME composition Support |inputType| and |data| field. 'beforeinput' will be fired when: 1. Typing without IME 2. Pressing hot keys like Ctrl-Z 3. Typing with IME, will be fire before compositionUpdate 'beforeinput' will not be fired by Document.execCommand(). SPEC=http://w3c.github.io/editing/input-events.html#event-type-beforeinput BUG=585875 Committed: https://crrev.com/232c62972c919927922d08050749280f11ce4f2b Cr-Commit-Position: refs/heads/master@{#388519} ========== to ========== Fire 'beforeinput' during typing, pressing hot keys and IME composition Support |inputType| and |data| field. 'beforeinput' will be fired when: 1. Typing without IME 2. Pressing hot keys like Ctrl-Z 3. Typing with IME, will be fire before compositionUpdate 'beforeinput' will not be fired by Document.execCommand(). SPEC=http://w3c.github.io/editing/input-events.html#event-type-beforeinput BUG=585875 Committed: https://crrev.com/232c62972c919927922d08050749280f11ce4f2b Cr-Commit-Position: refs/heads/master@{#388519} ========== |
