Bug 164241 – Setting foreground color when text is selected should fire an input event with color data

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Wenson Hsieh]

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!

[Wenson Hsieh]

<rdar://problem/29032759>

[Wenson Hsieh]

Created attachment 293472 [details]
Patch

[Wenson Hsieh]

Created attachment 293485 [details]
Rebased on ToT

[Ryosuke Niwa]

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.

[Wenson Hsieh]

Created attachment 293508 [details]
Patch

[Darin Adler]

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.

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 294246 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 294246 [details]
Patch for landing

Clearing flags on attachment: 294246

Committed r208461: <http://trac.webkit.org/changeset/208461>