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
Product: WebKit
Classification: Unclassified
Component: HTML Editing
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To: Justin Garcia
: InRadar
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-27 22:13 PDT by Justin Garcia
Modified: 2009-11-05 03:09 PST (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
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