1940790 - Support PNG when pasting images into Firefox

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P2)

Description

[Ray Kraesig [:rkraesig]]

At present, when a pasted/dropped image is PNG and the paste/drop target within Firefox wants PNG, we unconditionally convert the pasted image(s) from PNG into BMP and back. Not only is this wasteful, it strips transparency information.

We should avoid doing this whenever possible.

Comment 1

[mercury13]

I meant another thing.
Some programs, in addition to DIB, DIBv5 and BITMAP, produce clipboard in PNG format.
As DIB does not support transparency and DIBv5 fragmented into several formats (short/long, raw/premultiplied), support PNG in clipboard if it’s present, with higher priority than DIB.

Comment 2

[mercury13]

As my issue is related to a single program (Paint.NET), preferring PNG to DIB will perfectly solve my concerns.

Comment 3

[mercury13]

Paint.NET’s explanations are right, but on paper only. Saving clipboard in one format (DIB, DIBv5, BITMAP) is enough, Windows will auto-convert. And, AFAIR, it creates “long” DIBv5 when converts to, but accepts ”short” for conversion from. Or vice-versa, IDR.

Comment 4

[mercury13]

Well, yes. Produces long DIBv5, but converts as it would be short.

Comment 5

[Ray Kraesig [:rkraesig]]

This bug is not about DIB or DIBv5. If you have any comments appertaining thereto, they belong on bug 1866655 or on the now-meta-bug 460969, not here.

Comment 6

[mercury13]

IDK why you separated from the old issue, but I supposed, you decided to support PNG in clipboard. That was one of possible solutions of the original issue.

Comment 7

[Greg Stoll :gstoll]

Attachment: Bug 1940790 - read PNG format from clipboard on Windows r=edgar!

On Windows, for the image/png MIME type we should read the "PNG" format
from the clipboard. Note that this is not officially defined by Microsoft
but seems to be commonly in use. (by at least us and Paint.NET)

The new test passes on Windows (after this change), and also Mac and
Linux.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77a9e19339e4
read PNG format from clipboard on Windows r=edgar

Comment 9

[amarc]

https://hg.mozilla.org/mozilla-central/rev/77a9e19339e4

Comment 10

[Greg Stoll :gstoll]

Release Note Request (optional, but appreciated)
[Why is this notable]: Long-standing S2 bug (it was only filed a few months ago but I doubt Firefox ever supported this) that caused images pasted into Firefox to lose their transparency
[Affects Firefox for Android]: no
[Suggested wording]: PNG images with transparency will keep their transparency when pasted into Firefox.
[Links (documentation, blog post, etc)]:

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Added to the Fx139 relnotes.

Comment 12

[Serban Stanca [:SerbanS]]

Backed out as requested by gstoll for causing Bug 1958434.

• Backout link

Comment 13

[Norisz Fay [:noriszfay]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/a494c411ee44

Comment 14

[mercury13]

+      } else if (flavorStr.EqualsLiteral(kPNGImageMime)) {
+        // Fall back to DIBV5 format
+        dataFound = NS_SUCCEEDED(GetNativeDataOffClipboard(
+            aDataObject, anIndex, CF_DIBV5, flavorStr.get(), &data, &dataLen));

I do not know your code really well, but this code wouldn’t pass my review. Found PNG and dig for DIB?

Comment 15

[Greg Stoll :gstoll]

(In reply to mercury13 from comment #14)

+      } else if (flavorStr.EqualsLiteral(kPNGImageMime)) {
+        // Fall back to DIBV5 format
+        dataFound = NS_SUCCEEDED(GetNativeDataOffClipboard(
+            aDataObject, anIndex, CF_DIBV5, flavorStr.get(), &data, &dataLen));

I do not know your code really well, but this code wouldn’t pass my review. Found PNG and dig for DIB?

This was essentially the old behavior before this fix. Although it's definitely possible this is what caused the regression, which I'll take a look at next week.

Comment 16

[mercury13]

This was essentially the old behavior before this fix. Although it's definitely possible this is what caused the regression, which I'll take a look at next week.

No, that’s from https://hg.mozilla.org/mozilla-central/rev/77a9e19339e4 pushed on Apr 2 and quickly reverted.

Comment 17

[Greg Stoll :gstoll]

My point is that this is basically equivalent to this line which set the format to CF_DIBV5 if the flavorStr was kPNGImageMime, which was the behavior before this change.

Comment 18

[mercury13]

Another strange line. Format JPEG → set DIB, probably “JPEG is not supported”.
Format JPEG or PNG → set DIB, what?

Comment 19

[mercury13]

SORRY, that’s actually OK, used to be JPEG and PNG not supported, and now some special bhv for PNG.

Comment 20

[Greg Stoll :gstoll]

Unfortunately after reapplying my change locally I'm having trouble reproducing bug 1958434

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/743e7d6d8927
read PNG format from clipboard on Windows r=edgar

Comment 22

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/743e7d6d8927

Comment 23

[Greg Stoll :gstoll]

In case this becomes a problem in the future, https://bug460969.bmoattachments.org/attachment.cgi?id=9053448 is a good testcase for this.