[Webkit-unassigned] [Bug 61594] REGRESSION: Hitting enter in the middle of this span causes the cursor to go to the end of the span

[bugzilla-daemon at webkit.org]

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





--- Comment #8 from Annie Sullivan <sullivan at chromium.org>  2011-06-03 19:58:51 PST ---
(In reply to comment #6)
> (From update of attachment 96004 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96004&action=review
> 
> > Source/WebCore/editing/CompositeEditCommand.cpp:1250
> > +        // If the position is at the end of the node, split at the next node.
> > +        if ((p.containerNode()->isTextNode() && p.offsetInContainerNode() == (int)static_cast<Text*>(p.containerNode())->length())
> > +            || (p.containerNode()->isElementNode() && p.offsetInContainerNode() == (int)splitNode->childNodes()->length()))
> > +            splitNode = splitNode->nextSibling();
> 
> This logic is likely wrong as it doesn't take invisible nodes into account.

You're right that I didn't take invisible nodes into account at all when writing this code. By invisible, do you mean display:none or something else? Is there a case I should add as a layout test?

(In reply to comment #7)
> Thanks for working on this bug.  I'm excited to see a progress.
> 
> (In reply to comment #5)
> > I wasn't sure whether to implement a new splitTreeToPosition method, or modify splitTreeToNode to take either. Please let me know what you think.
> 
> We should modify splitTreeToNode to take a Position instead of adding a new function.

Sounds good. I rewrote as a separate function to start with so that it's easier to see what I'm trying to do and point out mistakes in early review. I also wasn't clear on what the arguments should be--it could take a node AND a position, expecting one of them to be null, or just take a position, expecting that in the common case the position would be at the start of the node.

> > splitTreeToPosition currently doesn't handle splitting nodes if the position is not at the start or end of the node. Currently InsertParagraphSeparatorCommand is the only caller, and it never creates a case like that because it splits text nodes before calling splitTreeTo(Node|Position).
> 
> It worries me to no end that we're adding a brand new function for dealing with this very specific case whereas splitTreeToNode is a well-understood function and used in many places.

At the same time, complicating the arguments and adding one-off functionality to a simple, well-understood function used in many places seems quite dangerous, too. When I modify the function to take a position, I'll still need to answer this question about whether the position should be split, and make sure I don't introduce bugs with invisible nodes or anything else. Am I missing a less complicated way to fix this bug?

Also, it took me a bit of extra time to understand what splitTreeToNode() is supposed to do because there wasn't a unit testing framework that allowed me to pass in arbitrary nodes/dom trees and look at the output. Do you think it would be worth trying to add common editing methods like this to window.internals so that we can test them independently? (Or maybe I'm missing some existing way to do this?)

> 
> > The reason that the id is duplicated for the span "wrapper" is that InsertParagraphSeparatorCommand::cloneHierarchyUnderNewBlock() doesn't avoid duplicating ids, as SplitElementCommand::executeApply() does. Should I file a separate bug/write a separate patch for that?
> 
> Yes, please file a separate bug for that.  But maybe we should first merge that function with CompositeEditCommand::cloneParagraphUnderNewElement.  It's really terrible that we have all these functions that do slightly different things all over the place.

Okay, I'll look into that.

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