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)
Firefox
Bookmarks & History
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)
(deleted),
application/xhtml+xml
|
Details |
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•13 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•13 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•13 years ago
|
||
(I'm on vacation next week - feel free to steal it from me ;-) )
Also happens with HTML(5) now, depending on the markup structure.
Testcase re-uploaded with correct content type
Attachment #596427 -
Attachment is obsolete: true
Updated•13 years ago
|
Attachment #596429 -
Attachment mime type: text/plain → text/html
Attachment #596429 -
Attachment is obsolete: true
Uh, obviously bugzilla doesn't like HTML attachments, can only upload as plain text, sorry :(
Updated•13 years ago
|
Attachment #596430 -
Attachment mime type: text/plain → text/html
Comment 8•13 years ago
|
||
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•13 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•12 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.
Assignee | ||
Comment 12•12 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•12 years ago
|
Attachment #596430 -
Attachment is obsolete: true
Assignee | ||
Comment 13•12 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•12 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.
Comment 15•12 years ago
|
||
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•12 years ago
|
||
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
Comment 17•12 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.
Comment 18•12 years ago
|
||
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•12 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•12 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.
Description
•