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.
Created attachment 434568 [details] WIP Patch
Created attachment 434577 [details] Patch
Created attachment 434585 [details] Patch
Created attachment 434635 [details] Patch
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].
<rdar://problem/81333038>
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.
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.
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.
Well, does setting "innerText" have the same behavior as pasting?
(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.
Reverted r280482 for reason: Will take a more conservative approach Committed r280502 (240134@main): <https://commits.webkit.org/240134@main>
(In reply to Chris Dumez from comment #12) > Will take a more conservative approach I hope that’s not just because of my questions.
(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.
Created attachment 434671 [details] Patch
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.
(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.
Created attachment 434677 [details] Patch
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].