Bug 120559 – [Forms:color] ColorInputType::didChooseColor should set sanitize value in element.

• Component: Forms
• Status: RESOLVED CONFIGURATION CHANGED

[Santosh Mahto]

When color is selected from ColorPicker , the ColorInputType::didChooseColor try to set original selected value in element.
Instead we should set sanitized value.

The following crash is observed when we set unsanitized color value having alpha component.

ASSERTION FAILED: value == sanitizeValue(value) || sanitizeValue(value).isEmpty()
/home/santosh/projects/WebKit/Source/WebCore/html/HTMLInputElement.cpp(1092) : void WebCore::HTMLInputElement::setValueFromRenderer(const WTF::String&)
1   0xb613c1f0 WTFCrash
2   0xb199df91 WebCore::HTMLInputElement::setValueFromRenderer(WTF::String const&)
3   0xb194dd12 WebCore::ColorInputType::didChooseColor(WebCore::Color const&)
4   0xb71d61f5 WebKit::WebColorChooser::didChooseColor(WebCore::Color const&)
5   0xb72179da WebKit::WebPage::didChooseColor(WebCore::Color const&)
6   0xb72d9bdc void CoreIPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(WebCore::Color const&), WebCore::Color>(CoreIPC::Arguments1<WebCore::Color> const&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebCore::Color const&))
7   0xb72d6dd7 void CoreIPC::handleMessage<Messages::WebPage::DidChooseColor, WebKit::WebPage, void (WebKit::WebPage::*)(WebCore::Color const&)>(CoreIPC::MessageDecoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(WebCore::Color const&))
8   0xb72d346d WebKit::WebPage::didReceiveWebPageMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
9   0xb721887d WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
10  0xb6fd00ce CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
11  0xb713ec7f WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageDecoder&)
12  0xb6fbe098 CoreIPC::Connection::dispatchMessage(CoreIPC::MessageDecoder&)
13  0xb6fbe166 CoreIPC::Connection::dispatchMessage(WTF::PassOwnPtr<CoreIPC::MessageDecoder>)
14  0xb6fbe335 CoreIPC::Connection::dispatchOneMessage()
15  0xb6fcf470 WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)
16  0xb6fcf02a WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void (CoreIPC::Connection*)>::operator()()
17  0xb71f441d WTF::Function<void ()>::operator()() const
18  0xb1d8e4ed WebCore::RunLoop::performWork()
19  0xb28c9dd4 WebCore::RunLoop::wakeUpEvent(void*, void*, unsigned int)

[Santosh Mahto]

Created attachment 210187 [details]
Patch

[Darin Adler]

Comment on attachment 210187 [details]
Patch

What effect does this have? Can we create a regression test demonstrating the problem this solves?

[Santosh Mahto]

(In reply to comment #2)
> (From update of attachment 210187 [details])
> What effect does this have? Can we create a regression test demonstrating the problem this solves?

Effect:
input type= color take color of "#rrggbb" format only (http://www.w3.org/TR/html-markup/datatypes.html#form.data.color).
if user selection has other format e.g including alpha value ("#rrggbbaa") then we should set the fallback value of color.
This sanitizing is done when we set the color value through js call.But not done 
when color is set by colorpicker.

I will try to add test cases for this.

[Santosh Mahto]

As the modified api is initiated by external picker, its difficult to generate a test for this as I could not find any test framwork in layout test.
any suggestion is welcomed.

[Andreas Kling]

So <input type=color> does not support colors with alpha values, and the solution here is to turn colors with alpha into solid black by "sanitizing" the value?

That seems counter-intuitive. Since colors with alpha values are not allowed by the API, it would be better if they could not be selected at all. If that's not possible due to platform color picker restrictions, I think we should just always force the alpha component to 1.0.

[Santosh Mahto]

(In reply to comment #5)
> So <input type=color> does not support colors with alpha values, and the solution here is to turn colors with alpha into solid black by "sanitizing" the value?
You are Right...

> That seems counter-intuitive. Since colors with alpha values are not allowed by the API, it would be better if they could not be selected at all. If that's not possible due to platform color picker restrictions, I think we should just always force the alpha component to 1.0.

But its not about only alpha value, its about invalid color selecting e.g "RED". and also lowercase translation,
 ie if we set value = '#RRGGBB' by javascript the value set will be sanitized to lowercase so value set will  be value = '#rrggbb' in string format.
input-value-sanitization-color.html:  shouldBe('input.value = "#DEF012"; input.value', '"#def012"');

But if we set '#RRGGBB' by color selector it will be same as '#RRGGBB' without sanitize.
 

Valid/Invalid color decision is done(and appropriate) in ColorInputType.cpp and is right place to to adjustment for wrong value.I feel we should not let other part of code to modify color based on what [Input type=Color] accepts.

JavaScript call can try to set any color value, but we do sanitize before adding in DOM node in HTMLInputElement::setValue().

Sanitizing Color convert any invalid color to fallback color(#) as input-value-sanitization-color.html test case specify.
//
// Invalid values
shouldBe('input.value = "000000"; input.value', fallbackValue);
shouldBe('input.value = "#FFF"; input.value', fallbackValue);
shouldBe('input.value = " #ffffff"; input.value', fallbackValue);
shouldBe('input.value = "#ffffff "; input.value', fallbackValue);
shouldBe('input.value = "#00112233"; input.value', fallbackValue);
shouldBe('input.value = "rgb(0,0,0)"; input.value', fallbackValue);

I think I should have returned if color is invalid, otherwise it will update the current color with fallback value(solid black color).

[Kent Tamura]

Comment on attachment 210187 [details]
Patch

The assumption is that a color picker implementation must pass a color without alpha.  It must not provide UI to select alpha channel. 
Lowercasing the serialized string makes sense.

[Ahmad Saleem]

I think we don't have SanitizeValue still as this patch was proposing:

https://github.com/WebKit/WebKit/blob/af43ca312063d6ce476c3eb23c3755919916001d/Source/WebCore/html/ColorInputType.cpp#L237

and Web-Specs are as follow:

https://html.spec.whatwg.org/multipage/input.html#color-state-(type=color)

"The value sanitization algorithm is as follows: If the value of the element is a valid simple color, then set it to the value of the element converted to ASCII lowercase; otherwise, set it to the string "#000000"."

Do we need to do anything, appreciate if someone can comment. Thanks!

[Ahmad Saleem]

Just to update - Safari pass all WPT tests (even Invalid cases) so I think we can close this:

https://wpt.fyi/results/html/semantics/forms/the-input-element/color.html?label=master&label=experimental&aligned&view=subtest&q=color%20input

[Brent Fulgham]

Sounds great, thank you for testing!