1. Go to whsieh.github.io/examples/input-events 2. Type some text and select it 3. Set foreground color - The data attribute of the input event should not be null, but it is!
<rdar://problem/29032759>
Created attachment 293472 [details] Patch
Created attachment 293485 [details] Rebased on ToT
Comment on attachment 293485 [details] Rebased on ToT View in context: https://bugs.webkit.org/attachment.cgi?id=293485&action=review > Source/WebCore/editing/ApplyStyleCommand.cpp:234 > +String ApplyStyleCommand::inputEventData() const > +{ > + if (editingAction() != EditActionSetColor) > + return CompositeEditCommand::inputEventData(); > + > + auto* styleProperties = m_style->style(); > + if (!styleProperties) > + return { }; > + > + return styleProperties->getPropertyValue(CSSPropertyColor); > +} This really needs to be done at the call site of ApplyStyleCommand. See how toggleStyle, etc... works in EditorCommands.cpp We need to set the input event's data there instead of trying to figure it out down in ApplyStyleCommand.
Created attachment 293508 [details] Patch
Comment on attachment 293508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293508&action=review > Source/WebCore/editing/Editor.cpp:794 > + auto* element = m_frame.selection().selection().rootEditableElement(); I think that likely this needs to be a RefPtr because this pointer is used after calling applyCommand, which I assume can run arbitrary JavaScript. > Source/WebCore/editing/Editor.cpp:800 > + if (selectionType == VisibleSelection::CaretSelection) > + computeAndSetTypingStyle(*style, editingAction); > + > + if (selectionType == VisibleSelection::RangeSelection) > applyCommand(ApplyStyleCommand::create(document(), style.get(), editingAction)); I think a switch statement would be better than two if statements. > Source/WebCore/editing/Editor.cpp:803 > + dispatchInputEvent(*element, inputTypeName, inputEventData); Are we sure it’s OK to dispatch this event before calling didApplyStyle, given the event could do additional editing before returning? I think that generally events need to be dispatched after all other code runs and before returning to JavaScript. We may want to come up with a system similar to the one used for custom elements, or share its system. Not just for this event, but many others. > Source/WebCore/editing/Editor.cpp:824 > + auto* element = m_frame.selection().selection().rootEditableElement(); Ditto. > Source/WebCore/editing/Editor.cpp:828 > + dispatchInputEvent(*element, inputTypeName); Ditto.
Comment on attachment 293508 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=293508&action=review >> Source/WebCore/editing/Editor.cpp:794 >> + auto* element = m_frame.selection().selection().rootEditableElement(); > > I think that likely this needs to be a RefPtr because this pointer is used after calling applyCommand, which I assume can run arbitrary JavaScript. Good catch -- fixed. >> Source/WebCore/editing/Editor.cpp:800 >> applyCommand(ApplyStyleCommand::create(document(), style.get(), editingAction)); > > I think a switch statement would be better than two if statements. Ok! Done. >> Source/WebCore/editing/Editor.cpp:803 >> + dispatchInputEvent(*element, inputTypeName, inputEventData); > > Are we sure it’s OK to dispatch this event before calling didApplyStyle, given the event could do additional editing before returning? I think that generally events need to be dispatched after all other code runs and before returning to JavaScript. We may want to come up with a system similar to the one used for custom elements, or share its system. Not just for this event, but many others. I see. It looks like we've always dispatched the input event and then called didApplyStyle, so this patch maintains the current behavior. However, didApplyStyle is a no-op in WK2, and in WK1, it just updates the font selection panel by calling into -[WebHTMLView _updateFontPanel]. I'll move the input event dispatch to after we invoke didApplyStyle.
Created attachment 294246 [details] Patch for landing
Comment on attachment 294246 [details] Patch for landing Clearing flags on attachment: 294246 Committed r208461: <http://trac.webkit.org/changeset/208461>