Summary: | [macOS] Update the appearance of editable date/time controls to match the system | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aditya Keerthi <akeerthi> | ||||||||||||||
Component: | Forms | Assignee: | 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
Aditya Keerthi
2020-09-16 11:59:12 PDT
Created attachment 408943 [details]
Patch
Created attachment 409068 [details]
Patch
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) Created attachment 409243 [details]
Patch
(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. Created attachment 409313 [details]
Patch
Created attachment 409742 [details]
Patch
Comment on attachment 409742 [details]
Patch
r=me
Created attachment 409861 [details]
Patch for landing
Removed some rebaselines that now pass after the recent change to dumpAsText (https://trac.webkit.org/r267640). Committed r267701: <https://trac.webkit.org/changeset/267701> All reviewed patches have been landed. Closing bug and clearing flags on attachment 409861 [details]. |