Bug 4726 – Drop of multiple non-image file URLs only yields one item

• Component: HTML Editing
• Status: RESOLVED FIXED

[Dan Wood]

When you select multiple non-image files in the finder and drag them into an ContentEditable div, only 
the first file's URL in that selection is inserted.

To reproduce:
* Activate an editable div in a web page
* Select 2 or more non image files (e.g. txt, rtf, etc, from the finder)
* Drag into the editable area

Expected:
You should see file: URLs inserted for each item
Actual:
Only one URL is inserted.

Notes:
I did some looking into the logic of this by breaking on _documentFragmentFromPasteboard:.... in 
WebHTMLView.m.  One of the first checks is:
([types containsObject:NSFilenamesPboardType])
If this returns true (which it does, it calls:

        DOMDocumentFragment *fragment = [self _documentFragmentWithPaths:[pasteboard 
propertyListForType:NSFilenamesPboardType]];

Now  you'd think that this would be the one to handle the pasteboard contents, but actually, -
[WebHTMLView _documentFragmentWithPaths:] only heeds image files.

Way down further in _documentFragmentFromPasteboard:..., as almost a last resort, it checks to see if 
the pasteboard contains a single URL with this line:  if ((URL = [NSURL 
URLFromPasteboard:pasteboard]))

That's the code that succeeds in processing the pasteboard, but it only yields a single URL.

I would propose that another check be inserted above that which again checks if ([types 
containsObject:NSFilenamesPboardType])  but this time, it processes all the file paths as URLs.  that 
would allow multiple files to be dragged in.

[Andrew Wellington]

Created attachment 4696 [details]
Proposed patch

If no images are found in the dragged file list we instead get a list of the
URLs of the file list (space separated) and use that.

No regression test due to difficulty testing a pasteboard containing
NSFilenamesPboardType. (Is it even possible to test that automatically?)

[Maciej Stachowiak]

Comment on attachment 4696 [details]
Proposed patch

Looks like a generally good fix. But, two comments.

In TextEdit, the paths end up separated by newlines, not spaces. So it would be
better to use "\n" as the separator instead of " ", for consistency. Second,
what happens if you have a mix of image and non-image URLs? Seems to me like
with this code, you'd lose the non-image paths still.

And finally, as a matter of style, shouldn't the support for non-image types be
right in _documentFragmentWithPaths:, instead of in a separate else case? That
would also make it easier to handle a mix of image and non-image types.

Great first cut, please make these revisions and it will be read to land I
think.

[Andrew Wellington]

Created attachment 4710 [details]
Proposed patch 2

- Moved code to _documentFragmentWithPaths
- Now places a new line between URLs
- Can include both images and other files in a single drag

[Darin Adler]

Comment on attachment 4710 [details]
Proposed patch 2

For multiple lines in HTML we use p elements, not br elements.

So this code needs to be more like createFragmentFromText in markup.cpp.

Each item should be in a paragraph created by createDefaultParagraphElement.
Image items could be image elements and text items could be text nodes just as
in the code here.

A clean way to do this is to put a function over on the WebCore side that takes
a list of nodes and puts each one in a separate paragraph. The code on this
side could just pass an NSArray of DOMNode objects over the bridge to get to
that side.

Having more of the code on the WebCore side is better for portability to other
platforms anyway.

Sorry to make this harder, but we really want functions like this to be
consistent with other editing code.

[Andrew Wellington]

Created attachment 4781 [details]
Proposed patch 3

Now generates a document fragment containing pargraphs made using
createDefaultParagraphElement in WebCore using an NSArray passed from WebKit.

[Darin Adler]

Comment on attachment 4781 [details]
Proposed patch 3

This looks great! Excellent work.

A few comments:

    1) Formatting nit:

+    while ((node = [nodeEnum nextObject]))
+    {

In our coding style, the brace is supposed to be on the same line as the while
statement.

    2) The node should be added to the paragraph with appendChild, not with
addChild. The addChild function is for the HTML parser only.

    3) Ideally, the body of this function should be inside markup.cpp -- to do
that the NSArray would have to be converted to some C++ data structure,
presumably a QPtrList<DOM::NodeImpl>. While this would make this patch more
complicated, it would reduce the amount of code that's in Mac OS X specific
Objective-C and make some other improvements practical as well (read on).

    4) This function is different from createFragmentFromText in one
significant way: createFragmentFromText uses a "magic BR" at the end. I
remember precisely why that's necessary (something related to editing of
course), but I think it may be important in this case as well as that one.
Ideally, we'd refactor createFragmentFromText into two pieces, and then code to
put the nodes into paragraphs can be shared by this new function and by
createFragmentFromText. Thus createFragmentFromText would create the text nodes
and put them in a QPtrList and then call the new createFragmentfromNodeList
function.

    5) The fragment returned from createDocumentFragment needs to be ref'd and
deref'd at the appropriate time; it's not legal to just call appendChild on
something newly created and floating; that could cause the fragment to be
deleted. Either use a SharedPtr<DocumentFragmentImpl> or put in explicit
ref/deref calls.

[Andrew Wellington]

Created attachment 4814 [details]
Proposed patch 4

- Correct coding style problem with while

- Moved the actual fragment creation to markup.cpp in
createFragmentFromNodeList. Does not incorporate with createFragmentFromText
for now as createFragmentFromText doesn't create straight nodes, but uses
createParagraphContentsFromString to fill in a paragraph object that has
already been created. This means that while we have DOMNodes from
_documentFragmentWithPaths in WebKit passed across the bridge and moved to a
QPtrList<DOM::NodeImpl>, we'd have QStrings or a full paragraph (which we don't
want to wrap in another paragraph) from createParagraphContentsFromString,
creating different required code paths for the input from each routine.

- Correctly ref/deref elements and fragment while we construct the fragment.
This pattern of ref/deref matches createFragmentFromText

- Add a magic BR to the end of the fragment (this appears to be how
createFragmentFromText does it, simply appending the magic BR to the end of the
list). It looks OK in the DOM tree in Safari.

[Darin Adler]

Comment on attachment 4814 [details]
Proposed patch 4

Very nice!

Two comments:

    1) I feel like an idiot. Now that I read it I see that
createFragmentFromText puts each element inside a default paragraph except for
the last line if it is empty. So the magic "BR" code in
createFragmentFromNodeList is probably not needed at all. I'm really sorry
about leading you astray about this; we should probably just take that out.

    2) createFragmentFromNodeList should take a const QPtrList<DOM::NodeImpl>&
rather than QPtrList<DOM::NodeImpl>*.

Despite my marking it review-, this patch is in great shape. Thanks for all the
hard work on this.

[Darin Adler]

(In reply to comment #7)
> - Moved the actual fragment creation to markup.cpp in
> createFragmentFromNodeList. Does not incorporate with createFragmentFromText
> for now as createFragmentFromText doesn't create straight nodes, but uses
> createParagraphContentsFromString to fill in a paragraph object that has
> already been created. This means that while we have DOMNodes from
> _documentFragmentWithPaths in WebKit passed across the bridge and moved to a
> QPtrList<DOM::NodeImpl>, we'd have QStrings or a full paragraph (which we don't
> want to wrap in another paragraph) from createParagraphContentsFromString,
> creating different required code paths for the input from each routine.

The createParagraphContentsFromString function is used only inside createFragmentFromText, so it's logically part of the 
createFragmentFromText function. We could easily change it so that it creates a node. It can create a fragment, which is a single 
node that can represent a list of nodes, or the single block placeholder element. Then those nodes could be put into a QPtrList. So 
in the long run I do think it's practical to make createFragmentFromText use createFragmentFromNodeList.

[Andrew Wellington]

Created attachment 4825 [details]
Proposed patch 5

- Removed magic BR code
- Now takes const QPtrList<DOM::NodeImpl>& instead of QPtrList<DOM::NodeImpl>*

As to createFragmentFromText, I'm not sure what the benefit would be to create
a fragment instead of a paragraph, only to place that fragment in a paragraph
in a different routine? It would seem that this creates (possibly) a large
number of extra nodes that aren't required for what seems to be little benefit.


I will however attach an alternate patch in a moment that implements
createFragmentFromText through the new createFragmentFromNodeList.

[Andrew Wellington]

Created attachment 4826 [details]
Proposed patch 5 alternate

Alternate version of proposed patch 5 as mentioned in comment above.

[Darin Adler]

Comment on attachment 4825 [details]
Proposed patch 5

r=me

[Darin Adler]

Comment on attachment 4826 [details]
Proposed patch 5 alternate

This isn't really what I had in mind as far as sharing code with the new
createFragmentFromNodeList function, so we should not land this version. We can
revisit the issue later.