can't drag bookmarklet to toolbar from real xhtml page

VERIFIED FIXED in Firefox 23

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: Magnus Melin, Assigned: mats)

Tracking

(Depends on: 1 bug, {regression, xhtml})

unspecified
Firefox 23
regression, xhtml
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed by bug 723163])

Attachments

(1 attachment, 3 obsolete attachments)

170 bytes, application/xhtml+xml
Details
(Reporter)

Description

6 years ago
Created attachment 575403 [details]
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

6 years ago
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
Blocks: 39098
(Assignee)

Comment 2

6 years ago
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;
Assignee: nobody → matspal
(Assignee)

Comment 3

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

Updated

6 years ago
Keywords: xhtml

Comment 4

6 years ago
Created attachment 596427 [details]
Minimal HTML5 testcase

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

Comment 5

6 years ago
Created attachment 596429 [details]
Minimal HTML5 testcase

Testcase re-uploaded with correct content type
Attachment #596427 - Attachment is obsolete: true

Updated

6 years ago
Attachment #596429 - Attachment mime type: text/plain → text/html

Comment 6

6 years ago
Created attachment 596430 [details]
Minimal HTML5 testcase
Attachment #596429 - Attachment is obsolete: true

Comment 7

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

Updated

6 years ago
Attachment #596430 - Attachment mime type: text/plain → text/html
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?
(Assignee)

Updated

6 years ago
Duplicate of this bug: 751778

Comment 10

6 years ago
(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?
(Assignee)

Comment 11

6 years ago
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/mpalmgren@mozilla.com-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.
Keywords: qawanted
OS: Windows 7 → All
Hardware: x86 → All
(Assignee)

Comment 12

6 years ago
(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.
(Assignee)

Updated

6 years ago
Attachment #596430 - Attachment is obsolete: true
(Assignee)

Comment 13

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

Comment 14

6 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 857915
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?
(Assignee)

Comment 16

5 years ago
Yep, works for me in Nightly 23.0a1 (2013-04-04) on Linux.  Thanks Drew!
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Keywords: qawanted
Resolution: --- → FIXED
Whiteboard: [fixed by bug 723163]
Target Milestone: --- → Firefox 23

Comment 17

5 years ago
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.
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.
(Assignee)

Comment 19

5 years ago
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.
Flags: needinfo?(bogdan.maris)
(Reporter)

Comment 20

5 years ago
Yup, verified this now works on trunk.
Status: RESOLVED → VERIFIED
Flags: needinfo?(bogdan.maris)
You need to log in before you can comment on or make changes to this bug.