Bug 216616

Summary: [macOS] Update the appearance of editable date/time controls to match the system
Product: WebKit Reporter: Aditya Keerthi <akeerthi>
Component: FormsAssignee: Aditya Keerthi <akeerthi>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, changseok, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hi, kondapallykalyan, macpherson, menard, pdr, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Mac   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=216838
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
hi: review+, ews-feeder: commit-queue-
Patch for landing none

Description Aditya Keerthi 2020-09-16 11:59:12 PDT
- Rounded corners on focused component
- Raised colon separator for time inputs
- Fixed width components, with a variable width for the control itself
Comment 1 Aditya Keerthi 2020-09-16 12:02:09 PDT
<rdar://problem/69004603>
Comment 2 Aditya Keerthi 2020-09-16 12:29:04 PDT
Created attachment 408943 [details]
Patch
Comment 3 Aditya Keerthi 2020-09-17 15:07:37 PDT
Created attachment 409068 [details]
Patch
Comment 4 Devin Rousso 2020-09-18 17:50:27 PDT
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)
Comment 5 Aditya Keerthi 2020-09-20 13:06:19 PDT
Created attachment 409243 [details]
Patch
Comment 6 Aditya Keerthi 2020-09-20 13:14:18 PDT
(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.
Comment 7 Aditya Keerthi 2020-09-21 13:39:31 PDT
Created attachment 409313 [details]
Patch
Comment 8 Aditya Keerthi 2020-09-25 14:38:57 PDT
Created attachment 409742 [details]
Patch
Comment 9 Devin Rousso 2020-09-25 14:41:41 PDT
Comment on attachment 409742 [details]
Patch

r=me
Comment 10 Aditya Keerthi 2020-09-27 15:33:33 PDT
Created attachment 409861 [details]
Patch for landing
Comment 11 Aditya Keerthi 2020-09-27 15:35:55 PDT
Removed some rebaselines that now pass after the recent change to dumpAsText (https://trac.webkit.org/r267640).
Comment 12 EWS 2020-09-28 08:37:23 PDT
Committed r267701: <https://trac.webkit.org/changeset/267701>

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