Closed Bug 703514 Opened 13 years ago Closed 12 years ago

can't drag bookmarklet to toolbar from real xhtml page

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 23

People

(Reporter: mkmelin, Assigned: MatsPalmgren_bugz)

References

(Depends on 1 open bug)

Details

(Keywords: regression, xhtml, Whiteboard: [fixed by bug 723163])

Attachments

(1 file, 3 obsolete files)

Attached file testcase (deleted) —
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->
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
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
(I'm on vacation next week - feel free to steal it from me ;-) )
Keywords: xhtml
Attached file Minimal HTML5 testcase (obsolete) (deleted) —
Also happens with HTML(5) now, depending on the markup structure.
Attached file Minimal HTML5 testcase (obsolete) (deleted) —
Testcase re-uploaded with correct content type
Attachment #596427 - Attachment is obsolete: true
Attachment #596429 - Attachment mime type: text/plain → text/html
Attached file Minimal HTML5 testcase (obsolete) (deleted) —
Attachment #596429 - Attachment is obsolete: true
Uh, obviously bugzilla doesn't like HTML attachments, can only upload as plain text, sorry :(
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?
(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?
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
(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.
Attachment #596430 - Attachment is obsolete: true
Hmm, it appears to break some clipboard related about:memory tests: https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=3456edcbdb48
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.
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?
Yep, works for me in Nightly 23.0a1 (2013-04-04) on Linux. Thanks Drew!
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Keywords: qawanted
Resolution: --- → FIXED
Whiteboard: [fixed by bug 723163]
Target Milestone: --- → Firefox 23
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.
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)
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.

Attachment

General

Created:
Updated:
Size: