1391044 - Textarea placeholder does not respect newlines

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P3)

Description

[Terence Eden]

Attachment: Firefox for Android displaying a multiline placeholder text on one line

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170612121707

Steps to reproduce:

Create a `<textarea>` with a multi-line placeholder.

```
<textarea placeholder="In loving memory of
Buffy Anne Summers
She saved the world
A lot..."></textarea>
```


Actual results:

The placeholder renders as a single line.

`In loving memory ofBuffy Anne SummersShe saved the worldA lot...`


Expected results:

Placeholder should display over multiple lines.

Ref: MDN documentation https://developer.mozilla.org/en-US/docs/Web/HTML/Element/textarea#attr-placeholder

Ref: W3C specification https://www.w3.org/TR/html52/sec-forms.html#element-attrdef-textarea-placeholder

> User agents should present this hint to the user when the element’s value is the empty string and the control is not focused (e.g., by displaying it inside a blank unfocused control). All U+000D CARRIAGE RETURN U+000A LINE FEED character pairs (CRLF) in the hint, as well as all other U+000D CARRIAGE RETURN (CR) and U+000A LINE FEED (LF) characters in the hint, must be treated as line breaks when rendering the hint.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

This looks busted in desktop too. Test case here: https://glitch.com/edit/#!/phantom-brochure?path=views/index.html

Moving to Layout

Comment 2

[Terence Eden]

Attachment: 57 being affected

Comment 3

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1391044 - Preserve newlines in textarea placeholders.

Review commit: https://reviewboard.mozilla.org/r/203214/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203214/

Comment 4

[Ian Moody [:Kwan] (UTC+1)]

Despite not knowing C++, this turned out to be (seemingly) fairly simple.
Try run (changeset not listed because it was in an earlier aborted run, is parent of the listed change):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1323a57b8bbf84763ee1141893abf52bbf5a5d45

Boris, I've picked you because you have most of the recent r+ in this file, and your queue isn't too big right now (almost the shortest of the Layout Engine peers), feel free to re-direct.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8932205 [details]
Bug 1391044 - Preserve newlines in textarea placeholders.

https://reviewboard.mozilla.org/r/203214/#review208638

This needs tests.

In particular, what is the actual behavior this produces for lone \r and does it match the spec?

Comment 6

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> Comment on attachment 8932205 [details]
> Bug 1391044 - Preserve newlines in textarea placeholders.
> 
> https://reviewboard.mozilla.org/r/203214/#review208638
> 
> This needs tests.
Are reftests sufficient?
e.g.
<textarea placeholder="this is\ra multiline\rplaceholder"></textarea>
matching
<textarea>this is\ra multiline\rplaceholder</textarea>
and another for input?
<input placeholder="this is\ra multiline\rplaceholder">
matching
<input value="this isa multilineplaceholder">
(with ::placeholder { opacity: 1; })

> In particular, what is the actual behavior this produces for lone \r and
> does it match the spec?
For <textarea>, a placeholder with a single newline.  For an <input>, no placeholder, identical to placeholder="" and bare placeholder. (checked via the inspector in the browser toolbox with e10s off).  Which as far as I can tell does match the spec.  Relevant parts:

https://html.spec.whatwg.org/#attr-textarea-placeholder
>User agents should present this hint to the user when the element's value is the empty string and the control is not focused (e.g. by displaying it inside a blank unfocused control). All U+000D CARRIAGE RETURN U+000A LINE FEED character pairs (CRLF) in the hint, as well as all other U+000D CARRIAGE RETURN (CR) and U+000A LINE FEED (LF) characters in the hint, must be treated as line breaks when rendering the hint. 

https://html.spec.whatwg.org/#the-placeholder-attribute
>User agents should present this hint to the user, after having stripped newlines[1] from it, when the element's value is the empty string, especially if the control is not focused.
[1]https://infra.spec.whatwg.org/#strip-newlines
>To strip newlines from a string, remove any U+000A LF and U+000D CR code points from the string.

Comment 7

[Ian Moody [:Kwan] (UTC+1)]

Note <input value> doesn't work as a reference because of minor font differences, but <input placeholder> without the newlines does.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Are reftests sufficient?

Yes, absolutely.

> For <textarea>, a placeholder with a single newline.

Excellent.

Comment 9

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1391044 - Add wpt reftests for multiline placeholders in <textarea> and <input>.

Review commit: https://reviewboard.mozilla.org/r/203504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203504/

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8932456 [details]
Bug 1391044 - Add wpt reftests for multiline placeholders in <textarea> and <input>.

https://reviewboard.mozilla.org/r/203504/#review209484

This is a good start, but it really should test various combinations of \r, \r\n, and \n....

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8932456 [details]
Bug 1391044 - Add wpt reftests for multiline placeholders in <textarea> and <input>.

https://reviewboard.mozilla.org/r/203504/#review209492

And also, it might be nice to make these wpt reftests...

Comment 12

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10)
> Comment on attachment 8932456 [details]
> Bug 1391044 - Add reftests for multiline placeholders in <textarea> and
> <input>.
> 
> https://reviewboard.mozilla.org/r/203504/#review209484
> 
> This is a good start, but it really should test various combinations of \r,
> \r\n, and \n....
Do you mean different types of line endings with the same file/textarea?
i.e.
<textarea rows="6" placeholder="this is\r
a multiline\r
placeholder\r\n
with different\n
line endings"></textarea>
Or multiple test files?
If so that seems slightly fragile, since certainly at least Sublime unifies the line-endings on save.  Though since generally test files aren't edited much I guess it'd be fine, just leave a comment in the source about it.  SciTE and Notepad++ certainly handle it alright.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> Comment on attachment 8932456 [details]
> Bug 1391044 - Add reftests for multiline placeholders in <textarea> and
> <input>.
> 
> https://reviewboard.mozilla.org/r/203504/#review209492
> 
> And also, it might be nice to make these wpt reftests...
I think I've figured out how to do that, I'll attach WIP shortly.

Comment 13

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8932456 [details]
Bug 1391044 - Add wpt reftests for multiline placeholders in <textarea> and <input>.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203504/diff/1-2/

Comment 14

[Boris Zbarsky [:bzbarsky]]

> Do you mean different types of line endings with the same file/textarea?

That would be fine.  Different files would be fine too.  Whatever is easiest.

> since certainly at least Sublime unifies the line-endings on save. 

Yeah, editors....  The test could get the .textContent of the textarea, check it for the charCodeAt() values, and if they're wrong replace the text, or the whole textarea, with a string that says "your editor messed up the newlines" or something.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> check it for the charCodeAt() values

Or just an explicit equality check against a JS string that uses \r and \n to represent the relevant newlines.  Editors shouldn't mess with that.  ;)

Comment 16

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #14)
> > Do you mean different types of line endings with the same file/textarea?
> 
> That would be fine.  Different files would be fine too.  Whatever is easiest.
Ah cool, I'll do separate files to sidestep the editor unification problem, I just wasn't sure if you specifically wanted to test a placeholder than had multiple types of line endings in it.
> > since certainly at least Sublime unifies the line-endings on save. 
> 
> Yeah, editors....  The test could get the .textContent of the textarea,
> check it for the charCodeAt() values, and if they're wrong replace the text,
> or the whole textarea, with a string that says "your editor messed up the
> newlines" or something.
I'm guessing you meant .placeholder? (which I didn't know about until just now), but it doesn't matter since I'm going with the separate files.

Comment 17

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8932205 [details]
Bug 1391044 - Preserve newlines in textarea placeholders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203214/diff/1-2/

Comment 18

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8932456 [details]
Bug 1391044 - Add wpt reftests for multiline placeholders in <textarea> and <input>.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203504/diff/2-3/

Comment 19

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8932456 [details]
Bug 1391044 - Add wpt reftests for multiline placeholders in <textarea> and <input>.

Requesting re-review manually since mozreview doesn't seem to let me.

Mozreview also doesn't handle CR EOLs well.  The raw diff shows it though.

(I forgot to ask if you prefer splinter)

Comment 20

[Boris Zbarsky [:bzbarsky]]

> I'm guessing you meant .placeholder? 

Er, yes.  ;)

Might still be worth checking the value in case editors normalize anyway...

Comment 21

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> > check it for the charCodeAt() values
> 
> Or just an explicit equality check against a JS string that uses \r and \n
> to represent the relevant newlines.  Editors shouldn't mess with that.  ;)

Browsers seem to though.  Both Firefox (with my patch to fix this bug) and Chromium match all three placeholders only to \n-only versions of the string.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Ah, the parser might normalize newlines.  You can get there by doing setAttribute("placeholder", value) where the value is a JS string with the relevant \r bits.

Comment 23

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #22)
> Ah, the parser might normalize newlines.  You can get there by doing
> setAttribute("placeholder", value) where the value is a JS string with the
> relevant \r bits.

Ah, so at the moment the extra tests are only actually testing how the parser treats newlines? (normalises them all to \n, from the look of it)

In that case, using textarea.placeholder = "test\rmultiline" (or setAttribute()), all of Firefox, Chromium, and Edge don't match the spec when the placeholder is set via JS, since none of them treat CR as a line break (and charCodeAt(4) returns 13, so it's there).

https://html.spec.whatwg.org/multipage/form-elements.html#attr-textarea-placeholder
>All U+000D CARRIAGE RETURN U+000A LINE FEED character pairs (CRLF) in the hint, as well as all other U+000D CARRIAGE RETURN (CR) and U+000A LINE FEED (LF) characters in the hint, must be treated as line breaks when rendering the hint.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> all of Firefox, Chromium, and Edge don't match the spec when the placeholder is set via JS

"awesome".

In the Firefox case, this should be pretty easy to fix, I expect:

  if (IsTextArea()) {
    nsContentUtils::PlatformToDOMLineBreaks(placeholderTxt);
  } else {
    nsContentUtils::RemoveNewlines(placeholderTxt);
  }

Now that I think about this, you could also test this more directly as:

  <textarea placeholder="first line&#xd;second line"></textarea>

and I can definitely confirm that Chrome fails to handle that right.  :(  Are you up for filing bugs on the other browsers?  If not, I can file one on Chrome, at least, though not on Edge...

Comment 25

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #24)
> > all of Firefox, Chromium, and Edge don't match the spec when the placeholder is set via JS
> 
> "awesome".

Heh.

> In the Firefox case, this should be pretty easy to fix, I expect:
> 
>   if (IsTextArea()) {
>     nsContentUtils::PlatformToDOMLineBreaks(placeholderTxt);
>   } else {
>     nsContentUtils::RemoveNewlines(placeholderTxt);
>   }

Yep, that works.

> Now that I think about this, you could also test this more directly as:
> 
>   <textarea placeholder="first line&#xd;second line"></textarea>
> 
> and I can definitely confirm that Chrome fails to handle that right.  :( 
> Are you up for filing bugs on the other browsers?  If not, I can file one on
> Chrome, at least, though not on Edge...

Filed: https://bugs.chromium.org/p/chromium/issues/detail?id=791194
Edge also has the same problem for textarea content, where Chrome and Firefox always break on CR.
Test case:
http://kwan.perix.co.uk/mozilla/ta-ph-cr.html

Comment 26

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8932205 [details]
Bug 1391044 - Preserve newlines in textarea placeholders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203214/diff/2-3/

Comment 27

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8932456 [details]
Bug 1391044 - Add wpt reftests for multiline placeholders in <textarea> and <input>.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203504/diff/3-4/

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8932205 [details]
Bug 1391044 - Preserve newlines in textarea placeholders.

https://reviewboard.mozilla.org/r/203214/#review210308

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8932456 [details]
Bug 1391044 - Add wpt reftests for multiline placeholders in <textarea> and <input>.

https://reviewboard.mozilla.org/r/203504/#review210310

The <input> tests are still just testing parser behavior; they should probably use &#xa/d like the textarea tests.  r=me with that.

Comment 30

[Boris Zbarsky [:bzbarsky]]

And thank you for putting up with all the back-and-forth and reporting those issues!

Comment 31

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8932205 [details]
Bug 1391044 - Preserve newlines in textarea placeholders.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203214/diff/3-4/

Comment 32

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8932456 [details]
Bug 1391044 - Add wpt reftests for multiline placeholders in <textarea> and <input>.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203504/diff/4-5/

Comment 33

[Ian Moody [:Kwan] (UTC+1)]

Urgh, rebasing over a wpt update is _not fun_.
Also the old "manifest update for another bug" went away, but I picked up two new ones from a different bug.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #29)
> Comment on attachment 8932456 [details]
> Bug 1391044 - Add wpt reftests for multiline placeholders in <textarea> and
> <input>.
> 
> https://reviewboard.mozilla.org/r/203504/#review210310
> 
> The <input> tests are still just testing parser behavior; they should
> probably use &#xa/d like the textarea tests.  r=me with that.

Ah, thanks, I'd practically forgotten about the input tests after the fiddling with the textarea ones.  I brought them up to date with the textarea ones with the entity and JS tests.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #30)
> And thank you for putting up with all the back-and-forth and reporting those
> issues!

Thank you for all the feedback and the reviews!

Sheriff: green wpt try at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91f99207467a610395c3aafff911f4416656271b
(There's an older version with Reftests and browser-chrome tests at
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1323a57b8bbf84763ee1141893abf52bbf5a5d45
though the changeset isn't immediately listed because it's on an earlier push as well)

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/97d4dda25d65
Preserve newlines in textarea placeholders. r=bz
https://hg.mozilla.org/integration/autoland/rev/ec84ecc153cc
Add wpt reftests for multiline placeholders in <textarea> and <input>. r=bz

Comment 35

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Ian Moody [:Kwan] from comment #25)
> (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from
> comment #24)
> > > all of Firefox, Chromium, and Edge don't match the spec when the placeholder is set via JS
> > 
> > "awesome".
> 
> Filed: https://bugs.chromium.org/p/chromium/issues/detail?id=791194
> Edge also has the same problem for textarea content, where Chrome and
> Firefox always break on CR.
> Test case:
> http://kwan.perix.co.uk/mozilla/ta-ph-cr.html

It seems I'd missed some nuance with Edge here, I must have tested it with an older version of the page.
Edge does line break on CR in content set via .value, but not via .textContent.
Yay.

Comment 36

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/97d4dda25d65
https://hg.mozilla.org/mozilla-central/rev/ec84ecc153cc