Bug 163348 – Add experimental support for the "formatForeColor" inputType

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

[Wenson Hsieh]

Add experimental support for the "formatForeColor" inputType

[Wenson Hsieh]

Created attachment 291379 [details]
Patch

[Wenson Hsieh]

<rdar://problem/28739334>

[Ryosuke Niwa]

Comment on attachment 291379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291379&action=review

> Source/WebCore/editing/Editor.cpp:145
> +        auto colorValue = properties->getPropertyCSSValue(CSSPropertyColor);
> +        ASSERT(colorValue);
> +        return colorValue->cssText();

Why don't we just call getPropertyValue which returns String?

> LayoutTests/fast/events/input-events-forecolor-data.html:19
> +            if (event.inputType === "formatForeColor")
> +                shouldBe("event.data", "'rgb(255, 255, 255)'");

I'm not certain the functional syntax is what we want to be using here
but I guess that'd be consistent with CSSOM: https://drafts.csswg.org/cssom/#serialize-a-css-component-value

[Ryosuke Niwa]

Comment on attachment 291379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291379&action=review

Sorry, a few more comments.

> LayoutTests/fast/events/input-events-forecolor-data-expected.txt:2
> +PASS event.data is 'rgb(255, 255, 255)'
> +PASS event.data is 'rgb(255, 255, 255)'

It's not clear why these results are correct.  At minimum we should print out the event name.

> LayoutTests/fast/events/input-events-forecolor-data.html:12
> +            document.getElementById("editable").focus();
> +            testRunner.execCommand("ForeColor", "rgb(255, 255, 255)");

You don't need to wrap this inside the if. execCommand('foreColor') always works and so does focus().

You should probably test more colors though.

[Wenson Hsieh]

Comment on attachment 291379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=291379&action=review

>> Source/WebCore/editing/Editor.cpp:145
>> +        return colorValue->cssText();
> 
> Why don't we just call getPropertyValue which returns String?

Good point! Changed to return getPropertyValue().

>> LayoutTests/fast/events/input-events-forecolor-data-expected.txt:2
>> +PASS event.data is 'rgb(255, 255, 255)'
> 
> It's not clear why these results are correct.  At minimum we should print out the event name.

Sounds good. I'll make it more clear by printing out the event type/inputType.

>> LayoutTests/fast/events/input-events-forecolor-data.html:12
>> +            testRunner.execCommand("ForeColor", "rgb(255, 255, 255)");
> 
> You don't need to wrap this inside the if. execCommand('foreColor') always works and so does focus().
> 
> You should probably test more colors though.

Ok, done! Also added more colors in this test.

>> LayoutTests/fast/events/input-events-forecolor-data.html:19
>> +                shouldBe("event.data", "'rgb(255, 255, 255)'");
> 
> I'm not certain the functional syntax is what we want to be using here
> but I guess that'd be consistent with CSSOM: https://drafts.csswg.org/cssom/#serialize-a-css-component-value

Good point -- let's keep this issue in mind.

[Wenson Hsieh]

Created attachment 291384 [details]
Patch for landing

[Build Bot]

Comment on attachment 291379 [details]
Patch

Attachment 291379 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2272193

New failing tests:
fast/events/input-events-forecolor-data.html

[Build Bot]

Created attachment 291386 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Wenson Hsieh]

Created attachment 291394 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 291394 [details]
Patch for landing

Clearing flags on attachment: 291394

Committed r207244: <http://trac.webkit.org/changeset/207244>

[Build Bot]

Comment on attachment 291379 [details]
Patch

Attachment 291379 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2272726

New failing tests:
fast/events/input-events-forecolor-data.html

[Build Bot]

Created attachment 291401 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5