Bug 25439 - Deleting when in front of a block image removes character from previous paragraph
: Deleting when in front of a block image removes character from previous parag...
Status: RESOLVED FIXED
: WebKit
HTML Editing
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
: InRadar
:
:
  Show dependency treegraph
 
Reported: 2009-04-27 22:13 PST by
Modified: 2009-11-05 03:09 PST (History)


Attachments
patch (11.54 KB, patch)
2009-11-02 02:47 PST, Justin Garcia
no flags Review Patch | Details | Formatted Diff | Diff
patch (12.46 KB, patch)
2009-11-03 20:39 PST, Justin Garcia
adele: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-04-27 22:13:56 PST
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 From 2009-04-27 22:22:28 PST -------
<rdar://problem/6767162>
------- Comment #2 From 2009-11-02 02:47:31 PST -------
Created an attachment (id=42310) [details]
patch

patch and layout tests
------- Comment #3 From 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 From 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 From 2009-11-03 00:41:49 PST -------
(From update of attachment 42310 [details])
> +    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 From 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 From 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 From 2009-11-03 18:27:01 PST -------
> I think it is better to give the test a meaningful name.

Will fix.
------- Comment #9 From 2009-11-03 20:39:46 PST -------
Created an attachment (id=42445) [details]
patch

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