Closed Bug 679183 Opened 10 years ago Closed 9 years ago
Handle DOM mutations correctly as far as selection is concerned
With bug 619273 fixed, we should start handling elements being added to the selected ranges in the DOM. The simplest strategy for doing so is to see if we can look at the parent of an element to see if it has the selected bit set, and set the selection bit on the new element if it does. We would at least need to do this when an element is bound to the tree or moved inside the DOM. Does that seem like a sane solution?
It's the right idea but not quite enough. For example if a range starts between the second and third child of element A, inserting a node into A's children may need to mark the node selected even though A isn't selected.
(In reply to comment #1) > It's the right idea but not quite enough. For example if a range starts between > the second and third child of element A, inserting a node into A's children may > need to mark the node selected even though A isn't selected. Good point! Mats was worried about the performance characteristics. Should we consider doing some sort of a coalescing in order to win some perf back in the case of a large number of mutations?
I agree this is difficult to get right without a performance hit, but I'm confident it can be done. I think this should be part of bug 619273 actually. Maybe the design should integrate with ranges. Right now each range observes every mutation of the document, so if you have a lot of ranges every DOM operation gets slow. Maybe this? -- Have each Range track the nearest common ancestor of its start and end nodes. (We need to keep this separate from mRoot.) -- Have each Range know whether it belongs to a selection -- Add a node flag "is the nearest common ancestor for some Range", and a property listing all the Ranges for which the node is the nearest common ancestor (as a hashtable) -- Add a node flag "has an ancestor node with the above flag" -- Then a node with no such flags set cannot be part of a Range or selection. If one of those flags is set, we can quickly walk up the ancestor chain to find all Ranges it might be part of, and check those ranges to see which ones are for selection and whether the node is in those ranges. -- Stop Ranges from being mutation observers. Instead, have nsNodeUtils explicitly manage ranges during DOM mutations, using the above flags to optimize in the common case that the affected node(s) are obviously not part of a range/selection.
Looks like a good design to me. I'll start coding this and update if there's any problems.
I really just made that up without thinking hard, so I expect you'll find some problems and need to make some changes :-).
Why can't ranges just be mutation observers on the nearest common ancestor in the comment 3 proposal?
I guess they could.
That's what I was thinking too. I'd prefer to keep ranges use generic nsIMutationObservers rather than having explicit code paths. In fact, we could make ranges *only* observe the nearest common ancestor and just do fixup in the ParentChainChanged notification. We'd have to make the range remember it's whole parent chain and indexes though, so it might not be worth the complexity.
I've implemented roc's original proposal to the point were it pass regression tests. I'll try to adopt Boris' suggestion next, I'm pretty sure it should work based on what my current code that notifies Ranges looks like. Do we really need to track all ranges with the bits in that case? I think it would make things faster if the content bits were: "is the nearest common ancestor for some Range that is in a Selection" and "has an ancestor node with the above flag" BTW, can we remove the nsIDOMNSRange class? (merge it into nsIDOMRange) And merge nsIRange/nsRange? (into nsRange) http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIRange.h#117 (In reply to Jonas Sicking (:sicking) from comment #8) > In fact, we could make ranges *only* observe the nearest common ancestor and > just do fixup in the ParentChainChanged notification. I don't understand, why would observing only the nearest common ancestor not work? (isn't that what Boris suggested?)
I was saying that it might be possible to only observe the nearest common ancestor and not observe mRoot. However I've convinced myself that that is not currently possible to implement the "gravity" code without observing mRoot.
Aha, gravity, our mortal enemy :-) I think we can gravitate a range only observing the common ancestor since we always get a ParentChainChanged notification -- the problem is that the notification does not have enough data about what actually happened... By adding two parameters |nsINode *aOldAncestor, PRUint32 aOldIndex| I think we have what is needed (the range will collapse at that point if aOldAncestor is an ancestor or equal to the range common ancestor). It requires adding the same params to UnbindFromTree, set the values in the top-most call and then propagating the values... Could something like that work?
Yeah, if we add something like that then it'd work. I'm not convinced that's a good idea though as that would mean propagating those arguments through a lot of UnbindFromTree calls. It might give better performance to just leave things as they are now.
This was fixed as part of bug 619273. We could optimize it by observing the range common ancestor instead of the root, as discussed above, but since bug 641821 implements a better mechanism, we should probably wait until that has settled. It would be great if the problem described in comment 11 is addressed in the new mechanism!
Assignee: nobody → matspal
Status: NEW → RESOLVED
Closed: 9 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [fixed by bug 619273]
You need to log in before you can comment on or make changes to this bug.