Bug 216616 – [macOS] Update the appearance of editable date/time controls to match the system

• Component: Forms
• Status: RESOLVED FIXED

[Aditya Keerthi]

- Rounded corners on focused component
- Raised colon separator for time inputs
- Fixed width components, with a variable width for the control itself

[Aditya Keerthi]

<rdar://problem/69004603>

[Aditya Keerthi]

Created attachment 408943 [details]
Patch

[Aditya Keerthi]

Created attachment 409068 [details]
Patch

[Devin Rousso]

Comment on attachment 409068 [details]
Patch

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

> Source/WebCore/ChangeLog:25
> +        However, since we do not have support for nested-at rules in CSS, there is
> +        currently no way to workaround this problem. Instead, the issue should be
> +        fixed once we gain CSS support.

Wait what?  You can totally nest `@media`.  Or are you referring to the fact that you can't put `@media` inside a CSS rule?  How would an `@media` (or some other `@` rule) solve this problem?  Perhaps you could include the code here of what you would do if it was supported.

NIT: "nested at-rule"

> Source/WebCore/ChangeLog:27
> +        3. Components should have a fixed width, and the width of the control should match the content

Does this mean that the size of the overall `<input>` remains constant?  I'd hope so, as that's what all other `<input>` do IIRC where the `width` stays constant (unless overridden) and a horizontal scrollbar is shown.

> Source/WebCore/css/html.css:494
> +    /* FIXME: Font feature settings should be defined in a nested-at rule, so that the feature setting is only applied to the system font. */

NIT: "nested at-rule"

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:76
> +    unsigned length = 2;
> +    if (m_range.maximum > 999)
> +        length = 4;
> +    else if (m_range.maximum > 99)
> +        length = 3;

NIT: I wish there was an `std::countDigits` :(

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:82
> +        String number = locale.convertToLocalizedNumber(makeString(pad(c, length, String(&c, 1))));

Is the `makeString` necessary?  Also, perhaps you could use `makeString` instead of the `String(&c, 1)`.
```
    auto numberString = locale.convertToLocalizedNumber(pad(c, length, makeString(c)));
```

> Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:86
> +    elementStyle.renderStyle->setMinWidth(Length { width, Fixed });

NIT: is the `Length` necessary?

> Source/WebCore/html/shadow/DateTimeSymbolicFieldElement.cpp:73
> +    elementStyle.renderStyle->setMinWidth(Length { width, Fixed });

ditto (DateTimeNumericFieldElement.cpp:86)

[Aditya Keerthi]

Created attachment 409243 [details]
Patch

[Aditya Keerthi]

(In reply to Devin Rousso from comment #4)
> Comment on attachment 409068 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409068&action=review
> 
> > Source/WebCore/ChangeLog:25
> > +        However, since we do not have support for nested-at rules in CSS, there is
> > +        currently no way to workaround this problem. Instead, the issue should be
> > +        fixed once we gain CSS support.
> 
> Wait what?  You can totally nest `@media`.  Or are you referring to the fact
> that you can't put `@media` inside a CSS rule?  How would an `@media` (or
> some other `@` rule) solve this problem?  Perhaps you could include the code
> here of what you would do if it was supported.

I was referring to the @font-feature-values rule (see example here: https://developer.mozilla.org/en-US/docs/Web/CSS/@font-feature-values). Updated the Changelog and FIXME to be more specific.

> > Source/WebCore/ChangeLog:27
> > +        3. Components should have a fixed width, and the width of the control should match the content
> 
> Does this mean that the size of the overall `<input>` remains constant?  I'd
> hope so, as that's what all other `<input>` do IIRC where the `width` stays
> constant (unless overridden) and a horizontal scrollbar is shown.

The width is constant once the control is rendered, however different input types have different widths. Furthermore, the width of a single date/time input can vary depending on the fields present. For example, a datetime-local input that has a milliseconds field will be wider than one without it.

> > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:82
> > +        String number = locale.convertToLocalizedNumber(makeString(pad(c, length, String(&c, 1))));
> 
> Is the `makeString` necessary?  Also, perhaps you could use `makeString`
> instead of the `String(&c, 1)`.
> ```
>     auto numberString = locale.convertToLocalizedNumber(pad(c, length,
> makeString(c)));
> ```

The outer `makeString` is necessary as `pad` returns a `PaddingSpecification<String>`. Updated to use `makeString` instead of the `String(&c, 1)`.

> > Source/WebCore/html/shadow/DateTimeNumericFieldElement.cpp:86
> > +    elementStyle.renderStyle->setMinWidth(Length { width, Fixed });
> 
> NIT: is the `Length` necessary?

Removed.

[Aditya Keerthi]

Created attachment 409313 [details]
Patch

[Aditya Keerthi]

Created attachment 409742 [details]
Patch

[Devin Rousso]

Comment on attachment 409742 [details]
Patch

r=me

[Aditya Keerthi]

Created attachment 409861 [details]
Patch for landing

[Aditya Keerthi]

Removed some rebaselines that now pass after the recent change to dumpAsText (https://trac.webkit.org/r267640).

[EWS]

Committed r267701: <https://trac.webkit.org/changeset/267701>

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