Bug 46026 - createFragmentFromText should not clone the block surrounding its context parameter to wrap paragraphs
Summary: createFragmentFromText should not clone the block surrounding its context par...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Justin Garcia
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2010-09-17 18:51 PDT by Justin Garcia
Modified: 2011-03-08 20:54 PST (History)
3 users (show)

See Also:


Attachments
work in progress (4.96 KB, patch)
2010-09-17 18:55 PDT, Justin Garcia
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Garcia 2010-09-17 18:51:16 PDT
createFragmentFromText uses its Range* parameter to determine the kind of block that will be used to wrap paragraphs of text.  This is bad because the cloned blocks have the same id as the original and because there is no straightforward way to get it to use a particular kind of block/style.

We should write a new function, possibly:

PassRefPtr<DocumentFragment> createFragmentFromText(Document* document, CSSStyleDeclaration* style, const QualifiedName& tagName, const String& text)

that requires callers to be specific about the kind of block they want to use.

<rdar://problem/8446956>
Comment 1 Justin Garcia 2010-09-17 18:55:33 PDT
Created attachment 67992 [details]
work in progress

I wrote the current function and the old one in term of a new internal function that takes in a Document* and a block to clone.
Comment 2 Ryosuke Niwa 2010-09-17 20:03:29 PDT
Comment on attachment 67992 [details]
work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=67992&action=prettypatch

> WebCore/editing/markup.cpp:1280
> +            // For last line, use the "magic BR" rather than a P.

Since we're wrapping with blockToClone instead of a P, we might want to revise this comment.

> WebCore/editing/markup.cpp:1350
> +    return createFragmentFromText(document, useClonesOfEnclosingBlock ? block : createDefaultParagraphElement(document).get(), text);

Since useClonesOfEnclosingBlock is only used once, can we change the block to RefPtr<Element> and then do element = createDefaultParagraphElement(document) instead of doing trinary here?  i.e.
RefPtr<Element> block = static_cast<Element*>(blockNode);
if (blockNode && ...)
    block = createDefaultParagraphElement(document);

Otherwise looks nice.  But as you said, we should figure out a way to test this.