Bug 25439

Summary: Deleting when in front of a block image removes character from previous paragraph
Product: WebKit Reporter: Justin Garcia <justin.garcia>
Component: HTML EditingAssignee: Justin Garcia <justin.garcia>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, enrica, eric, rniwa
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
patch
none
patch adele: review+

Description Justin Garcia 2009-04-27 22:13:56 PDT
Use:

<div contentEditable="true">hello<img style="display:block;" src="http://www.google.com/logos/logo.gif"></div>

Put the caret in front of the image and hit delete.  Caret should probably just move back one, instead of deleting a character from the previous paragraph.
Comment 1 Justin Garcia 2009-04-27 22:22:28 PDT
<rdar://problem/6767162>
Comment 2 Justin Garcia 2009-11-02 02:47:31 PST
Created attachment 42310 [details]
patch

patch and layout tests
Comment 3 Adele Peterson 2009-11-02 11:47:18 PST
With the test case, I see the image disappear, not a character from the previous line.
Comment 4 Justin Garcia 2009-11-03 00:05:19 PST
Ryosuke pointed out that the layout tests were missing a newline at the end.  Fixed that.
Comment 5 Ryosuke Niwa 2009-11-03 00:41:49 PST
Comment on attachment 42310 [details]
patch

> +    if (isRenderedAsNonInlineTableImageOrHR(startOfParagraphToMove.deepEquivalent().node()) && !isStartOfParagraph(mergeDestination)) {
> +        m_endingPosition = m_upstreamStart;
> +        return;

Why do we care whether the destination is the start of paragraph or not?

> +bool isRenderedAsNonInlineTableImageOrHR(const Node* node)
> +{
> +    if (!node)
> +        return false;
> +    RenderObject* renderer = node->renderer();
> +    return renderer && ((renderer->isTable() && !renderer->isInline()) || renderer->isImage() && !renderer->isInline() || renderer->isHR());
...
> -static bool renderedAsNonInlineTableOrHR(RenderObject* renderer)
> -{
> -    return renderer && ((renderer->isTable() && !renderer->isInline()) || renderer->isHR());
...
> -    if (renderedAsNonInlineTableOrHR(startNode->renderer()) && p.atLastEditingPositionForNode())
> +    if (isRenderedAsNonInlineTableImageOrHR(startNode))

Why are we removing the condition that p.atLastEditingPositionForNode()?
Comment 6 Enrica Casucci 2009-11-03 11:45:31 PST
I thought we were moving away from naming the tests after the bug number. I think it is better to give the test a meaningful name.
Comment 7 Justin Garcia 2009-11-03 18:26:31 PST
> Why do we care whether the destination is the start of paragraph or not?

If it is at the start of a paragraph, then the block table, image, or hr can move up (we can merge).  I will add a comment about this.

> Why are we removing the condition that p.atLastEditingPositionForNode()?

Because firstDeepEditingPositionForNode(startNode) is also the start of the paragraph if p.atFirstEditingPositionForNode().
Comment 8 Justin Garcia 2009-11-03 18:27:01 PST
> I think it is better to give the test a meaningful name.

Will fix.
Comment 9 Justin Garcia 2009-11-03 20:39:46 PST
Created attachment 42445 [details]
patch

updated patch
Comment 10 Justin Garcia 2009-11-05 03:09:57 PST
http://trac.webkit.org/changeset/50542