[webkit-reviews] review granted: [Bug 219217] Space between minute and meridiem fields in time inputs is too large : [Attachment 414711] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Aditya Keerthi
<akeerthi at apple.com>'s request for review:
Bug 219217: Space between minute and meridiem fields in time inputs is too
large
https://bugs.webkit.org/show_bug.cgi?id=219217

Attachment 414711: Patch

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




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 414711
  --> https://bugs.webkit.org/attachment.cgi?id=414711
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)