Last Comment Bug 679183 - Handle DOM mutations correctly as far as selection is concerned
: Handle DOM mutations correctly as far as selection is concerned
Status: RESOLVED FIXED
[fixed by bug 619273]
:
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on: 619273
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-15 15:55 PDT by :Ehsan Akhgari
Modified: 2011-12-27 03:35 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description :Ehsan Akhgari 2011-08-15 15:55:23 PDT
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?
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-15 15:59:38 PDT
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.
Comment 2 :Ehsan Akhgari 2011-08-15 16:31:05 PDT
(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?
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-15 16:57:23 PDT
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.
Comment 4 Mats Palmgren (:mats) 2011-08-15 20:05:12 PDT
Looks like a good design to me.  I'll start coding this and update if there's
any problems.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-15 20:14:59 PDT
I really just made that up without thinking hard, so I expect you'll find some problems and need to make some changes :-).
Comment 6 Boris Zbarsky [:bz] (TPAC) 2011-08-25 22:04:26 PDT
Why can't ranges just be mutation observers on the nearest common ancestor in the comment 3 proposal?
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-25 22:43:17 PDT
I guess they could.
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-25 23:02:25 PDT
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.
Comment 9 Mats Palmgren (:mats) 2011-08-26 11:19:52 PDT
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?)
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-26 20:05:50 PDT
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.
Comment 11 Mats Palmgren (:mats) 2011-08-27 18:43:14 PDT
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?
Comment 12 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-28 22:54:49 PDT
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.
Comment 13 Mats Palmgren (:mats) 2011-12-27 03:35:53 PST
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!

Note You need to log in before you can comment on or make changes to this bug.