Closed Bug 1254980 Opened 8 years ago Closed 8 years ago

Copy-pasting selected HTML from Firefox to a new Thunderbird message (using TB 38) or to BlueGriffon is broken as of Firefox 45

Categories

(Core :: DOM: Editor, defect)

45 Branch
All
Windows
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
firefox45 blocking verified
firefox46 --- verified
firefox47 --- verified
firefox48 --- verified
firefox-esr45 --- verified
relnote-firefox --- 45+

People

(Reporter: vbr, Assigned: jorgk-bmo)

References

Details

(Keywords: regression, reproducible, Whiteboard: btpp-backlog)

Attachments

(6 files, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20160105164030

Steps to reproduce:

I try to copy a web page with pictures from firefox 45 to a new mail in thunderbird 38.6 in html mode


Actual results:

The mail does not show any pictures , only some text .
Same operation with firefox 43 and 44 is working but not with FF45.





Expected results:

It should display the pictures in the thunderbird new html mail as before FF45.
Can not use firefox 45 until this problem is not resolved.
Severity: normal → major
Has Regression Range: --- → yes
Component: Untriaged → General
Keywords: common-issue+
OS: Unspecified → Windows Vista
Priority: -- → P1
Hardware: Unspecified → x86
Please:
- provide an actual URL to test with
- explain how you're "copying a web page". Are you selecting everything on it, and if so, by mouse or by keyboard, and if not, how are you "copying" the web page?

Can you reproduce this on a clean profile? ( http://support.mozilla.org/kb/profile-manager-create-and-remove-firefox-profiles )

If possible, it would be really helpful if you could try to use http://mozilla.github.io/mozregression/ to see if you can figure out exactly when this broke.
Component: General → Untriaged
Flags: needinfo?(vbr)
Priority: P1 → --
Never mind, I can reproduce on my machine.

m-c regression window:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=45273bbed8efaface6f5ec56d984cb9faf4fbb6a&tochange=099f695d31326c39595264c34988a0f4b7cbc698
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(vbr)
OS: Windows Vista → Windows
Hardware: x86 → All
Jorg, bug 938991 is in that window, any chance you can reproduce this and/or know why it would be broken by your change?
Flags: needinfo?(mozilla)
(In reply to :Gijs Kruitbosch from comment #4)
> Jorg, bug 938991 is in that window, any chance you can reproduce this and/or
> know why it would be broken by your change?

Or bug 586587 for that matter...
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9bf9d97873fe66037bd14176d2d09affec617fe2&tochange=be19277b423fb7940380282e3f1877073c23762a

Also pinging Neil in case he has some idea about what's going wrong here.

Text with links also loses links, so it seems like pasting HTML from Firefox is just broken.

Workaround: copy/paste into internet explorer first, then copy/paste again and paste into Thunderbird.

Note that I don't know enough about the patches in these bugs to know if this is just something that Thunderbird needs to fix or something where Firefox is actively doing something that breaks Thunderbird.
Blocks: 938991, 586587
Flags: needinfo?(enndeakin)
Summary: copy-paste a page with pictures is no more working between firefox 45 and thunderbird → copy-pasting selected HTML from firefox to a new thunderbird message is broken as of Firefox 45
Component: Untriaged → Serializers
Product: Firefox → Core
More likely a a regression from 586587; probably something with the way kNativeHTMLMime and kHTMLMime are handled. Almost all of the change in that bug relate to pasting though.

Would be nice to have a more specific set of steps to reproduce and testcase.
Flags: needinfo?(enndeakin)
OK, let's get a few things straight here:
Bug 938991 has nothing to do with pasting images, it's all about pasting RTF.
The patch in bug 586587 was authored by Andrew Herron and reviewed by Neil Deakin, I finished it up and got it to land. And yes, it changed the copying and pasting of HTML (incl. images).

My observations are these:
I open a website with images in FF 45 (hey, great, already available two days after the branch day!) and copy some stuff that contains images. You can in fact copy from BMO, since some people have a picture.
I copy this into a composition of TB 46 (sorry, I'm a TB developer, so I need to use (yesterday's) Aurora). Copies just great.
I copy the same clipboard content into a composition of TB 38. Copies as plain text.

So the problem reported here is an ugly interoperability problem of copying from Gecko 45 and pasting into Gecko 38.
Flags: needinfo?(mozilla)
Further analysis:
Before bug 586587, FF 44 placed the HTML clipboard into a clipboard flavour text/html. After bug 586587 the Windows clipboard flavour CF_HTML is now used for better interoperability with for example Chrome.

TB 38 still wants to paste from flavour text/html, but that is no longer there, so it defaults to pasting the text. I'll attach two screenshots, so you can see it.
Attached image copied with FF 45
I've just copied an image from BMO using FF 45, you can see
https://secure.gravatar.com/avatar/5febf6778b9ddb7b74e0310c365caece?d=mm&size=64
(In reply to Jorg K (GMT+1) from comment #9)
> Further analysis:
> Before bug 586587, FF 44 placed the HTML clipboard into a clipboard flavour
> text/html. After bug 586587 the Windows clipboard flavour CF_HTML is now
> used for better interoperability with for example Chrome.
> 
> TB 38 still wants to paste from flavour text/html, but that is no longer
> there, so it defaults to pasting the text. I'll attach two screenshots, so
> you can see it.

What are the odds that other applications also don't support CF_HTML and only text/html ? Why aren't we supporting both?
Flags: needinfo?(mozilla)
Attached image copied with FF44
And here the same thing copied with FF 44. You can see the "text/html" flavour.

I'm not sure why copying from IE to TB 38 works better, since IE also only populates the CF_HTML. Another screenshot coming.
Attached image copied with IE
Copied with IE, still no "text/html" but copies OK into TB 38.
Flags: needinfo?(mozilla)
> What are the odds that other applications also don't support CF_HTML and
> only text/html ? Why aren't we supporting both?

The correct format for html on Windows is CF_HTML. Some applications might be using text/html if they wanted compatibility with Firefox.

If this becomes a problem, we may want to add some code to return the CF_HTML data when another application asks for text/html.
I'd still like to understand why copying from IE to TB 38 works better than copying from FF 45. As far as I can see they both supply CF_HTML. Might it be that Gecko 38 gets confused by the additional text/*moz* stuff?
My impression is that Gecko checks for kHTMLContext ("text/_moz_htmlcontext") here:
https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLDataTransfer.cpp#1228
If present, like when copying from FF45, Gecko 38 also expected the kHTMLMime "text/html" flavour, which is now no longer there in FF45.
Maybe I'm just fantasising, perhaps Neil can explain this better.
Summary: copy-pasting selected HTML from firefox to a new thunderbird message is broken as of Firefox 45 → copy-pasting selected HTML from firefox to a new thunderbird message using TB 38 is broken as of Firefox 45
If you look at the patch in bug 586587 (attachment 8690875 [details] [diff] [review]) you'll see that the code went mostly into "editor" and "widget". So let's move it from unowned Core::Serializers to unowned Core::Editor ;-(
No longer blocks: 938991
Component: Serializers → Editor
when I was working on bug 586587 I did wonder whether not having the old text/html format would cause problems. I would have preferred to keep both it and CF_HTML but that seemed difficult to achieve in the code.

Jorg I think you've hit on the reason why pre-45 gecko breaks. kHTMLContext doesn't even seem to be used for very much in the current code, I kept it around because deleting it seemed like an unnecessary change. We could get rid of it, or perhaps just change the name to something other than text/_moz_htmlcontext so that pre-45 gecko falls back to CF_HTML behaviour as it does when pasting from IE.
Hi Andrew, nice to see you here and thanks for the confirmation. So the problem is the presence of kHTMLContext without kHTMLMime which confuses the pre-45 Gecko.

Personally I think there is no need for action. When Thunderbird moves to Geck45 in a few weeks, the problem will be gone.

However, there is of course always room for improvement. Maybe you have time to suggest a patch. You've looked at the problem longer than I have so you're more familiar with what all the different flavours are for. Maybe they are used more for Mac and Linux. So far, kHTMLContext seems not only to be written but also to be read
  https://dxr.mozilla.org/mozilla-central/search?q=kHTMLContext,
so it doesn't look like it could just be removed. What is it used for? Maybe renaming it is indeed the simplest option.

Neil, can you please comment. I have a development environment set up, so I can easily create a one line change to rename text/_moz_htmlcontext to text/_moz_htmlcontext2.
Flags: needinfo?(enndeakin)
I don't think we should be changing that. The context/info is used to distinguish between a complete html document (with head, body, etc) and the fragment. It is used similarly to CF_HTML, which other platforms don't have.

If a new version of Thunderbird is coming soon that doesn't have this issue, I don't think there is a problem here at present.
Flags: needinfo?(enndeakin)
Changing it would mean that pre-45 clipboards don't overlap with post-45 clipboards; individually they would still work as they always have.

But not doing anything is definitely the best option, by the time a Firefox update is released with that fix it might already be resolved by the Thunderbird update.
Please close this when we've confirmed the Thunderbird update fixes it.
Whiteboard: btpp-backlog
TB 45 (and later) doesn't have the problem as per comment #8. I've just tried again with TB 45 since comment #8 refers to TB 46.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
BlueGriffon based on Gecko is affected too.
This is not resolved. It breaks copying into anything that uses Gecko. As mentioned, it it still broken in BlueGriffon (and heaven knows BlueGriffon 1.x won't be updating its Gecko anytime soon).

I'm guessing that it puts special Mozilla-specific types on the clipboard, and those special types have changed their format. Well if you change their format, create a separate type! Don't break compatibility with all the other Gecko products.

Or at least provide me a way that I can simply copy the source out as text/html without the extra Mozilla stuff, so that Gecko-based products will pick up the HTML correctly.
(In reply to Garret Wilson from comment #26)
> This is not resolved. It breaks copying into anything that uses Gecko. As
> mentioned, it it still broken in BlueGriffon (and heaven knows BlueGriffon
> 1.x won't be updating its Gecko anytime soon).
The bug is resolved, that means no one will look at it any more. The resolution lies in the decision not to fix the problem since most remaining Gecko products will be update soon.

Fixing bug 586587 brought many advantages (*) to maintained Gecko products like Firefox and Thunderbird, so that the incompatibility caused with previous versions is some unfortunate "collateral damage".

(Personally, I have my own opinion about BlueGriffon which I won't publicise here. But I can confirm that it doesn't appear to be well maintained.)

*) Just look at the long list of duplicates that it fixed.
What this comes down to is - yes there is a problem, but would a Gecko fix for the issue be released before all uses of the old Gecko codebase are replaced with the new Gecko. With the possible exception of BlueGriffon, I think the answer is no.
@Andrew Herron, I understand your response and it makes perfect pragmatic sense. However, you'll have to admit the implication of this policy: Gecko in no way can be recommended (if it ever could) as the basis for anyone's custom project, and must instead be related to a small group of integrated legacy applications (mostly FF and Thunderbird) that are slowly dying off. If Gecko were a viable platform for new projects, you would have some API versioning support guarantees and procedures. You're essentially admitting, "aw, no one else really builds on Gecko anyway." All very sad, but very true.
I don't work for or speak for Mozilla, so take my comments with a grain of salt.

I'm not saying nobody builds on gecko - I'm saying those who do probably update it regularly. And if they don't, they should. Whether we agree with the way this works or not, "evergreen" browsers mean compatibility issues resolve themselves as everyone is forced to upgrade.
I saw in bug 1151366 (security bug, access restricted) that Ehsan was concerned about a change that could affect BlueGriffon. So let's ask him and Daniel to tell us what they think of this bug here where the damage is already done ;-(
Flags: needinfo?(ehsan)
Flags: needinfo?(daniel)
Summary: copy-pasting selected HTML from firefox to a new thunderbird message using TB 38 is broken as of Firefox 45 → Copy-pasting selected HTML from Firefox to a new Thunderbird message (using TB 38) or to BlueGriffon is broken as of Firefox 45
(In reply to Jorg K (GMT+1) from comment #32)
> I saw in bug 1151366 (security bug, access restricted) that Ehsan was
> concerned about a change that could affect BlueGriffon. So let's ask him and
> Daniel to tell us what they think of this bug here where the damage is
> already done ;-(

(thanks for pinging me about this!)
I'm currently in a meeting and will head to Brussels right after it so I
won't be in a position to review this before monday. Is that ok for you?
Flags: needinfo?(daniel)
(In reply to Garret Wilson from comment #26)
> This is not resolved. It breaks copying into anything that uses Gecko. As
> mentioned, it it still broken in BlueGriffon (and heaven knows BlueGriffon
> 1.x won't be updating its Gecko anytime soon).

Well... BlueGriffon 2.0 is on its way (and that's why there are no
other minor releases) and uses an updated Gecko.
@glazou, that's great news! But you confuse me; just a few weeks ago you told me, "sorry, but the Windows builds are far from ready yet..." https://github.com/therealglazou/bluegriffon/issues/10#issuecomment-176223681

So are we really nearing the release of a Windows version of BG 2.0 with an updated Gecko? That would be awesome, and would indeed make this current bug irrelevant for me! Could you give us a hint as to when you expect to release the Windows version of BG 2.0? At least which quarter of the year? Or at least which year?
(In reply to Garret Wilson from comment #35)
> @glazou, that's great news! But you confuse me; just a few weeks ago you
> told me, "sorry, but the Windows builds are far from ready yet..."
> https://github.com/therealglazou/bluegriffon/issues/10#issuecomment-176223681

That was more than a month and a half ago...

> So are we really nearing the release of a Windows version of BG 2.0 with an
> updated Gecko?

Yes.

> That would be awesome, and would indeed make this current bug
> irrelevant for me! Could you give us a hint as to when you expect to release
> the Windows version of BG 2.0? At least which quarter of the year? Or at
> least which year?

Any time between now and May. It'll be ready when it's ready; sorry but I can't
be more precise.
Flags: needinfo?(daniel)
BlueGriffon would have to be based on Gecko45 or later to fix this issue.
Flags: needinfo?(ehsan)
(In reply to Jorg K (GMT+1) from comment #37)
> BlueGriffon would have to be based on Gecko45 or later to fix this issue.

Right. Confirming older versions of BlueGriffon <2.0 are impacted.
Currently rebuilding BlueGriffon 2.0 based on trunk to see how it goes.
<off topic>
I suppose you'd be hit by all the bustage we had to fix in Thunderbird, for example bug 1241764, bug 1241901 or bug 1229985, just to give a short excerpt ;-(
Perhaps you could base on Mozilla45 ESR.
</off topic>
(In reply to Jorg K (GMT+1) from comment #39)
> <off topic>
> I suppose you'd be hit by all the bustage we had to fix in Thunderbird, for
> example bug 1241764, bug 1241901 or bug 1229985, just to give a short
> excerpt ;-(
> Perhaps you could base on Mozilla45 ESR.
> </off topic>

« Being hit by a build bustage » has been my daily welcome message when I arrive
at the office for the last thirteen years trying to build standalone 3rd-party
apps embedding Gecko :-)

More seriously, I just built BlueGriffon 2.0 from a one hour old clone of
mozilla-central and I can confirm it's immune to the copy/paste bug. So the
WONTFIX is fine by me.

FWIW, I hate when such a major feature (copy/paste) is broken because it's
not understandable nor acceptable from a user's perspective.
Asking users to update is not the solution here, most users having a compatibility
issue will be in corporate environment where updating could be outside of their
hands. I know it can sound a bit rough, but it should just work, period, even if it's
we have to deal with complexity or backwards-compat or even hacks.
I would like to add some further information to this bug report.  I have looked at the clipboard contents using the free program InsideClipboard.exe from www.nirsoft.net.  When web page contents are copied to the Windows clipboard from Firefox 44, InsideClipboard reports the following formats and their positions in the clipboard:

Index 1 text/html
Index 2 HTML Format
Index 3 text/_moz_htmlcontext
Index 4 text/_moz_htmlinfo
Index 5 CF_UNICODETEXT
Index 6 CF_TEXT
index 7 text/x-moz-url-priv

When the same contents are copied to the clipboard from Firefox 45, InsideClipboard reports

Index 1 HTML Format
Index 2 HTML Format
Index 3 text/_moz_htmlcontext
Index 4 text/_moz_htmlinfo
Index 5 CF_UNICODETEXT
Index 6 CF_TEXT
index 7 text/x-moz-url-priv

Note that "text/html" is no longer present and "HTML Format" appears twice.  I wrote a program using the Windows functions GetClipboardData(), EmptyClipboard() and SetClipboardData() to read the Firefox 45 "HTML Format" data from the clipboard, erase the clipboard, and rewrite the "HTML Format" data to the clipboard.  The clipboard then contained only

Index 1 HTML Format

This data pasted correctly (i.e. the same as with Firefox 44) into Thunderbird 38.  However, it only pasted using Ctrl-V; the right-click menu in Thunderbird had the Paste operation disabled.

I wrote another program to convert the "HTML Format" data to "text/html" Unicode data so that the clipboard contained only

Index 1 text/html

This data pasted correctly into Thunderbird 38 with both Ctrl-V and the right click Paste.

Even if Firefox is no longer going to put "text/html" into the clipboard I think the anomaly with "HTML Format" needs further investigation.
You're not reporting anything new. Please look at attachment 8728453 [details] and attachment 8728451 [details].
We know that "text/html" is no longer placed onto the clipboard which causes confusion in versions before Gecko 45 when other text/_moz* are present. See comment #16 and comment #18.
As a naive developer not knowing the underlying details, my mind would be tempted to think: "I am copying from HTML. Therefore the principal content type I would expect on the clipboard would be text/html." Before removing what seems to be straightforward and basic functionality, I assume that a lot of thinking and discussion went into this decision. Could you point me to the archives of those discussions so that I and others won't accidentally ask stupid questions based upon our naive expectations? I just want to learn the reasons behind this, and I'm sure there must be a record somewhere of the decision.
It's all in bug 586587 which was about providing HTML in the JavaScript HTML5 clipboard API. There's a lot of history, the short answer is that Gecko is now more compatible with pasting HTML from other windows applications which speak "HTML Format" not "text/html". As a result of this change it was unfortunately not possible to set both old and new formats on the clipboard, which we now know breaks pre-45 Gecko.

As the primary developer of that patch I'm happy to delve deeper into all of this if anyone wants, perhaps off-list rather than clutter this bug report further.
(In reply to Jorg K (GMT+1) from comment #42)

Sorry, and thank you for your reply.  I was led astray by the extra HTML Format entry that Inside Clipboard was showing which I thought might be important but now appears to be an error.  At least I have a workaround now that re-creates "text/html" in the clipboard.
To clarify, the bug that caused this issue was to properly support the Windows html type, which has the clipboard type name "HTML Format". The old type was 'text/html' and was Mozilla-specific. (Windows does not use mime types for the clipboard.) This allowed better compatibility with Windows applications such as Outlook and Office. Given the choice, I think we can agree that this is a better path than supporting older Thunderbirds. 

But I'm wondering, Andrew, what was the reason we couldn't include the old text/html format as well for compatibility?
Flags: needinfo?(andrew.herron)
I'll admit that it could just be my lack of codebase knowledge, but the updated clipboard code means that what core thinks of as "text/html" the windows clipboard code translates to "HTML Format" for both copying and pasting. I couldn't see a way to set "text/html" at the same time even if it was just a duplicate of "HTML Format" (plus UTF8-UTF16 conversion).
Flags: needinfo?(andrew.herron)
(In reply to Neil Deakin from comment #46)

> But I'm wondering, Andrew, what was the reason we couldn't include the old
> text/html format as well for compatibility?

I had the same question. I am pretty familiar with nsCopySupport and nsHTMLDataTransfer
and the clipboard is just (big image) a set of data with associated types we can deal with
in any custom priority order. What bp289 pasted above shows that Gecko pre-45 did
correctly put both html flavors into the clipboard so is it only a question of clipboard
order? Thinking out loud, windows app assuming the clipboard index 1 being always CF_HTML
while we were putting it at index 2? Again, I'm only thinking out loud, I want to be sure
we chose the right strategy here and no other/better option remains.
(In reply to Daniel Glazman (:glazou) from comment #48)

> What bp289 pasted above shows that Gecko pre-45 did
> correctly put both html flavors into the clipboard so is it only a question
> of clipboard order?

Order doesn't really matter here, it's all about the name of the format.

With the change I made, as far as widget/windows/nsClipboard.cpp is concerned "text/html" and "HTML Format" are the same thing. This is how things need to work to normalise Gecko clipboard HTML with other windows applications, but as a result "text/html" is no longer added.

If "text/html" can be added to the clipboard at the same time, that would be ideal. I just couldn't find a way to do it.
After looking at this, I think the issue is that nsClipboard::GetFormat was changed to map text/html to 'HTML Format', but there are cases where we don't want to do that.

Copying to the clipboard on Windows is done at nsClipboard::SetupNativeDataObject and the data is retrieved at nsDataObj::GetData.

I am going to reopen this bug for further investigation. I'll take a look if I have time later today or tomorrow.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
For compatibility with other windows apps doing that made the most sense to me. I'm happy to verify that any changes you make don't break 586587.

Perhaps the code that was ensuring both text/html and 'HTML Format' were set on the clipboard pre-45 (I'm not sure where that is) could be adapted to do the same, only now it adds text/html?
If I undo
https://hg.mozilla.org/mozilla-central/diff/e1b5927361ac/widget/windows/nsClipboard.cpp#l1.12
then the following happens with a patched local build:
The "text/html" clipboard type appears on the clipboard, as well as "HTML Format".
If I copy, I can successfully copy to TB 38 and TB 45. That's the good news.

The bad news is that the JS stuff then stops working:
If I copy with the local build and paste into the test case (attachment 8593110 [details]), nothing happens.
Equally, if I drag HTML from Chrome there, nothing happens.
Also, I can't drag from Chrome to a data:text/html, <html contenteditable>.
Attached patch How about this? (v1). (obsolete) — Splinter Review
Very simple patch based on Neil's comment #50. As far as I can see, with the patch we put the "text/html" flavour onto the clipboard, but everything else works as before.

I'm just messing around, so tell me if it's not right.
Attachment #8733485 - Flags: feedback?(enndeakin)
I think this is the easiest option, but I would reverse the flag and have false be the default value.

+      else if (strcmp(aDataFlavor, kHTMLMime) == 0) {
+        // If the client wants html, maybe it's in "HTML Format".
+        format = nsClipboard::GetFormat(kHTMLMime);
+        SET_FORMATETC(fe, format, 0, DVASPECT_CONTENT, -1,
+                      TYMED_HGLOBAL);
+        if (mDataObject->QueryGetData(&fe) == S_OK)
+          *_retval = true;                 // found it!
+      }

This change that was made in bug 586587 doesn't seem necessary though after looking at this. Maybe we could investigate.
Attached patch How about this? (v2). (obsolete) — Splinter Review
Reversed like this? I don't find double negation, as in
  !dontMapHTMLMime && strcmp(aMimeStr, kHTMLMime) == 0
particularly elegant.

I can't think of a better variable name. We want to pass in whether kHTMLMime (text/html) is mapped to CF_HTML or not.

> This change that was made in bug 586587 doesn't seem necessary though after
> looking at this. Maybe we could investigate.
That was added in nsDragService.cpp. Should I see what happens without it?

But that's additional to the other change, right?
I meant:

static UINT     GetFormat(const char* aMimeStr, bool mapHTMLMime = false);


> But that's additional to the other change, right?

Actually, I think it may be needed after all. But it would be clearer if it said 'GetFormat(kNativeHTMLMime)'
(In reply to Neil Deakin from comment #56)
> I meant:
> static UINT     GetFormat(const char* aMimeStr, bool mapHTMLMime = false);
But show me where how you intend to use it.

My idea was to change as little code/calls as possible, so mostly run the 
   else if (strcmp(aMimeStr, kNativeHTMLMime) == 0 ||
            strcmp(aMimeStr, kHTMLMime) == 0)
     format = CF_HTML;
and only exclude some calls explicitly.

If I'd follow your suggestion, I'd end up with
   else if (strcmp(aMimeStr, kNativeHTMLMime) == 0 ||
            !mapHTMLMime && strcmp(aMimeStr, kHTMLMime) == 0)
     format = CF_HTML;
so, if it says, don't map it, it will map it??

> Actually, I think it may be needed after all. But it would be clearer if it
> said 'GetFormat(kNativeHTMLMime)'
The code you mentioned:
+      else if (strcmp(aDataFlavor, kHTMLMime) == 0) {
+        // If the client wants html, maybe it's in "HTML Format".
+ etc.
doesn't seem to be executed at all.

Anyway, with the patch, dragging from Chrome to the test page (attachment 8593110 [details]) works, dragging from a second FF window doesn't.

Sorry, I'm struggling a little here. I wasn't the original author of bug 586587, I just wanted to see it fixed since it fixed all the "drag from Chrome" issues that Thunderbird users complained about.
(In reply to Neil Deakin from comment #54)
> 
> +      else if (strcmp(aDataFlavor, kHTMLMime) == 0) {
> +        // If the client wants html, maybe it's in "HTML Format".
> +        format = nsClipboard::GetFormat(kHTMLMime);
> +        SET_FORMATETC(fe, format, 0, DVASPECT_CONTENT, -1,
> +                      TYMED_HGLOBAL);
> +        if (mDataObject->QueryGetData(&fe) == S_OK)
> +          *_retval = true;                 // found it!
> +      }
> 
> This change that was made in bug 586587 doesn't seem necessary though after
> looking at this. Maybe we could investigate.

That's one of those "already existed in the patch I started with" pieces of code. It wouldn't surprise me if it's unnecessary.

Jorg, there are probably more places that need to have the flag set to true than just the one inside nsClipboard itself - what you've done is only for pasting, not copying. Finding which of the GetFormat calls across the codebase need the flag could take a while (I'm not sure the answer is less than "all of them", which is why I did it that way, but I don't know the codebase well enough).

I've deleted my local checkout of FireFox by now so I can't easily help track down the code that used to add "HTML Format" (known as CF_HTML in the code) to the clipboard before my patch. That might be the best place to start instead.
> Jorg, there are probably more places that need to have the flag set to true
> than just the one inside nsClipboard itself - what you've done is only for
> pasting, not copying. Finding which of the GetFormat calls across the
> codebase need the flag could take a while (I'm not sure the answer is less
> than "all of them", which is why I did it that way, but I don't know the
> codebase well enough).
> 

I looked at this and I think the proposed patch is ok in this regard.


>   else if (strcmp(aMimeStr, kNativeHTMLMime) == 0 ||
>            !mapHTMLMime && strcmp(aMimeStr, kHTMLMime) == 0)
>     format = CF_HTML;

You'd want:
  mapHTMLMime && strcmp(aMimeStr, kHTMLMime) == 0)
(In reply to Neil Deakin from comment #61)
> You'd want:
>   mapHTMLMime && strcmp(aMimeStr, kHTMLMime) == 0)
But then you need to give most of the callers the 'true' argument. The idea was to not change (n-1) callers and have true by default. If I have to change (n-1) callers, I don't need a default.

Is there a misunderstanding here?

Anyway, as I said, the patch destroys dragging from FF to FF.

> what you've done is only for pasting, not copying
Umm, I thought is was for copying, since with the patch, after pressing <ctrl>C to copy, I have both CF_HTML and text/html on the clipboard.
(In reply to Jorg K (GMT+1) from comment #62)
> Anyway, as I said, the patch destroys dragging from FF to FF.
OK, I've confused myself. Here are the test I have done:

Local build, patched version:
1) copied stuff onto the clipboard, I get text/html and "HTLM Format"
2) copied/dragged stuff to a TB 38 and TB 45 window. Works.
3) copied/dragged stuff from Chrome into the test case (attachment 8593110 [details]). Works.
4) copied stuff from another FF window onto the test case. Works.
5) dragged stuff from another FF window onto the test case. DOES NOT WORK.
6) copied/dragged stuff onto a data:text/html, <html contenteditable>. Works.

So I thought I broke case 5).

Turns out that case 5) already DOES NOT WORK in FF 45, so I didn't break it.

Andrew, can you please confirm:
Start FF 45.
Open your test case, (attachment 8593110 [details]) in another window.
Select and drag my "JK" icon together with the test next to it into the paste area.
It's not working! Compare with copying the same.

So it works when it's dragged from Chrome, but not from FF itself.
Flags: needinfo?(andrew.herron)
You're test case confused itself with some of the other flavours which contained the string "html". I've corrected that.

So with the test case, everything works with my patch.
Flags: needinfo?(andrew.herron)
Neil, as you stated in comment #61 after running various manual tests I have the impression that my patch works.

I don't understand how you'd like it changed. If I make the default 'false', then I have to fix (n-1) calls, since they all have to pass in 'true' which makes the default useless.

Would you like me to repeat my manual tests with the code you mentioned removed, the ...
+      else if (strcmp(aDataFlavor, kHTMLMime) == 0) {
+        // If the client wants html, maybe it's in "HTML Format".
+ etc.

Andrew reckons it was there before he started and my debugging shows that it's not executed, although I'd have to confirm that.
Flags: needinfo?(enndeakin)
Attachment #8733537 - Attachment is obsolete: true
(In reply to Jorg K (GMT+1) from comment #62)
> > what you've done is only for pasting, not copying
> Umm, I thought is was for copying, since with the patch, after pressing
> <ctrl>C to copy, I have both CF_HTML and text/html on the clipboard.

Apologies, it's been a while since I looked at the code.

What's interesting about my old test case is that it uses very similar logic to the product I work - so when it goes looking for HTML it won't find "text/html" in FF45 if the source is Gecko. Luckily that problem won't impact it at the moment but it is interesting that the internal flavors are now available in the clipboard event.
Attached patch Proposed solution (v3). (obsolete) — Splinter Review
So here is my proposal:
Always map text/html onto the "native" HTML unless when setting up data on the clipboard. As a consequence we will put "text/html" and "HTML Format" but only read from "HTML Format" as before.

The code Neil mentioned was superfluous since "text/html" (kHTMLMime) was already handled in the generic case
  format = nsClipboard::GetFormat(aDataFlavor);
above.

Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=45b55ca763d3
Oops, pushed some other patches as well, but they shouldn't matter.

Neil: Sorry, I never understood how you wanted the default for the mapping to be false.
Attachment #8733485 - Attachment is obsolete: true
Attachment #8733485 - Flags: feedback?(enndeakin)
Flags: needinfo?(enndeakin)
Attachment #8733770 - Flags: review?(enndeakin)
Nice!

I wonder whether your patch works because the old code to add HTML Format when copying is still active. That would certainly explain why HTML Format appears twice for FF45 in comment #41. It didn't when I was working on the patch but I used a different clipboard viewer tool.

Might be nice to locate that code and note the details next to the GetFormat(flavorStr, false) call for future reference?
(In reply to Andrew Herron from comment #68)
> I wonder whether your patch works because the old code to add HTML Format
> when copying is still active. That would certainly explain why HTML Format
> appears twice for FF45 in comment #41. It didn't when I was working on the
> patch but I used a different clipboard viewer tool.
Comment #41 was a mistake, see comment #45.

The change works because GetFormat(flavorStr, false) maps "text/html" to "text/html" and native to "HTML Format" for *putting* stuff onto the clipboard *only*. If you don't map "text/html" onto "HTML Format", it gets pumped out here:
format = ::RegisterClipboardFormatW(NS_ConvertASCIItoUTF16(aMimeStr).get());

All the other calls are with default 'true' and only access the native data. As we know, the "text/html" is not necessary any more, but we write it out to support pre-45 Gecko programs.
Assignee: nobody → mozilla
Status: REOPENED → ASSIGNED
<partly off topic - not about the text/html format but about enumerating the clipboard formats>

Further to my comments 41 and 45, the second HTML Format entry is not an error.  Microsoft (https://msdn.microsoft.com/en-us/library/35awt0bz.aspx) points out that there is a Windows Clipboard and an OLE Clipboard mechanism.

For Firefox 45, the Windows API function EnumClipboardFormats (used by the program Free Clipboard Viewer) shows

DataObject
HTML Format
text/_moz_htmlcontext
text/_moz_htmlinfo
CF_UNICODETEXT
CF_TEXT
text/x-moz-url-priv
Ole Private Data
CF_LOCALE
CF_OEMTEXT

Whereas the OLE function IDataObject::EnumFormatEtc (used by the program Inside Clipboard) shows

HTML Format
HTML Format
text/_moz_htmlcontext
text/_moz_htmlinfo
CF_UNICODETEXT
CF_TEXT
text/x-moz-url-priv
Comment on attachment 8733770 [details] [diff] [review]
Proposed solution (v3).

aLen);
>   static nsresult GetGlobalData(HGLOBAL aHGBL, void ** aData, uint32_t * aLen);
>-  static UINT     GetFormat(const char* aMimeStr);
>+  static UINT     GetFormat(const char* aMimeStr, bool aMapHTMLMime = true);

Please add some documentation for this.

>       }
>-      else if (strcmp(aDataFlavor, kHTMLMime) == 0) {
>-        // If the client wants html, maybe it's in "HTML Format".
>-        format = nsClipboard::GetFormat(kHTMLMime);
>-        SET_FORMATETC(fe, format, 0, DVASPECT_CONTENT, -1,
>-                      TYMED_HGLOBAL);
>-        if (mDataObject->QueryGetData(&fe) == S_OK)
>-          *_retval = true;                 // found it!
>-      }
>     } // else try again

I think this is OK since nsClipboard::GetDataFromDataObject doesn't handle text/html anymore.

We should do some tests to make sure:

1. Dragging from thunderbird to Firefox
2. Dragging to and from a contenteditable area in Firefox
3. A page which adds some text/html for dragging. For example, dragging something within the first red box at https://bug1226977.bmoattachments.org/attachment.cgi?id=8721543&t=kCF3W7cCvSuhl1pZHQZDxS   to the body of data:text/html,<body contenteditable>.
Attachment #8733770 - Flags: review?(enndeakin) → review+
Thanks for the review. I will add the documentation and run the tests you've specified.

I need access to bug 1226977 to be able to access attachment 8721543 [details]. Or you could copy it to this bug.

I'm not sure about the comment
> I think this is OK since nsClipboard::GetDataFromDataObject doesn't handle text/html anymore.

I've stepped though nsDragService::IsDataFlavorSupported() and "text/html" is handled here:
https://dxr.mozilla.org/mozilla-central/source/widget/windows/nsDragService.cpp#515
via format = nsClipboard::GetFormat(aDataFlavor); (as I stated in comment #67).
So the extra code after "else if (strcmp(aDataFlavor, kHTMLMime) == 0) {" was actually never run.
Don't you agree?
Flags: needinfo?(enndeakin)
Attached file dragorclip.html
I'll put the testcase here since it is a useful general test and others can try it also.
Flags: needinfo?(enndeakin)
(In reply to Jorg K (GMT+1) from comment #72)
> I've stepped though nsDragService::IsDataFlavorSupported() and "text/html"
> is handled here:
> https://dxr.mozilla.org/mozilla-central/source/widget/windows/nsDragService.
> cpp#515
> via format = nsClipboard::GetFormat(aDataFlavor); (as I stated in comment
> #67).
> So the extra code after "else if (strcmp(aDataFlavor, kHTMLMime) == 0) {"
> was actually never run.
> Don't you agree?

The only time it will run is if there is "text/html" in the drag data but not "HTML Format". It ends up being useless though, since it just calls GetFormat and getting mapped again.

Older versions of Firefox/Thunderbird might produce the situation where only "text/html" exists; the tests I mentioned can check for some of these.

If a problem comes up though, we may need to support "text/html".
OK, looks like I finally understand ;-)

In nsDragService::IsDataFlavorSupported()
we first have this call:
  else {
    // Ok, so we have a single object. Check to see if has the correct
    // data type. Since this can come from an outside app, we also
    // need to see if we need to perform text->unicode conversion if
    // the client asked for unicode and it wasn't available.
    format = nsClipboard::GetFormat(aDataFlavor);
    SET_FORMATETC(fe, format, 0, DVASPECT_CONTENT, -1,
                  TYMED_HGLOBAL | TYMED_FILE | TYMED_GDI);
    if (mDataObject->QueryGetData(&fe) == S_OK)
      *_retval = true;                 // found it!

followed by the code to be removed:

-      else if (strcmp(aDataFlavor, kHTMLMime) == 0) {
-        // If the client wants html, maybe it's in "HTML Format".
-        format = nsClipboard::GetFormat(kHTMLMime);
-        SET_FORMATETC(fe, format, 0, DVASPECT_CONTENT, -1,
-                      TYMED_HGLOBAL);
-        if (mDataObject->QueryGetData(&fe) == S_OK)
-          *_retval = true;                 // found it!
-      }

Now the first call of nsClipboard::GetFormat() will map "text/html" onto "HTML Format" (since it's called with the mapping of true). If no "HTML Format" were present, it would fail.

The second call would do the same and fail equally. So in it's present form the second call makes no sense.

If we have a situation where only "text/html" is present on the clipboard, then we would need the second call, but not in it's present form, but instead like this:
      else if (strcmp(aDataFlavor, kHTMLMime) == 0) {
        // If the client wants html, maybe it's in "text/html" if no native "HTML Format" was present. <====
        format = nsClipboard::GetFormat(kHTMLMime, false);  <=======
        SET_FORMATETC(fe, format, 0, DVASPECT_CONTENT, -1,
                      TYMED_HGLOBAL);
        if (mDataObject->QueryGetData(&fe) == S_OK)
          *_retval = true;                 // found it!
      }

Agreed?

Do you think there are older version of FF/TB which only had "text/html"? With them, one wouldn't have been able to copy/drag to other Windows applications expecting "HTML Format".

Or should I just leave the code in question but use nsClipboard::GetFormat(kHTMLMime, false); and fix the comment?

(Sorry about the NI. I don't know how closely you follow new comments here)
Flags: needinfo?(enndeakin)
I ran an old TB 1.5 from 2005. It populated "text/html" and "HTML Format". I think we're on the safe side to remove the code (after additional testing).
Flags: needinfo?(enndeakin)
I think we're on the safe side to remove the code since it's already been shipped like this, we know it's useless and no one has complained ;-)
I think you can just remove the code. GetDataFromDataObject no longer handles text/html anyway.

If testing or a future bug showed an issue, we would need to add support for the text/html only case. I don't think it will be though; the clipboard setting code appears to always set it in CF_HTML on Windows.
(In reply to bp289 from comment #70)
> Further to my comments 41 and 45, the second HTML Format entry is not an
> error.
I was wrong, the CF_HTML was added twice, I've just found it going over the code again. With the patch it's fixed.
Local build, patched version:
Repeated testing from comment #63:
1) copied stuff onto the clipboard, I get text/html and "HTLM Format"
2) copied/dragged stuff to a TB 38 and TB 45 compose window. Works.
3) copied/dragged stuff from Chrome into the test case (attachment 8733628 [details]). Works.
4)5) copied/dragged stuff from another FF window onto the test case. Works.
6) copied/dragged stuff from FF and Chrome onto a data:text/html, <html contenteditable>. Works.
Additional test as requested in comment #71:
1. Dragging from thunderbird (both 38 and 45) to Firefox (both to contenteditable and attachment 8733628 [details]).
   Works.
2. Dragging to and from a contenteditable area in Firefox. Works.
3. A page which adds some text/html for dragging. Attachment 8734351 [details]:
   Dragged from red box to contenteditable. Works, <b>Bold</b> gets transferred.
So I conclude that it all works.
Carrying forward Neil's r+.
I added the comments that were requested.
Attachment #8733770 - Attachment is obsolete: true
Attachment #8734442 - Flags: review+
Keywords: checkin-needed
Can you please send me a build to play with? Looking at the patch it shouldn't impact bug 586587 but I'd like to make sure :)
It'll be fine, take my word for it. It's essentially a one-liner. I won't start a try job, you can try Nightly after it landed. Have a nice Easter break ;-)
Comment on attachment 8734442 [details] [diff] [review]
Proposed solution (v3) with additional comments.

I'm requesting uplift to all possible versions since the bug that broke this landed on FF 45 ESR.

Approval Request Comment
Feature/regressing bug #: bug 586587
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
As stated above.
User impact if declined: Incompatibility of FF 45+ with older Gecko applications.
Fix Landed on Version: 48.
Risk to taking this patch (and alternatives if risky):
I don't think its risky, all it does is provide the "text/html" clipboard flavour again on the clipboard as it has always done before FF 45. The change is really minimal.
String or UUID changes made by this patch: None.
Describe test coverage new/current, TreeHerder:
Copying/pasting is well covered in various Mochitests. No functionality that affects this was changed, the only change that was made was to restore the "text/html" clipboard flavour. FF users will only notice a change if they use other older Gecko applications.

The timing is essential here: If we wait until the other older Gecko applications have updated, there is no need any more to accelerate the patch.
Attachment #8734442 - Flags: approval-mozilla-release?
Attachment #8734442 - Flags: approval-mozilla-esr45?
Attachment #8734442 - Flags: approval-mozilla-beta?
Attachment #8734442 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/946feaef8a9c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Vbr, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(vbr)
Comment on attachment 8734442 [details] [diff] [review]
Proposed solution (v3) with additional comments.

Recent regression, Aurora47+
Attachment #8734442 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8734442 [details] [diff] [review]
Proposed solution (v3) with additional comments.

Regression from 45, has mochitest coverage.
Discussed with Sylvestre, please uplift to beta and esr45.
Attachment #8734442 - Flags: approval-mozilla-esr45?
Attachment #8734442 - Flags: approval-mozilla-esr45+
Attachment #8734442 - Flags: approval-mozilla-beta?
Attachment #8734442 - Flags: approval-mozilla-beta+
approval-mozilla-release is not required?
Flags: needinfo?(lhenry)
Sylvestre, Wes, not sure if you intended this to go to release. I did not approve it.
Flags: needinfo?(wkocher)
Flags: needinfo?(sledru)
Flags: needinfo?(lhenry)
We should verify this issue before making a call.
Flags: needinfo?(sledru) → qe-verify+
Pretty easy to verify:
Open http://www-archive.mozilla.org/editor/midasdemo/ in FF 44 or older using profile 1.
Open any page with text and images in FF 45 using profile 2.
Copy some HTML with text and images from FF 45 and paste to the FF 44 window.
Result: Plain text is copied.
Now instead of FF 45 use a Nightly. Result: Text and images are copied to the FF 44 window.
Instead of FF 44 and the Midas demo, you can also use Thunderbird 38.
Comment on attachment 8734442 [details] [diff] [review]
Proposed solution (v3) with additional comments.

For 45.0.2
Attachment #8734442 - Flags: approval-mozilla-release? → approval-mozilla-release+
Tested this issue on Windows 7 by copying text with images in Thunderbird 38.7.2. 
I can confirm that the issue is fixed on the following builds:
- Firefox 45.0.2 x32 and x64
- Firefox 45.0.2esr x32 and x64
- Beta 46.0.2 x32 and x64
- Aurora 47.0a2
- Nightly 48.0a1
(In reply to Karla Merza [:KarlaMerza] from comment #100)
> Tested this issue on Windows 7 by copying text with images in Thunderbird
You mean: You copied text from FF and pasted it into TB, right?
Added to the release notes: "Fix a regression with the copy and paste with some old versions of some Gecko applications like Thunderbird (1254980)"
I hope this is okay to ask here...I've been following this (have same issue with FF 45.0.1 suddenly not pasting formatting into TB 38.7.2, and am anxious for this fix in the 45.0.2 release. Do you know when that will be released, or can you direct me to a page to follow that might keep me updated on that? This Wiki channel only says 'probably tomorrow', but that's what it said a few days ago https://wiki.mozilla.org/Firefox/Channels/Meetings/2016-04-05 Thank you.
Sylvestre, can you please make that more precise:
"Fix a regression with the copy from Firefox and paste to some old versions of other Gecko applications like Thunderbird (1254980)"
Flags: needinfo?(vbr) → needinfo?(sledru)
I am not sure to understand the point, this is mentioned in the Firefox release notes, I don't see the need to state "from Firefox".

@procrastistamper@gmail.com Next monday or tuesday
Flags: needinfo?(sledru)
The point is to specify the transfer direction, from Firefox to TB and not the other way around. As a secondary effect my version fixes the slightly unfortunate wording that uses "with" twice. Compare:

"Fix a regression with the copy and paste with some old versions of some Gecko applications like Thunderbird (1254980)"
to
"Fix a regression with the copy from Firefox and paste to some old versions of other Gecko applications like Thunderbird (1254980)"

Yes, it's in the Firefox release notes, so perhaps even shorter:
"Fix a regression with copy/paste to some old versions of other Gecko applications like Thunderbird (1254980)"
That's what you had but with "to" instead of "with". Sorry to be picky ;-)
I just wanted to give my sincere thanks to everyone for taking another look at this and deciding to fix it. It is proving so useful in converting some remaining content from a CMS to a Gecko-based editor. I~m grateful; this would be a royal pain otherwise.
You need to log in before you can comment on or make changes to this bug.