Bug 228605 – HTMLElement.innerText setter should convert new lines to <br>

• Component: HTML Editing
• Status: RESOLVED FIXED

[Chris Dumez]

HTMLElement.innerText setter should convert new lines to <br>:
- https://html.spec.whatwg.org/multipage/dom.html#the-innertext-idl-attribute:dom-innertext-3

This is causing some failures on http://wpt.live/html/dom/elements/the-innertext-and-outertext-properties/innertext-setter.html, in Webkit only.

[Chris Dumez]

Created attachment 434568 [details]
WIP Patch

[Chris Dumez]

Created attachment 434577 [details]
Patch

[Chris Dumez]

Created attachment 434585 [details]
Patch

[Chris Dumez]

Created attachment 434635 [details]
Patch

[EWS]

Committed r280482 (240117@main): <https://commits.webkit.org/240117@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/81333038>

[Darin Adler]

Comment on attachment 434635 [details]
Patch

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

> Source/WebCore/html/HTMLElement.cpp:-573
> -    if ((r && r->style().preserveNewline()) || (isConnected() && isTextControlInnerTextElement())) {

Do we have test cases that cover text controls? Just wanted to make sure we didn’t lose any valuable behavior that happened to not be covered by web platform tests.

[Chris Dumez]

Comment on attachment 434635 [details]
Patch

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

>> Source/WebCore/html/HTMLElement.cpp:-573
>> -    if ((r && r->style().preserveNewline()) || (isConnected() && isTextControlInnerTextElement())) {
> 
> Do we have test cases that cover text controls? Just wanted to make sure we didn’t lose any valuable behavior that happened to not be covered by web platform tests.

I suspect that the "Newlines convert to <br> in <textarea>" WPT test covers this but I'll double check.

[Chris Dumez]

Comment on attachment 434635 [details]
Patch

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

> LayoutTests/platform/mac-catalina/fast/parser/open-comment-in-textarea-expected.txt:13
> +      RenderBR {BR} at (21,0) size 1x13

Oh, the `isConnected() && isTextControlInnerTextElement()` logic doesn't actually trigger in those WPT tests because the logic only triggers when calling setInnerText() on browser-shadow-element inside a text control. Therefore, I think this is probably not Web-Observable. This is what's causing the output change in this test though where we have a blank text run replaced with a RenderBR. I looked at those test's visual output with and without my change and they do look the same. For safety, I could keep the old behavior in the browser-shadow-dom case since it should not be a compatibility-risk AFAICT. However, there may be assumption in WebKit code about the formatting in those browser-shadow-dom-elements.

[Darin Adler]

Well, does setting "innerText" have the same behavior as pasting?

[Chris Dumez]

(In reply to Darin Adler from comment #10)
> Well, does setting "innerText" have the same behavior as pasting?

I cannot call innerText from JS on a browser-shadow-dom element AFAIK and the virtual output looks just the same before and after my change. I am not sure I understand the question.

[Chris Dumez]

Reverted r280482 for reason:

Will take a more conservative approach

Committed r280502 (240134@main): <https://commits.webkit.org/240134@main>

[Darin Adler]

(In reply to Chris Dumez from comment #12)
> Will take a more conservative approach

I hope that’s not just because of my questions.

[Darin Adler]

(In reply to Chris Dumez from comment #11)
> (In reply to Darin Adler from comment #10)
> > Well, does setting "innerText" have the same behavior as pasting?
> 
> I cannot call innerText from JS on a browser-shadow-dom element AFAIK and
> the virtual output looks just the same before and after my change. I am not
> sure I understand the question.

My question was: When we handle a paste, can we end up with newline characters inside the shadow DOM rather than <br> elements? If so, that might be OK.

[Chris Dumez]

Created attachment 434671 [details]
Patch

[Darin Adler]

Comment on attachment 434671 [details]
Patch

Does seem smart to do the part that is visible; we can clean up by removing the other part at our leisure once we are sure it’s not needed.

[Chris Dumez]

(In reply to Darin Adler from comment #16)
> Comment on attachment 434671 [details]
> Patch
> 
> Does seem smart to do the part that is visible; we can clean up by removing
> the other part at our leisure once we are sure it’s not needed.

Yes, I don't want to take any chances, especially at this time of the year. My initial patch was definitely more heavy-handed than I realized because I thought it was about text controls, not browser-shadow-elements inside those controls.

[Chris Dumez]

Created attachment 434677 [details]
Patch

[EWS]

Committed r280541 (240168@main): <https://commits.webkit.org/240168@main>

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