[Webkit-unassigned] [Bug 62621] Span ID duplicated when pressing enter at beginning of span

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62621





--- Comment #3 from Annie Sullivan <sullivan at chromium.org>  2011-06-14 15:54:44 PST ---
(In reply to comment #2)
> > 2. For listing the ancestors, getAncestorsInsideBlock takes an insertion Node and an outer block Element. It excludes both these nodes from the list of ancestors, only appending the elements between them. By contrast, cloneParagraphUnderNewElement() takes a start Position and an outer Node, and includes the position.deprecatedNode() and the outer node in the list of ancestors. Swapping between using positions and using nodes, counting ancestors differently, and keeping lists of Nodes instead of 
> Elements is a bigger change in behavior. We also end up with another dependency on deprecatedNode.
> 
> The merged function should take a starting Position and the outer block node.  We may need to get rid of deprecatedNode dependency in a separate bug.

CompositeEditCommand::cloneParagraphUnderNewElement() also takes an end position (in case there are also sibling nodes to clone) and a block element (to clone the paragraph under). I think both of these are needed as well.

> > 3. cloneParagraphUnderNewElement() has no return value. cloneHierarchyUnderNewBlock() returns the deepest Element in the ancestor list that was cloned. We'd need to add this functionality into cloneParagraphUnderNewElement(), but it seems like an arbitrary return value to add.
> 
> I agree. I'm curious why we'd need that.

I think InsertParagraphSeparatorCommand is the only caller that would need it. The reason is that it inserts the actual line break by calling appendBlockPlaceholder() on that deepest cloned node. Maybe there is a better way to do this, like creating the block placeholder before cloning?

> > Do you still think that merging these functions is the right thing to do? I am working on a patch to do this, but because of the differences in functionality I'm having a lot of trouble getting the arguments to cloneParagraphUnderNewElement correct. Each time I fix a layout test, a different layout test breaks with a slightly different problem.
> 
> I think we should eventually merge these two functions, but that isn't really a requirement for you to fix other bugs.

The whole thing seems pretty fragile; the existing code calls deprecatedNode() in both places and it seems like we'd want to get rid of those before we do a bigger refactor. But I think just getting rid of IDs when cloning can be done in a much simpler change that doesn't require any refactoring.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.