703514 - can't drag bookmarklet to toolbar from real xhtml page

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Magnus Melin [:mkmelin]]

Attachment: testcase

Can't drag a bookmarklet to the personal toolbar if the page the bookmarklet is on is an xhtml page (application/xhtml+xml). Works fine if it's a normal html page.

This gets output to the console

Error: dragged is undefined
Source File: chrome://browser/content/places/controller.js
Line: 1437

This used to work at least a year/two back. Broken at least ff v7->

Comment 1

[Alice0775 White]

Regression window
Works:
http://hg.mozilla.org/mozilla-central/rev/714faf8ec149
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100524 Minefield/3.7a5pre ID:20100524162132
Fails:
http://hg.mozilla.org/mozilla-central/rev/2e4105f85be9
Mozilla/5.0 (Windows; U; Windows NT 6.1; WOW64; en-US; rv:1.9.3a5pre) Gecko/20100524 Minefield/3.7a5pre ID:20100524172122
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=714faf8ec149&tochange=2e4105f85be9
Triggered by:
0c440c656ada	Mats Palmgren — Bug 39098 - Elements with visibility:hidden, visibility:collapse, or display:none get copied to the clipboard. r=dbaron

Comment 2

[Mats Palmgren (inactive)]

It looks like the old code unconditionally used a HTML encoder, whereas
the new code goes through nsCopySupport which uses nsCopySupport::IsPlainTextContext
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCopySupport.cpp#436
which results in 'bIsHTMLCopy' being true for HTML and false for XTHTML.

It works with this change:

   // also consider ourselves in a text widget if we can't find an html
   // document. Note that XHTML is not counted as HTML here, because we can't
   // copy it properly (all the copy code for non-plaintext assumes using HTML
   // serializers and parsers is OK, and those mess up XHTML).
   nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(aDoc);
-  if (!(htmlDoc && aDoc->IsHTML()))
+  if (!(htmlDoc))
     *aIsPlainTextContext = true;

Comment 3

[Mats Palmgren (inactive)]

(I'm on vacation next week - feel free to steal it from me ;-)  )

Comment 4

[T.Rosenau]

Attachment: Minimal HTML5 testcase

Also happens with HTML(5) now, depending on the markup structure.

Comment 5

[T.Rosenau]

Attachment: Minimal HTML5 testcase

Testcase re-uploaded with correct content type

Comment 6

[T.Rosenau]

Attachment: Minimal HTML5 testcase

Comment 7

[T.Rosenau]

Uh, obviously bugzilla doesn't like HTML attachments, can only upload as plain text, sorry :(

Comment 8

[Boris Zbarsky [:bzbarsky]]

Uh... uploading as HTML should work fine.  I do it all the time.  But I've seen a lot of people run into this problem recently.  Can you reproduce the problem on https://landfill.bugzilla.org/bugzilla-4.2-branch/ ?  Please mail me privately so we can figure out what's going on with that?

Comment 10

[John A. Bilicki III]

(In reply to Mats Palmgren [:mats] from comment #2)
> It looks like the old code unconditionally used a HTML encoder, whereas
> the new code goes through nsCopySupport which uses
> nsCopySupport::IsPlainTextContext
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCopySupport.
> cpp#436
> which results in 'bIsHTMLCopy' being true for HTML and false for XTHTML.
> 
> It works with this change:
> 
>    // also consider ourselves in a text widget if we can't find an html
>    // document. Note that XHTML is not counted as HTML here, because we can't
>    // copy it properly (all the copy code for non-plaintext assumes using
> HTML
>    // serializers and parsers is OK, and those mess up XHTML).
>    nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(aDoc);
> -  if (!(htmlDoc && aDoc->IsHTML()))
> +  if (!(htmlDoc))
>      *aIsPlainTextContext = true;

If you folks have the fix why hasn't it been implemented?

Comment 11

[Mats Palmgren (inactive)]

Just because the suggested change fix the reported problem doesn't mean
it's a correct fix that won't cause new problems for other use cases.
As the comment in the code says, it's *intentionally* not treating XHTML
documents as HTML documents here, which is a strong indication that there
would be a problem if it did.  That said, perhaps fixing this regression
trumps any concerns regarding XHTML copy-correctness.

I've pushed the change to the Try server, you can help getting this bug
fixed by testing the builds here:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3456edcbdb48/

I'm particularly interested in if there's any change of copy-paste or
drag-n-drop of various XHTML content into text/html/xhtml contexts,
especially to native applications on Tier-1 platforms.

Comment 12

[Mats Palmgren (inactive)]

(In reply to T.Rosenau from comment #4)
> Also happens with HTML(5) now, depending on the markup structure.

That is an unrelated problem.  I filed bug 758997 for it.

Comment 13

[Mats Palmgren (inactive)]

Hmm, it appears to break some clipboard related about:memory tests:
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=3456edcbdb48

Comment 14

[John A. Bilicki III]

Local test of Firefox 3.6 shows that when an element (in example an image) is dragged and dropped it is duplicated if the page is served as application/xhtml+xml though merely moved (correct action) is the page is served as text/html.

Comment 15

[Drew Willcoxon :adw]

Actually it looks like bug 723163 fixed this.  The text/plain flavor apparently is enough, no text/html (and therefore bug 857915) needed.

Mats, John, can you confirm?

Comment 16

[Mats Palmgren (inactive)]

Yep, works for me in Nightly 23.0a1 (2013-04-04) on Linux.  Thanks Drew!

Comment 17

[John A. Bilicki III]

I'm on Windows 7 64 / Firefox 17.0.0.5 ESR. I dragged the tab of a page from my site (always XHTML as application/xhtml+xml) to the bookmarks toolbar and it created a bookmark. If that is what this bug was about then works-for-me. I vaguely recall posting bug #751778, I will try to confirm that bug either still broken of fixed later tomorrow as the issue I mentioned was not related to bookmarks.

Comment 18

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20100101 Firefox/21.0

Tested on Firefox 21 beta 1 (buildID: 20130401192816).

Using the simple test-case from bug 723163 I am able to drag intro bookmark toolbar, open in new tab. Using the test-case provided in this bug I`m not able to complete those actions. 
So the status of the bug can be moved to Verified if the test-case provided in bug 723163 has reference to this bug.

Comment 19

[Mats Palmgren (inactive)]

Bogdan, please use the testcase in this bug for verifying.  You should open it
and try to drag the "alert foo" link into the bookmark toolbar.
It should fail in Firefox 21, because bug 723163 is not fixed there.
It should succeed in a current Nightly 23.0a1.

Comment 20

[Magnus Melin [:mkmelin]]

Yup, verified this now works on trunk.