Bug 276365 - script.innerText interop issues with new lines
Summary: script.innerText interop issues with new lines
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari 17
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: BrowserCompat, InRadar
Depends on:
Blocks:
 
Reported: 2024-07-09 06:31 PDT by Luke Warlow
Modified: 2024-08-22 22:06 PDT (History)
6 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Warlow 2024-07-09 06:31:13 PDT
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.
Comment 1 Luke Warlow 2024-07-09 06:32:57 PDT
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.
Comment 2 Ryosuke Niwa 2024-07-09 10:29:31 PDT
Such an interesting bug!
Comment 3 Ahmad Saleem 2024-07-09 10:46:57 PDT
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.
Comment 4 Radar WebKit Bug Importer 2024-07-16 06:32:13 PDT
<rdar://problem/131835109>
Comment 5 Karl Dubost 2024-08-06 00:27:53 PDT
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.
Comment 6 Karl Dubost 2024-08-06 00:38:07 PDT
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.
Comment 7 Karl Dubost 2024-08-06 18:05:56 PDT
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');
Comment 8 Karl Dubost 2024-08-22 22:06:24 PDT
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;
}