Bug 276365
| Summary: | script.innerText interop issues with new lines | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Luke Warlow <lwarlow> |
| Component: | DOM | Assignee: | Nobody <webkit-unassigned> |
| Status: | NEW | ||
| Severity: | Normal | CC: | ahmad.saleem792, annevk, cdumez, karlcow, rniwa, webkit-bug-importer |
| Priority: | P2 | Keywords: | BrowserCompat, InRadar |
| Version: | Safari 17 | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=289076 | ||
| Bug Depends on: | |||
| Bug Blocks: | 289597 | ||
Luke Warlow
There's an interop issue with WebKit and its newline handling inside of script.innerText setter.
1. Load data:text/html,<body><script>const script = document.createElement('script'); document.body.appendChild(script); script.innerText = `console.log('line 1');\nconsole.log('line 2');`</script> in Safari.
2. See only 'line 1' is logged.
Repeat in Chromium and Firefox and see line 2 is logged as well.
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Luke Warlow
It's worth pointing out this only happens when the script is attached to the document *before* setting innerText to the value.
If I had to hazard a guess the innerText setter is probably resulting in HTMLScriptElement::childrenChanged() being called after the first text node is inserted which executes the script early.
Ryosuke Niwa
Such an interesting bug!
Ahmad Saleem
We don't have this test on WPT - since all browsers pass all 'setter' test: https://wpt.fyi/results/html/dom/elements/the-innertext-and-outertext-properties/innertext-setter.html?label=master&label=experimental&aligned&q=setter
Would be good to add test coverage on WPT as well.
Radar WebKit Bug Importer
<rdar://problem/131835109>
Karl Dubost
I wonder if it is related to some of the failures in
https://wpt.fyi/results/dom/nodes/insertion-removing-steps
To note that this is working with textContent
data:text/html,<body><script>const script = document.createElement('script'); document.body.appendChild(script); script.textContent = `console.log('line 1');\nconsole.log('line 2');`</script>
And this is working if we do innerText without \n
data:text/html,<body><script>const script = document.createElement('script'); document.body.appendChild(script); script.innerText = `console.log('line 1');console.log('line 2');`</script>
So this is the processing of `\n` in the innerText setter which screws up things.
Karl Dubost
https://searchfox.org/wubkat/rev/b36cbce69fddb7da33823f316bd8ead5bebee970/Source/WebCore/html/HTMLScriptElement.cpp#112-135
HTMLScriptElement::setTextContent()
calls setTextContent(WTFMove(newValue));
https://searchfox.org/wubkat/rev/b36cbce69fddb7da33823f316bd8ead5bebee970/Source/WebCore/dom/Node.cpp#1774
HTMLScriptElement::setInnerText()
calls setInnerText(WTFMove(newValue));
https://searchfox.org/wubkat/rev/b36cbce69fddb7da33823f316bd8ead5bebee970/Source/WebCore/html/HTMLElement.cpp#542
These two are very different.
ExceptionOr<void> HTMLElement::setInnerText(String&& text)
{
// FIXME: This doesn't take whitespace collapsing into account at all.
if (!text.contains([](UChar c) { return c == '\n' || c == '\r'; })) {
stringReplaceAll(WTFMove(text));
return { };
}
if (isConnected() && isTextControlInnerTextElement()) {
if (!text.contains('\r')) {
stringReplaceAll(WTFMove(text));
return { };
}
String textWithConsistentLineBreaks = makeStringBySimplifyingNewLines(text);
stringReplaceAll(WTFMove(textWithConsistentLineBreaks));
return { };
}
// FIXME: This should use replaceAll(), after we fix that to work properly for DocumentFragment.
// Add text nodes and <br> elements.
Ref fragment = textToFragment(document(), WTFMove(text));
// It's safe to dispatch events on the new fragment since author scripts have no access to it yet.
ScriptDisallowedScope::EventAllowedScope allowedScope(fragment.get());
return replaceChildrenWithFragment(*this, WTFMove(fragment));
}
I wonder what
String textWithConsistentLineBreaks = makeStringBySimplifyingNewLines(text);
does to the string.
Karl Dubost
The OUTPUT is
-------
line 1
-------
for:
console.log('line 1');\nconsole.log('line 2');
console.log('line 1');\rconsole.log('line 2');
The OUTPUT is
-------
line 1
line 2
-------
for:
console.log('line 1');console.log('line 2');
console.log('line 1');\fconsole.log('line 2');
console.log('line 1');\tconsole.log('line 2');
console.log('line 1');\vconsole.log('line 2');
Karl Dubost
https://github.com/WebKit/WebKit/blob/501b962b3159b23f860841c5b2dd0931df79f186/Source/WTF/wtf/text/StringView.cpp#L461-L481
template<typename CharacterType> static String makeStringBySimplifyingNewLinesSlowCase(const String& string, unsigned firstCarriageReturn)
{
unsigned length = string.length();
unsigned resultLength = firstCarriageReturn;
auto characters = string.span<CharacterType>();
CharacterType* resultCharacters;
auto result = String::createUninitialized(length, resultCharacters);
memcpy(resultCharacters, characters.data(), firstCarriageReturn * sizeof(CharacterType));
for (unsigned i = firstCarriageReturn; i < length; ++i) {
if (characters[i] != '\r')
resultCharacters[resultLength++] = characters[i];
else {
resultCharacters[resultLength++] = '\n';
if (i + 1 < length && characters[i + 1] == '\n')
++i;
}
}
if (resultLength < length)
result = StringImpl::createSubstringSharingImpl(*result.impl(), 0, resultLength);
return result;
}