Bug 219217 - Space between minute and meridiem fields in time inputs is too large
Summary: Space between minute and meridiem fields in time inputs is too large
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari Technology Preview
Hardware: Macintosh Unspecified
: P2 Normal
Assignee: Aditya Keerthi
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-11-20 12:01 PST by Aditya Keerthi
Modified: 2020-11-21 09:24 PST (History)
8 users (show)

See Also:


Attachments
Patch (2.70 KB, patch)
2020-11-20 13:34 PST, Aditya Keerthi
drousso: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (14.19 KB, patch)
2020-11-20 21:12 PST, Aditya Keerthi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aditya Keerthi 2020-11-20 12:01:26 PST
The space is 2px larger than expected.
Comment 1 Aditya Keerthi 2020-11-20 12:01:42 PST
<rdar://problem/71637133>
Comment 2 Aditya Keerthi 2020-11-20 13:34:10 PST
Created attachment 414711 [details]
Patch
Comment 3 Devin Rousso 2020-11-20 16:02:49 PST
Comment on attachment 414711 [details]
Patch

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

r=me

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:174
> +    // If the literal begins/ends with a space, the gap between two fields will appear

Do we care about any other forms of whitespace?

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:178
> +    // together by applying a negative margin.

Is it possible to avoid using a negative margin?  What about if we only applied the `padding` if the first/last character is not a space?

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:179
> +    if (text.characterAt(0) == ' ')

NIT: `text.startsWith(' ')`

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:180
> +        element->setInlineStyleProperty(CSSPropertyMarginLeft, -1, CSSUnitType::CSS_PX);

What about RTL?  Should this be `CSSPropertyMarginInlineStart`?

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:181
> +    if (text.characterAt(text.length() - 1) == ' ')

NIT: `text.endsWith(' ')`

> Source/WebCore/html/shadow/DateTimeEditElement.cpp:182
> +        element->setInlineStyleProperty(CSSPropertyMarginRight, -1, CSSUnitType::CSS_PX);

ditto (:180)
Comment 4 Aditya Keerthi 2020-11-20 17:06:12 PST
(In reply to Devin Rousso from comment #3)
> Comment on attachment 414711 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414711&action=review
> 
> r=me
> 
> > Source/WebCore/html/shadow/DateTimeEditElement.cpp:174
> > +    // If the literal begins/ends with a space, the gap between two fields will appear
> 
> Do we care about any other forms of whitespace?

We don't because other forms of whitespace should not appear in a date/time format string.
 
> > Source/WebCore/html/shadow/DateTimeEditElement.cpp:178
> > +    // together by applying a negative margin.
> 
> Is it possible to avoid using a negative margin?  What about if we only
> applied the `padding` if the first/last character is not a space?

If we don't add the padding, the selected appearance of the fields would appear too squished, and would also be inconsistent with the other fields.

The only other alternative I can think of to avoid using a negative margin would be to replace all spaces with fixed-width spans, but that would be a much more complex solution.

> > Source/WebCore/html/shadow/DateTimeEditElement.cpp:180
> > +        element->setInlineStyleProperty(CSSPropertyMarginLeft, -1, CSSUnitType::CSS_PX);
> 
> What about RTL?  Should this be `CSSPropertyMarginInlineStart`?

Will change to use `CSSPropertyMarginInlineStart` and `CSSPropertyMarginInlineEnd`.
Comment 5 Aditya Keerthi 2020-11-20 21:12:59 PST
Created attachment 414751 [details]
Patch for landing
Comment 6 EWS 2020-11-21 09:24:47 PST
Committed r270148: <https://trac.webkit.org/changeset/270148>

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