Bug 195125 – [iOS] Caret x-position in empty text area does not match text field

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Daniel Bates]

The x-position of the caret in an empty text field does not match the x-position of the caret in a text field. You can see this for yourself by visiting <data:text/html,<input placeholder="text"><textarea placeholder="text"></textarea>>.

[Daniel Bates]

<rdar://problem/47161070>

[Daniel Bates]

Created attachment 363140 [details]
Work-in-progress

[Daniel Bates]

Created attachment 363348 [details]
Patch and layout test

[EWS Watchlist]

Comment on attachment 363348 [details]
Patch and layout test

Attachment 363348 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11334077

New failing tests:
fast/forms/textarea-scrolled-type.html
fast/forms/textarea-width.html
fast/forms/textarea-placeholder-visibility-2.html
fast/forms/textarea-align.html
fast/forms/form-element-geometry.html
fast/forms/text-control-intrinsic-widths.html
fast/forms/textarea-scroll-height.html
fast/forms/textarea-scrollbar.html
fast/forms/negativeLineHeight.html
fast/forms/textarea-setinnerhtml.html
fast/forms/textarea-placeholder-visibility-1.html
fast/forms/textAreaLineHeight.html
fast/forms/textarea-placeholder-pseudo-style.html
fast/forms/basic-textareas.html

[EWS Watchlist]

Created attachment 363368 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Daniel Bates]

Created attachment 363370 [details]
Patch and layout tests

[Darin Adler]

Comment on attachment 363370 [details]
Patch and layout tests

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

> Source/WebCore/css/html.css:400
> +    padding: 0.2em 0.5em 0.3em 0.5em; /* Keep padding-left, padding-right in sync with textarea selector (below). */

Should say "keep the values the same", not "keep them in sync" since "in sync" sounds like something more sophisticated that could mean something different. Also, this comment, like all comments, ought to say *why*, not just leave instructions for future programmers to follow without rationale. Still want to keep the comment wording short, if possible.

> Source/WebCore/css/html.css:681
> +    /* Keep padding-left, padding-right sync with input selector (above). */

Would be best if the comment format was identical to above. The other one is at the end of a line but this is between lines. Also, this says "keep sync" rather than "keep in sync", but should be reworded anyway.

But also, the thing above isn’t the input selector. On the iOS Family platform, which is the only platform this code is active on, the above selector is a selector that matches *many* things, including textarea, just like this one. So it’s a misnomer to call the thing about the "input selector". And that observation points to a way to avoid this problem. Instead of "padding: 2x", which I suppose is what we want for other platforms, on the iOS Family platforms maybe we should instead just do "padding-top: 2px; padding-bottom: 2px", then we will inherit the padding values from the earlier rule and won’t need padding-left/padding-right values nor a comment telling us to keep things "in sync". Still might need a comment explaining why we carefully avoid setting padding-left and padding-right.

[Daniel Bates]

(In reply to Darin Adler from comment #7)
> Comment on attachment 363370 [details]
> Patch and layout tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363370&action=review
> 
> > Source/WebCore/css/html.css:400
> > +    padding: 0.2em 0.5em 0.3em 0.5em; /* Keep padding-left, padding-right in sync with textarea selector (below). */
> 
> Should say "keep the values the same", not "keep them in sync" since "in
> sync" sounds like something more sophisticated that could mean something
> different. Also, this comment, like all comments, ought to say *why*, not
> just leave instructions for future programmers to follow without rationale.
> Still want to keep the comment wording short, if possible.
> 
> > Source/WebCore/css/html.css:681
> > +    /* Keep padding-left, padding-right sync with input selector (above). */
> 
> Would be best if the comment format was identical to above. The other one is
> at the end of a line but this is between lines. Also, this says "keep sync"
> rather than "keep in sync", but should be reworded anyway.
> 

Will fix comment wording.

> But also, the thing above isn’t the input selector. On the iOS Family
> platform, which is the only platform this code is active on, the above
> selector is a selector that matches *many* things, including textarea, just
> like this one. So it’s a misnomer to call the thing about the "input
> selector". And that observation points to a way to avoid this problem.
> Instead of "padding: 2x", which I suppose is what we want for other
> platforms, on the iOS Family platforms maybe we should instead just do
> "padding-top: 2px; padding-bottom: 2px", then we will inherit the padding
> values from the earlier rule and won’t need padding-left/padding-right
> values nor a comment telling us to keep things "in sync". Still might need a
> comment explaining why we carefully avoid setting padding-left and
> padding-right.

Yeah, that's a better idea. Will do that.

[Daniel Bates]

Comment on attachment 363370 [details]
Patch and layout tests

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

>>> Source/WebCore/css/html.css:400
>>> +    padding: 0.2em 0.5em 0.3em 0.5em; /* Keep padding-left, padding-right in sync with textarea selector (below). */
>> 
>> Should say "keep the values the same", not "keep them in sync" since "in sync" sounds like something more sophisticated that could mean something different. Also, this comment, like all comments, ought to say *why*, not just leave instructions for future programmers to follow without rationale. Still want to keep the comment wording short, if possible.
> 
> Will fix comment wording.

Removed this comment entirely. Instead added the following comment below:

/* On iOS we want to inherit the left and right padding for consistency with
other form controls (e.g. <input type="text">). We want to use override the
top and bottom padding to ensure we have a symmetric look for text areas. */

[Daniel Bates]

Created attachment 363544 [details]
To land

[Daniel Bates]

Comment on attachment 363544 [details]
To land

Clearing flags on attachment: 363544

Committed r242379: <https://trac.webkit.org/changeset/242379>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to Daniel Bates from comment #9)
> /* On iOS we want to inherit the left and right padding for consistency with
> other form controls (e.g. <input type="text">). We want to use override the
> top and bottom padding to ensure we have a symmetric look for text areas. */

The phrase "use override" might need to be revised; doesn't seem quite right.

[Daniel Bates]

(In reply to Darin Adler from comment #13)
> (In reply to Daniel Bates from comment #9)
> > /* On iOS we want to inherit the left and right padding for consistency with
> > other form controls (e.g. <input type="text">). We want to use override the
> > top and bottom padding to ensure we have a symmetric look for text areas. */
> 
> The phrase "use override" might need to be revised; doesn't seem quite right.

Yikes! Changing "use override" to "override"

[Daniel Bates]

(In reply to Daniel Bates from comment #14)
> (In reply to Darin Adler from comment #13)
> > (In reply to Daniel Bates from comment #9)
> > > /* On iOS we want to inherit the left and right padding for consistency with
> > > other form controls (e.g. <input type="text">). We want to use override the
> > > top and bottom padding to ensure we have a symmetric look for text areas. */
> > 
> > The phrase "use override" might need to be revised; doesn't seem quite right.
> 
> Yikes! Changing "use override" to "override"

Fixed in <https://trac.webkit.org/r242389>.

[Truitt Savell]

It appears that the changes in https://trac.webkit.org/changeset/242379/webkit

caused an API failure on iOS:
Failed

    TestWebKitAPI.DragAndDropTests.ContentEditableToTextarea
        2019-03-04 15:04:34.682 TestWebKitAPI[44455:23074074] Expected selection rects: (
            "NSRect: {{6, 203}, {990, 232}}"
        ) but observed: (
            "NSRect: {{101, 203}, {990, 232}}"
        )
        
        /Volumes/Data/slave/ios-simulator-12-release/build/Tools/TestWebKitAPI/Tests/ios/DragAndDropTestsIOS.mm:137
        Value of: [expected isEqualToArray:observed]
          Actual: false
        Expected: true

Build:
https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/2957

reproduced this locally and found 242379 causes the failure while 242378 does not.

[Daniel Bates]

(In reply to Truitt Savell from comment #16)
> It appears that the changes in
> https://trac.webkit.org/changeset/242379/webkit
> 
> caused an API failure on iOS:
> Failed
> 
>     TestWebKitAPI.DragAndDropTests.ContentEditableToTextarea
>         2019-03-04 15:04:34.682 TestWebKitAPI[44455:23074074] Expected
> selection rects: (
>             "NSRect: {{6, 203}, {990, 232}}"
>         ) but observed: (
>             "NSRect: {{101, 203}, {990, 232}}"
>         )
>         
>        
> /Volumes/Data/slave/ios-simulator-12-release/build/Tools/TestWebKitAPI/Tests/
> ios/DragAndDropTestsIOS.mm:137
>         Value of: [expected isEqualToArray:observed]
>           Actual: false
>         Expected: true
> 
> Build:
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/2957
> 
> reproduced this locally and found 242379 causes the failure while 242378
> does not.

Will look.

[Daniel Bates]

(In reply to Daniel Bates from comment #17)
> (In reply to Truitt Savell from comment #16)
> > It appears that the changes in
> > https://trac.webkit.org/changeset/242379/webkit
> > 
> > caused an API failure on iOS:
> > Failed
> > 
> >     TestWebKitAPI.DragAndDropTests.ContentEditableToTextarea
> >         2019-03-04 15:04:34.682 TestWebKitAPI[44455:23074074] Expected
> > selection rects: (
> >             "NSRect: {{6, 203}, {990, 232}}"
> >         ) but observed: (
> >             "NSRect: {{101, 203}, {990, 232}}"
> >         )
> >         
> >        
> > /Volumes/Data/slave/ios-simulator-12-release/build/Tools/TestWebKitAPI/Tests/
> > ios/DragAndDropTestsIOS.mm:137
> >         Value of: [expected isEqualToArray:observed]
> >           Actual: false
> >         Expected: true
> > 
> > Build:
> > https://build.webkit.org/builders/
> > Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/2957
> > 
> > reproduced this locally and found 242379 causes the failure while 242378
> > does not.
> 
> Will look.

This result is expected now that the left padding of a textarea is specified in ems (so, dependent on the font size). Committed updated test expectations in <https://trac.webkit.org/changeset/242530>.

[Daniel Bates]

(In reply to Truitt Savell from comment #16)
> It appears that the changes in
> https://trac.webkit.org/changeset/242379/webkit
> 
> caused an API failure on iOS:
> Failed
> 
>     TestWebKitAPI.DragAndDropTests.ContentEditableToTextarea
>         2019-03-04 15:04:34.682 TestWebKitAPI[44455:23074074] Expected
> selection rects: (
>             "NSRect: {{6, 203}, {990, 232}}"
>         ) but observed: (
>             "NSRect: {{101, 203}, {990, 232}}"
>         )
>         
>        
> /Volumes/Data/slave/ios-simulator-12-release/build/Tools/TestWebKitAPI/Tests/
> ios/DragAndDropTestsIOS.mm:137
>         Value of: [expected isEqualToArray:observed]
>           Actual: false
>         Expected: true
> 
> Build:
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/2957
> 
> reproduced this locally and found 242379 causes the failure while 242378
> does not.

By the way, I assumed you made typos and meant r242389 and r242388.