264 bytes, text/html
2.71 KB, patch
|Details | Diff | Splinter Review|
858 bytes, patch
|Details | Diff | Splinter Review|
8.08 KB, patch
|Details | Diff | Splinter Review|
This makes performance suck when web content animates something and carelessly updates the value over and over at every animation frame.
This is actually a pain since the DOM4 spec calls for creating a new text node and replacing the contents of the element with it. And that requires frame reconstruction. So the simplest way to fix this would be to violate/change the DOM4 spec. We could change the spec for setting Element/DocumentFragment textContent so that if it has a single text node child already, defer to setting textContent on the text node. Anne, what do you think? Then we could implement that, and also optimize nsDOMGenericDataNode::SetTextInternal to do nothing when the new text equals the replaced text.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2) > We could change the spec for setting Element/DocumentFragment textContent so > that if it has a single text node child already, defer to setting > textContent on the text node. Anne, what do you think? ... if the new text is a non-empty string.
Or we could say that if the first child is a text node, then we set the textContent of the text node and remove the rest of the children. That matches the aTryReuse option in nsContentUtils::SetNodeTextContent.
Assignee: nobody → roc
This changes mutation event behavior --- events don't fire when the old and new text is the same --- but I'm pretty sure we don't care about that.
Attachment #595352 - Flags: review?(jst)
This violates the current DOM4 spec. I think we should just change the spec to allow/require this. The exact behavior change is, when setting textContent on an element or document fragment whose first child is a text node, we do the equivalent of setting .data on that text node and removing the rest of the children (instead of removing all the children and creating a new text node).
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6) > The exact behavior change is, when setting textContent on an element or > document fragment whose first child is a text node, "... and the new text is non-empty," Actually I have a nagging suspicion that someone might have tried this in the past and backed off for some reason. But maybe I'm imagining it.
This seems somewhat fishy... Could you send email to www-dom to hear what other vendors think?
Does this mean I have to subscribe to yet another mailing list?
This sounds like the optimization that WebKit does for innerHTML in some cases... I can't recall what the spec on that ended up being. Does WebKit already do this for textContent too?
I thought webkit's innerHTML behavior was fixed when they moved to HTML5 parser. But I could be wrong.
Comment on attachment 595352 [details] [diff] [review] part 1: don't fire notifications when text doesn't change Yeah, agreed, we should at least take this optimization.
Attachment #595352 - Flags: review?(jst) → review+
So, did anyone profile .textContent handling? Is the the creation of new frame which takes time or what? I'd prefer if we could reuse the textframe, not dom node.
The problem is the invalidation and painting, not the creation of frames and nodes. So to follow the spec and still get the performance win, we'd need to switch the node out from under the frame without informing the frame that anything's changed. Maybe it could be done, but changing the content for a frame violates a fundamental layout invariant. It's much cleaner (and ultimately, more efficient) to reuse the text node.
Reusing text node breaks the API which has been there for almost a decade. I'd rather make changes to Gecko internal APIs than web APIs.
We're talking about breaking mutation events, which are just as old and which we know will break some code (unlike here). Preserving compatibility does not automatically trump any amount of implementation difficulty.
Anyway, I don't have the energy to deal with this. Andreas, if you need this, you'd better find someone on the content team to pick it up.
Assignee: roc → nobody
I can look at this soonish. We could either do some layout hackery, or optimize the hopefully common case when only parent node and nsIFrame own the text node and there are no mutation event listeners or (DOM) mutation observers.
Assignee: nobody → bugs
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f81109133b5 (Don't close bug when landing on central.)
(In reply to Olli Pettay [:smaug] from comment #20) > Created attachment 595712 [details] [diff] [review] > WIP (to replace part 2) That is clever!
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7f81109133b5 for a fistful of orange in content/base/test/test_bug415860.html - your guess is better than mine about which one in the push was at fault. (Rather than a comment that can wind up a ways up from the bottom, to get a bug left open, put "[Leave open after merge]" in the whiteboard.)
It was this patch. The test does n.textContent = "Hello!"; r = document.createRange(); r.setStart(n, 4); r.setEnd(n, 5); s.addRange(r); ... n.textContent = "Hello!"; ok(s == "", "Should have selected ''"); The test expects setting the text node to anything at all to collapse the selection, which is actually what the spec wants. So we need to do something else here, like add a boolean to CharacterDataChangeInfo to indicate whether the text actually changed. (It's a bit of a pain since most of the CharacterDataWillChange/CharacterDataDidChange listeners don't need to be notified if the text actually hasn't changed.)
(In reply to Olli Pettay [:smaug] from comment #20) > Created attachment 595712 [details] [diff] [review] > WIP (to replace part 2) BTW if you're going to do this, you probably should count the frames starting with the primary frame and following nsLayoutUtils::GetFirstContinuationOrSpecialSibling. You don't want the optimization to be defeated by bidi or line breaking.
See also bug 404385, same thing for setting innerHTML.
Attachment #595353 - Flags: feedback?(Ms2ger) → feedback-
Comment on attachment 595353 [details] [diff] [review] Part 2: reuse first text node child when setting textContent Clearing review request here. I think Olli's approach is probably the better way forward here.
Olli, any progress on this? It seems that D3.js ( http://d3js.org/ ) does this a lot (or makes it easy for users to do it a lot), and is hurting us relative to other implementations - see bug 873425.
https://tbpl.mozilla.org/?tree=Try&rev=8aebb68352f8 Would be still nice to find some real world testcase
Attachment #595712 - Attachment is obsolete: true
Hmm, this gets just too tricky. FormFillController, and its tests expects (correctly, per spec) that setting .textContent removes the existing nodes and adds a new one. Since FFC uses nsIMutationObserver, it becomes really tricky to optimize that case.
So one way to fix this is to change nsIMutationObserver method signatures so that old node can actually stay where it is, but if the new params to the methods aren't handled, it looks like a node is removed and then a new one inserted. Need to think that still through tough.
This may cost us a lot on retranslations since we don't check if the textContent changed. Is there any hope to have the engine become smart about it?
Doesn't translation use mutation observer? In that case we have to remove and recreate the dom node (because that is the only sane thing to do from API perspective).
It does, but gaia apps are not checking if the l10nId really changed when they reset data-l10n-id on elements. If MutationObserver is smart enough not to retrigger onMutations if the attributes value has not changed, then we're good here. Otherwise, we retrigger l10n on the node and reset textContent to the same value.
MutationObserver API explicitly requires creating MutationRecords even if the attribute value hasn't changed.
Then I guess it's a wontfix :( I have to say, I wish there was an attribute to turn this overzealous behavior off.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
I don't think this has to be WONTFIX just because we need to fire mutation observers. We should be able to fire mutation observers without triggering wasteful invalidation/layout/paint.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
You need to log in before you can comment on or make changes to this bug.