1106160 - Cannot Drag image in xhtml

Status: NEW

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[Pavel]

Attachment: imageDrag.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.65 Safari/537.36

Steps to reproduce:

example of html that demonstrates how to drag and drop an image.
Now change the file extension into xhtml.
Try to drag the image.


Actual results:

Image is not draggable


Expected results:

It should have worked the same way how it works in html

Comment 1

[:Gijs (he/him)]

So the draggable attribute is only in the HTML5 spec and not in the XHTML spec... but it still surprises me that this breaks, especially because selecting the image and then dragging it works (and fires the relevant events, it seems), and because AIUI <a> and <img> tags should be draggable by default, without even needing the draggable attribute.

Boris, what am I missing?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Same testcase as XHTML

Comment 3

[Boris Zbarsky [:bzbarsky]]

nsContentUtils::ContentIsDraggable returns true for the image in both testcases.

What's going wrong most immediately is that in DragDataProducer::Produce we do this:

683     rv = transferable->GetTransferData(kUnicodeMime, getter_AddRefs(supports),
684                                        &dataSize);

and get null for "supports".  That's because the transferable has an empty mDataArray, whereas in the text/html case it has 4 elements in it, of flavors "text/html", "text/_moz_htmlcontext", "text/_moz_htmlinfo", and "text/x-moz-url-priv".

OK, so where is all that stuff coming from?  From SelectionCopyHelper, called from nsCopySupport::GetTransferableForNode, called from DragDataProducer::Produce.

This starts messing with document encoders and serializers, which is all very different for HTML and XHTML.  And it keeps checking MIME types all over as it goes.  I didn't dig into the details, but the failure is clearly somewhere in there.

Comment 4

[John A. Bilicki III]

Attachment: xhtml_image_drag_and_drop.xhtml

A fully self-contained drag-and-drop test case served as XHTML (application/xhtml+xml). I will post the same as text/html.

Comment 5

[John A. Bilicki III]

Attachment: xhtml_image_drag_and_drop.html

The same exact code though as HTML (text/html).

Could someone please simply add the mime to the array and see if it works? I imagine this bug is simply an oversight of XHTML.

Comment 6

[:Gijs (he/him)]

(In reply to John A. Bilicki III from comment #5)
> Created attachment 8600665 [details]
> xhtml_image_drag_and_drop.html
> 
> The same exact code though as HTML (text/html).
> 
> Could someone please simply add the mime to the array and see if it works? I
> imagine this bug is simply an oversight of XHTML.

It's likely not that simple, unfortunately, based on comment #3 and some of what hg blame suggests...

Comment 7

[:Gijs (he/him)]

Attachment: Strawman patch

I suspect it's not this easy, but just in case...  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f75d2319d85c

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8601499 [details] [diff] [review]
Strawman patch

So what does this end up putting on the clipboard?  Does it end up putting HTML on there or XHTML?

In any case, this seems possibly-ok to me at first glance.... but you may want ehsan to take a look.  Or someone who has time to sit down and sort through the document encoder behavior if this new codepath is now followed.

Comment 9

[:Gijs (he/him)]

(In reply to Not doing reviews right now from comment #8)
> Comment on attachment 8601499 [details] [diff] [review]
> Strawman patch
> 
> So what does this end up putting on the clipboard?  Does it end up putting
> HTML on there or XHTML?
> 
> In any case, this seems possibly-ok to me at first glance.... but you may
> want ehsan to take a look.  Or someone who has time to sit down and sort
> through the document encoder behavior if this new codepath is now followed.

I've not tried the clipboard; I just tried the dragging, and that works with this change. I'm kinda hoping that the tests (some of adw's changes that moved this check are to do with XHTML behaviour, and they had test changes) would catch significant changes here. Perhaps that is overly optimistic. :-\

Comment 10

[:Gijs (he/him)]

(indeed, there are test failures in mochitest-1, at least...)

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8601499 [details] [diff] [review]
Strawman patch

Review of attachment 8601499 [details] [diff] [review]:
-----------------------------------------------------------------

I have nothing useful to say about this patch; it's in part of the code I'm totally unfamiliar with. Did you mean to ask somebody else for feedback?

Comment 12

[:Gijs (he/him)]

(In reply to Nicholas Nethercote [:njn] from comment #11)
> Comment on attachment 8601499 [details] [diff] [review]
> Strawman patch
> 
> Review of attachment 8601499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have nothing useful to say about this patch; it's in part of the code I'm
> totally unfamiliar with. Did you mean to ask somebody else for feedback?

Sorry, I was going by an author + reviewer for http://hg.mozilla.org/mozilla-central/rev/a215de599e7f - looking again, I guess that was about:memory stuff, and so I should have picked :hsivonen...

It looks like the patch is overly simplistic either way though...

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 8601499 [details] [diff] [review]
Strawman patch

Review of attachment 8601499 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay, but I won't be much help either.  I don't know this code very well, and it's been a while, and looking at http://hg.mozilla.org/mozilla-central/diff/a215de599e7f/content/base/src/nsDocumentEncoder.cpp, I didn't actually add the mDocument->IsHTML() (now IsHTMLDocument()) check.  I only moved it from a different place.  So I can't say if this is the right thing or not.  If it doesn't break copying though, I would think that's pretty good evidence that it is OK.

Right, hsivonen reviewed this part in the other bug.

Comment 14

[Paul Pasca[Away. Please needinfo? Paul Oiegas [:pauloiegasSV]]]

I'm still able to reproduce this on the latest release(43.0.3) and latest Nightly(46.0a1).

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151223140742

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160104030217

If anyone considers that the component is not the right one, please change it to a more appropriate one.

Comment 15

[Franklin Tse]

This remains an active issue in Firefox 64 and may have already been reported in https://bugzilla.mozilla.org/show_bug.cgi?id=751778