Cannot Drag image in xhtml

NEW
Unassigned

Status

()

Core
Drag and Drop
4 years ago
2 years ago

People

(Reporter: Pavel, Unassigned)

Tracking

({reproducible, testcase})

33 Branch
x86_64
Windows 7
reproducible, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Reporter)

Description

4 years ago
Created attachment 8530423 [details]
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

4 years ago
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?
Component: Untriaged → Untriaged
Flags: needinfo?(bzbarsky)
Keywords: reproducible, testcase
Product: Firefox → Core
Created attachment 8535025 [details]
Same testcase as XHTML
Flags: needinfo?(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

3 years ago
Created attachment 8600664 [details]
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

3 years ago
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.

Comment 6

3 years ago
(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...

Updated

3 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 7

3 years ago
Created attachment 8601499 [details] [diff] [review]
Strawman patch

I suspect it's not this easy, but just in case...  https://treeherder.mozilla.org/#/jobs?repo=try&revision=f75d2319d85c
Attachment #8601499 - Flags: feedback?(n.nethercote)
Attachment #8601499 - Flags: feedback?(bzbarsky)
Attachment #8601499 - Flags: feedback?(adw)
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.
Attachment #8601499 - Flags: feedback?(bzbarsky) → feedback+

Comment 9

3 years ago
(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

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

Updated

3 years ago
See Also: → bug 319141, bug 857915
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?
Attachment #8601499 - Flags: feedback?(n.nethercote)

Comment 12

3 years ago
(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 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.
Attachment #8601499 - Flags: feedback?(adw)
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.
Component: Untriaged → Drag and Drop
You need to log in before you can comment on or make changes to this bug.