1243507 - Drag & drop from external application (keepass 1.30) no longer possible in Firefox 44

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[[:philipp]]

we had 3 independent users reporting that they were no longer able to drag & drop text (username/password entries) from the external keepass 1.30 tool (later keepass 2 versions do work) into login forms since they updated to firefox 44. 

this is easily reproducible and regression testing this leads to https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=3df3d4856542e8ebad6262becab79058fb592b71&tochange=8ca7268d0d1f17375e870a43982b18a76a6fbd50 and bug 943294 as cause for this.

i don't know if this would be an intended by-product of bug 943294, but wanted to get it on file at least...

Comment 1

[Masatoshi Kimura [:emk]]

This is not expected. We expected that we would have no user-visible change.

My speculation is:
Although Windows Clipboard will automatically convert CF_TEXT for us, drag and drop will pass any random IDataObj that may not support CF_UNICODETEXT because the drop source app implements IDataObj, not the Windows system.

We will have to backout bug 943294 to support drag ang drop from Unicode-unaware apps :(

Comment 2

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd5c96b8784c

Comment 3

[Masatoshi Kimura [:emk]]

philipp, could you test this whether these builds fix the issue?

Win32:
https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-6eb7767edf6ba9d042c9a94587b441db56afb270/try-win32/firefox-47.0a1.en-US.win32.installer.exe
Win64:
https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-6eb7767edf6ba9d042c9a94587b441db56afb270/try-win64/firefox-47.0a1.en-US.win64.installer.exe

Comment 4

[[:philipp]]

yes, the issue is fixed in both those try builds.

Comment 5

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2541ec5c3082

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Backout bug 943294 as it breaks drag and drop from some Unicode-unaware apps

This is a straight backout generated by "hg backout".
This patch will not compile because we removed nsPrimitiveHelpers::ConvertPlatformPlainTextToUnicode. I'll fix it in the next patch.

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: Reimplement FindUnicodeFromPlainText using NS_CopyNativeToUnicode

See <https://hg.mozilla.org/mozilla-central/rev/01a5ca4ef014#l1.84> about the removed ConvertPlatformPlainTextToUnicode.
I'll fold two patches before landing.

Comment 8

[Henri Sivonen (:hsivonen)]

So very sad that Windows isn't consistent across clipboard and D&D--especially considering how Microsoft's clipboard docs endorse the approach taken in bug 943294.

Did you happen to try to research if there's some way to ask the D&D system to behave like the clipboard and do the encoding conversion on the system side?

Comment 9

[Masatoshi Kimura [:emk]]

I'd like to keep the change minimal because I consider to uplift once this fix landed on m-c.
The restored code should be harmless to clipboard operations because CF_UNICODETEXT should always be found.

Please file a follow-up bug for further improvements.

Comment 10

[Loic]

Will this patch fix the opposite operation, id est, drag and drop text from Firefox to external application (like Bing search field in IE)?

Comment 11

[Masatoshi Kimura [:emk]]

No, apparently it is a different problem. Notepad and Internet Explorer are Unicode apps.

Comment 12

[Masatoshi Kimura [:emk]]

Jim, ping?
Please redirect to another reviewer if you busy. I would like to fix this early in the cycle for uplift.

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 8713135 [details] [diff] [review]
Reimplement FindUnicodeFromPlainText using NS_CopyNativeToUnicode

Review of attachment 8713135 [details] [diff] [review]:
-----------------------------------------------------------------

sorry for the delay.

::: widget/windows/nsClipboard.cpp
@@ +791,2 @@
>  
> +  const char* castedText = static_cast<char*>(*outData);          

nit - ws

@@ +791,4 @@
>  
> +  const char* castedText = static_cast<char*>(*outData);          
> +  nsAutoString tmp;
> +  nsresult rv = NS_CopyNativeToUnicode(nsDependentCSubstring(castedText, *outDataLen), tmp);

lets change 'loadResult' to 'rv', and just reuse it.

@@ +795,5 @@
> +  if (NS_FAILED(rv)) {
> +    return false;
> +  }
> +
> +  // out with the old, in with the new 

nit - ws

Comment 14

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebb0d8d11b23ed0373df527cf62a04a234896c8c
Bug 1243507 - Reimplement non-Unicode clipboard formats as bug 943294 broke drag and drop between some Unicode-unaware apps. r=jimm

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: patch for checkin

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ebb0d8d11b23

Comment 18

[Masatoshi Kimura [:emk]]

Comment on attachment 8715418 [details] [diff] [review]
patch for checkin

Approval Request Comment
[Feature/regressing bug #]: bug 943294
[User impact if declined]: Users can not drag and drop text between Firefox and some apps.
[Describe test coverage new/current, TreeHerder]: manually tested
[Risks and why]: Low. Although the patch looks large, most of changes are a simple backout.
[String/UUID change made/needed]: none

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8715418 [details] [diff] [review]
patch for checkin

Simple backout, taking it.
Should be in 45 beta 3.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2aabafe49701

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/688a42663f99

Comment 23

[Henri Sivonen (:hsivonen)]

(In reply to Masatoshi Kimura [:emk] from comment #9)
> I'd like to keep the change minimal because I consider to uplift once this
> fix landed on m-c.
> The restored code should be harmless to clipboard operations because
> CF_UNICODETEXT should always be found.
> 
> Please file a follow-up bug for further improvements.

Thanks for fixing this.

I suppose further improvements aren't worthwhile at this point: since we now do the legacy conversion using Windows' own converters, getting Windows to do it for us wouldn't be much of a win.

At least we still got rid of the nsPlatformCharset usage.

Comment 24

[Chris Parsons]

Just to add: This bug shows itself with Outlook 2016. You can drag and drop onto a subject field, but not the bulk of an email.

Comment 27

[Jefferson]

Is this the only clipboard format related change in Firefox 45? Two users on SuMo report that Thunderbird isn't finding HTML on the clipboard when pasting from Firefox 45. ??

https://support.mozilla.org/questions/1113486
https://support.mozilla.org/questions/1113511

Comment 28

[Jefferson]

Oops, never mind: new bug already filed this morning for the Firefox 45 copy / Thunderbird paste problem: https://bugzilla.mozilla.org/show_bug.cgi?id=1254980