[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


Annie Sullivan <sullivan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #97678|1                           |0
        is obsolete|                            |




--- Comment #30 from Annie Sullivan <sullivan at chromium.org>  2011-06-21 18:24:43 PST ---
(From update of attachment 97678)
View in context: https://bugs.webkit.org/attachment.cgi?id=97678&action=review

>> Source/WebCore/ChangeLog:16
>> +            Implement new function. Like splitTreeToNode(), but picks start node based on position so that text nodes are split correctly.
> 
> So we usually start sentences after "splitTree): " then wrap at wherever is appropriate.  This line is probably too long.  You should also mention that this new function replaces the old splitTreeToNode.

Done.

>> Source/WebCore/editing/CompositeEditCommand.cpp:-1235
>> -    ASSERT(start);
> 
> We should probably assert that start.containerNode is not null.

Good catch! Done.

>> Source/WebCore/editing/CompositeEditCommand.cpp:1243
>> +    // moving the start node to the beginning of the position, splitting the node if necessary.
> 
> This comment repeats what code says.  You should either remove the comment or explain why this is needed instead.

Comment removed.

>>>> Source/WebCore/editing/FormatBlockCommand.cpp:64
>>>> +    RefPtr<Node> outerBlock = (start.deprecatedNode() == nodeToSplitTo) ? start.deprecatedNode() : splitTree(firstPositionInNode(start.deprecatedNode()), nodeToSplitTo);
>>> 
>>> So you can't just pass start here?  Also, you should probably call firstPositionInOrBeforeNode instead.
>> 
>> Unfortunately, a lot of edge cases around top-level non-block elements fail here. For example, if the editable div contains only a <hr>, start.deprectatedNode() is the <hr> and start.containerNode() is the editable div. The ASSERT(start.containerNode() != end) fails, and if I take it out I end up with a crash later on.
>> 
>> I'll try firstPositionInOrBeforeNode instead.
> 
> I see. Okay. I suppose we can live with this code for now.

It turns out that calling firstPositionInOrBeforeNode trips on the same edge cases that break when I pass the position into splitTree directly--the container node of the position ends up being the editable div instead of the <hr>, and I hit the assert and it crashes.

>> Source/WebCore/editing/InsertListCommand.cpp:277
>> +    // Unless this is the first node in the list, the list needs to be split.
> 
> Again, this comment seems to repeat code.  Can we explain why this needs to be done instead?  If not, we should get rid of this comment.

comment removed.

>> Source/WebCore/editing/InsertListCommand.cpp:282
>> +    // LayoutTests/editing/execCommand/switch-list-type.html
> 
> This comment seems a bit verbose.  I'd just say "insert a placeholder to avoid texts nodes of splitAt and the node before the placeholder won't get merged"

done.

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