Bug 19644 - Text copied with Select All pastes with a indent but shouldn't
Summary: Text copied with Select All pastes with a indent but shouldn't
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Justin Garcia
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-06-17 17:09 PDT by Justin Garcia
Modified: 2009-04-21 12:21 PDT (History)
1 user (show)

See Also:


Attachments
patch (21.96 KB, patch)
2009-04-20 23:20 PDT, Justin Garcia
darin: 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 2008-06-17 17:09:28 PDT
Open FCKEditor http://www.fckeditor.net/demo (this also happens in a few others)
Select all of the default content
Delete
Paste

Content pastes indented slightly.  It shouldn't.  Also, press return and paste again.  You'll see that the content is indented even more.

<rdar://problem/5581805> Text copied with Select All pastes with a indent but shouldn't
Comment 1 Justin Garcia 2009-04-20 23:20:10 PDT
Created attachment 29641 [details]
patch
Comment 2 Darin Adler 2009-04-21 07:53:58 PDT
Comment on attachment 29641 [details]
patch

Ideally I'd also like to see a check if the destination already has the properties in question. For example, the same background color or same image.

> +        if (fullySelectedRootStyle->getPropertyCSSValue(CSSPropertyBackgroundImage) ||
> +            fullySelectedRootStyle->getPropertyCSSValue(CSSPropertyBackgroundColor) ||
> +            static_cast<Element*>(fullySelectedRoot)->hasAttribute(backgroundAttr))

I find this if statement really difficult to read. I would prefer that you put it in a separate function or format it differently.

I don't understand why the cast to Element* here is safe. What guarantees that fullySelectedRoot is an Element, not another kind of Node?

Given that we're mutating the DOM here, maybe fullySelectedRoot should be a RefPtr<Node>, not just a Node*.

r=me
Comment 3 Justin Garcia 2009-04-21 11:13:01 PDT
> I don't understand why the cast to Element* here is safe. What guarantees that
> fullySelectedRoot is an Element, not another kind of Node?

Currently it must be the body element, but since that's non obvious maybe I'll add a check.

> Given that we're mutating the DOM here, maybe fullySelectedRoot should be a
> RefPtr<Node>, not just a Node*.

Are we?  Does attribute checking fire listeners that can modify the DOM, or?
Comment 4 Justin Garcia 2009-04-21 12:21:01 PDT
http://trac.webkit.org/changeset/42722