Bug 221782 – [iOS][FCR] Add new look for input type=range with datalist

• Component: Forms
• Status: RESOLVED FIXED

[Aditya Keerthi]

...

[Aditya Keerthi]

<rdar://problem/74251214>

[Aditya Keerthi]

Created attachment 420046 [details]
Patch

[Darin Adler]

Comment on attachment 420046 [details]
Patch

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

> Source/WebCore/rendering/RenderThemeIOS.mm:2305
> +    if (min == max)

Maybe >=?

> Source/WebCore/rendering/RenderThemeIOS.mm:2326
> +    auto& dataList = downcast<HTMLDataListElement>(*input.list());

Why aren’t we using the HTMLInputElement::dataList() function? This downcast is literally just undoing what the the list() function does.

Also seems like we should only do this once, not separately for the null check and then again for the local variable. A null check of a local variable would be nice, although it’s probably less elegant to have a pointer instead of a reference for the local.

> Source/WebCore/rendering/RenderThemeIOS.mm:2337
> +        auto optionValue = optionElement.value();
> +        if (!input.isValidValue(optionValue))
> +            continue;
> +
> +        auto parsedOptionValue = parseToDoubleForNumberType(input.sanitizeValue(optionValue));

It’s not great to have this code here in the theme. I’d suggest we create a function that returns an Optional<double> so this code doesn’t have to know things like to call isValidValue and sanitizeValue and which parse function to call.

The function could be a member of HTMLInputElement, or HTMLDataListElement, or HTMLOptionElement. Not sure where the cleanest responsibility would be. Could take a String or could take an HTMLOptionElement. Maybe the cleanest design would be an overload of the valueAsNumber function in HTMLInputElement that takes an HTMLOptionElement argument.

> Source/WebCore/rendering/RenderThemeIOS.mm:2346
> +        context.fillRoundedRect(roundedTickRect, (value >= parsedOptionValue) ? controlColor : controlBackgroundColor);

Does this need snapping to device pixel boundaries so it looks sharp? In the past that’s been required for some form control rendering.

[Aditya Keerthi]

Created attachment 420317 [details]
Patch

[Aditya Keerthi]

(In reply to Darin Adler from comment #3)
> Comment on attachment 420046 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420046&action=review
> 
> > Source/WebCore/rendering/RenderThemeIOS.mm:2305
> > +    if (min == max)
> 
> Maybe >=?

Done.
 
> > Source/WebCore/rendering/RenderThemeIOS.mm:2326
> > +    auto& dataList = downcast<HTMLDataListElement>(*input.list());
> 
> Why aren’t we using the HTMLInputElement::dataList() function? This downcast
> is literally just undoing what the the list() function does.

Updated to use HTMLInputElement::dataList(). I took this code from RenderTheme.cpp, so I've also updated the code there.
 
> Also seems like we should only do this once, not separately for the null
> check and then again for the local variable. A null check of a local
> variable would be nice, although it’s probably less elegant to have a
> pointer instead of a reference for the local.

Updated to null-check a local variable.
 
> > Source/WebCore/rendering/RenderThemeIOS.mm:2337
> > +        auto optionValue = optionElement.value();
> > +        if (!input.isValidValue(optionValue))
> > +            continue;
> > +
> > +        auto parsedOptionValue = parseToDoubleForNumberType(input.sanitizeValue(optionValue));
> 
> It’s not great to have this code here in the theme. I’d suggest we create a
> function that returns an Optional<double> so this code doesn’t have to know
> things like to call isValidValue and sanitizeValue and which parse function
> to call.
> 
> The function could be a member of HTMLInputElement, or HTMLDataListElement,
> or HTMLOptionElement. Not sure where the cleanest responsibility would be.
> Could take a String or could take an HTMLOptionElement. Maybe the cleanest
> design would be an overload of the valueAsNumber function in
> HTMLInputElement that takes an HTMLOptionElement argument.

I've added the listOptionValueAsDouble method to HTMLInputElement, which takes an HTMLOptionElement and returns an Optional<double>.

I didn't overload valueAsNumber, as that method is named for the corresponding JavaScript method on HTMLInputElement, and wanted to avoid confusion.

> > Source/WebCore/rendering/RenderThemeIOS.mm:2346
> > +        context.fillRoundedRect(roundedTickRect, (value >= parsedOptionValue) ? controlColor : controlBackgroundColor);
> 
> Does this need snapping to device pixel boundaries so it looks sharp? In the
> past that’s been required for some form control rendering.

I didn't observe any blurriness with the existing code, but I've updated the patch to snap to pixel boundaries to stay consistent with the rest of form control rendering.

[EWS]

Committed r272881: <https://commits.webkit.org/r272881>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420317 [details].