Bug 6608

Summary: REGRESSION: Line disappears when deleting
Product: WebKit Reporter: Duncan Wilcox <duncan>
Component: HTML EditingAssignee: Justin Garcia <justin.garcia>
Status: RESOLVED FIXED    
Severity: Normal CC: alice.barraclough, ap, harrison
Priority: P1 Keywords: InRadar, Regression
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
patch darin: review+

Description Duncan Wilcox 2006-01-16 23:27:39 PST
When running Mail.app on TOT, do the following:

- reply to any message
- type "asdfghj" + enter
- notice two new lines appear instead of one
- arrow right, arrow right
- now the cursor is before "On <date>, <name> wrote:"
- backspace, backspace
- the whole line disappears

These might be two separate bugs, but I think the appearance of an extra line and disappearing of 
another might be related.
Comment 1 Joost de Valk (AlthA) 2006-01-22 04:37:39 PST
Adding Regression keyword.
Comment 2 Joost de Valk (AlthA) 2006-01-22 04:43:39 PST
Upping to P1 because this is a regression.
Comment 3 Alice Liu 2006-01-26 17:23:38 PST
<rdar://problem/4424044>
Comment 4 Justin Garcia 2006-03-30 06:54:41 PST
Spun the extra line appearing problem into a separate bug (8075).
Comment 5 Justin Garcia 2006-03-30 08:17:54 PST
Created attachment 7391 [details]
patch

Rewrote moveNodesAfterNode to address several problems:
It moves nodes w/o worrying about how their style might change during the move.
It traverses _siblings_ looking for a br to know that it's at the end of the paragraph to merge.  If the br is burried inside a span, it won't find it.  If the text is whitespace:pre, it won't find stop at the end of the paragraph.
It reinserts the nodes at strange places, which was causing the bug.

This patch passes all pixel tests.  Render tree diffs are 1) fixes 2) placeholder brs were there were none.  This patch inserts them more aggressively to make sure that inline content after the merged paragraph doesn't collapse onto the line where the paragraph was moved to.  I'm writing lots of layout tests to illustrate problems with the old function and thinking hard about how to break the new code.
Comment 6 Darin Adler 2006-03-30 08:35:18 PST
Comment on attachment 7391 [details]
patch

Are you guaranteed that removing a node won't cause it's parent to be deref'd? If not, the Node* might need to be RefPtr<Node>.

+    void removeNodeAndPruneAncestors(WebCore::Node* node);
+    void prune(WebCore::Node* node);

No need for WebCore:: prefixes here.

r=me
Comment 7 Justin Garcia 2006-04-04 13:31:40 PDT
(In reply to comment #6)
> (From update of attachment 7391 [details] [edit])
> Are you guaranteed that removing a node won't cause it's parent to be deref'd?
> If not, the Node* might need to be RefPtr<Node>.

I guess a removal listener on the node to remove could remove/deref the parent.  There are lots of places in the editing code where the possibility is ignored, of course.