Minor textContent / string replace all optimization

[annevk]

It has come to my attention that Chromium has a textContent setter optimization whereby it no-ops for elements that contain a single Text node child whose data matches the given string value: https://bugs.chromium.org/p/chromium/issues/detail?id=793203.

It seems this only applies to textContent and not generally for "string replace all" (which is used in a bunch of places, such as <title>.text and document.title).

As observed in https://bugs.webkit.org/show_bug.cgi?id=244559 this might be worth standardizing, but what exactly do we want to standardize here? Optimize the textContent setter only or "string replace all" generally?

And bonus: do we want to consider further optimizations, such as only updating a single child Text node rather than replacing it? (I suspect this one is out due to web compatibility, but mentioning it for completeness.)

cc @mfreed7 @smaug---- @rniwa

[smaug----]

Oh, there still are these odd web visible optimizations. I'm of course ok with those if one can't observe them. But I'm not ok spec'ing them since they break observing mutations. In the old days webkit, IIRC, had something similar for .innerHTML.

Gecko does reuse text node in certain cases, but not with .textContent.

[rniwa]

In the old days webkit, IIRC, had something similar for .innerHTML.

We still have that optimization. We just made it not run when it's observable.

[annevk]

@smaug---- right, the one Chromium has is observable (it makes the setter no-op as described in OP) and at least WebKit has received a bug report about not doing that, albeit centered around a test case.

@mfreed7 would you be willing to remove the optimization from Chromium? That would be an alternative solution here.

[mfreed7]

We still have that optimization. We just made it not run when it's observable.

@rniwa can you add some detail about how you do that, ideally with a link to code? I assume it's just checking that there aren't any MutationObservers or synchronous event listeners on the node or containing tree?

I'd be up for trying to remove the optimization when it's observable, assuming that's relatively straightforward. There is some (hopefully small?) web compat risk, since the behavior is currently observable.

Generally, I think it'd be odd to try to spec the observable part of this behavior, since it's confusing for developers to have such a special case. (I don't think we need to worry about unobservable optimizations like this, of course.)

[annevk]

@mfreed7 https://github.com/WebKit/WebKit/blob/14c0a0df4180c23444c0c87502e96020065d0585/Source/WebCore/editing/markup.cpp#L1372-L1377.

[mfreed7]

@mfreed7 https://github.com/WebKit/WebKit/blob/14c0a0df4180c23444c0c87502e96020065d0585/Source/WebCore/editing/markup.cpp#L1372-L1377.

Thanks. So no mutation observers, mutation event listeners, and no references to the text node. The last part is interesting, since a) it's hard to do in Blink, and b) I wouldn't think it mattered. I.e. events are the way this is observable, right? From the node itself, all of the attributes would look the same before/after setting the text to the same value. I wonder why that check is there.

[rniwa]

refCount check is there to detect any range, selection, and other objects referencing this text node since range & selection changes are observable. It accounts for the existence of JS wrapper in which case refCount would be positive. If the script had created JS wrapper for this node, then it's observable via object identity checks.

[rniwa]

But I don't think anyone is suggesting that other engines implement the same optimization as WebKit. The question here is whether we'd want to enable the same optimization always even when its side effects are JS observable.

[mfreed7]

refCount check is there to detect any range, selection, and other objects referencing this text node since range & selection changes are observable. It accounts for the existence of JS wrapper in which case refCount would be positive. If the script had created JS wrapper for this node, then it's observable via object identity checks.

Ahh right - Ranges. Ok, makes sense.

But I don't think anyone is suggesting that other engines implement the same optimization as WebKit. The question here is whether we'd want to enable the same optimization always even when its side effects are JS observable.

Right, I understood that. I was just thinking it'd be more straightforward if we only spec the unobservable part. Speccing the observable part a) makes implementations faster overall, and b) makes me happy because I don't have to change Chromium. But it feels like a fairly odd special case. Maybe it's not that bad though, since it's been this way in at least Chromium for some time, and I don't recall seeing bugs about it.

I'd support standardizing at least the (observable) optimization for setting textContent. Going further would require some more research, I think.

[annevk]

I think if you could change Chromium that would be best and also not give us a potential objection from @smaug----. (Unless @rniwa knows about other cases than the aforementioned WebKit bug as to why we need to make this change.)