Closed Bug 1317322 Opened 7 years ago Closed 7 years ago

Pasting image from clipboard fails in some cases

Categories

(Core :: DOM: Editor, defect)

50 Branch
Unspecified
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
platform-rel --- +
firefox55 --- fixed

People

(Reporter: alexander, Assigned: nika)

References

(Blocks 1 open bug)

Details

(Whiteboard: [platform-rel-Facebook])

Attachments

(2 files, 4 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.99 Safari/537.36

Steps to reproduce:

OS Windows family, FF 49.

Looks like this bug partially fixed in FF50, but not fully. 

So, in FF49 - pasting images from clipboard doesn't work at all: for example, pasting image on Facebook timeline, Facebook messages, Slack messages (web version), etc. When clipboard contain any image data - Firefox didn't "see" this data. Easy to check - open context menu on edit field in Facebook - item "Paste" was disable.

In FF50 this problem partially fixed. Now it is possible to paste image in Facebook timelime, but not possible to paste on personal message.

How to reproduce:
1. Copy image to clipboard (for example from Paint), 
2. Open Facebook and try to Paste it into timeline post - it is possible to do so.
3. Now open personal messages page (letter icon->See all) and try to paste again into private message to somebody - this is not possible to do, paste action is disabled.
4. Now let's go back to Facebook main page (without reloading, with clicking on logo) and try to paste again into timeline post - Paste is disabled here again.

Only after reloading a page it is possible to paste image again.


Actual results:

Image not pasted normally in facebook private messages (on separate page, not in main page with popup). This could be an error of Facebook or in Firefox.

Also it is not possible to paste image in Slack at all (doesn't work)


Expected results:

Image could be pasted to all corresponding fields (on Facebook, on Slack, etc.)
OS: Unspecified → Windows
Component: Untriaged → Drag and Drop
Product: Firefox → Core
I can reproduce the issue with Beta on Facebook.
Summary: Paste image from clipboard → Pasting image from clipboard fails in some cases
Guess this is a stable FF50 bug now, so bumping the version.
Also, tossed the image paste implementation bug as a depends so what it's related to is clearer.
Should it be a "blocks" ?  No idea which one is appropriate since it is closed anyway ☺
But surely the reference is helpful in case there's an implementation bug and it isn't just the major sites needing Evang.
Depends on: 906420
Version: 49 Branch → 50 Branch
CCing Michael for his informed opinion.
Firefox hasn't supported pasting raw image data from the clipboard into web content until 50 - when it was added in bug 906420. This explains why before firefox 50 this feature did not work at all.

With a bit more investigation, it appears as though the problem with Facebook's messenger (and potentially with slack) is caused by our editor controller deciding that the paste command is not enabled when a textarea is focused, while it is available when a contenteditable area is focused. Because of that the paste command is only successfully fired on the contenteditable area. We don't allow it in text areas. This problem could be fixed by doing something similar to what we do with the Copy and Paste commands, which is having them active 100% of the time when the currently focused document is XHTML or HTML, and there is any data which we may support through, for example, DataTransfer, on the clipboard, and only checking for paste support when in "native" XUL documents.

It may also be fixable with some gymnastics by facebook & co.

Ehsan, do you think that this is a reasonable approach?
Flags: needinfo?(ehsan)
I'm not sure if I understand the suggestion...

We can't paste images in textareas.  How does enabling the paste command help us do that?  I think perhaps I don't understand what the website is trying to do and how Gecko breaks that yet...
Flags: needinfo?(ehsan) → needinfo?(michael)
(In reply to :Ehsan Akhgari from comment #5)
> I'm not sure if I understand the suggestion...
> 
> We can't paste images in textareas.  How does enabling the paste command
> help us do that?  I think perhaps I don't understand what the website is
> trying to do and how Gecko breaks that yet...

The document has a `paste` event listener attached to a textarea, when the user pastes an image into the textarea, the website is expecting the paste into the textarea to fail, but for the paste event listener to fire. It does not.

Does that kinda make sense?
Flags: needinfo?(michael) → needinfo?(ehsan)
(In reply to Michael Layzell [:mystor] [:mrl] from comment #6)
> (In reply to :Ehsan Akhgari from comment #5)
> > I'm not sure if I understand the suggestion...
> > 
> > We can't paste images in textareas.  How does enabling the paste command
> > help us do that?  I think perhaps I don't understand what the website is
> > trying to do and how Gecko breaks that yet...
> 
> The document has a `paste` event listener attached to a textarea, when the
> user pastes an image into the textarea, the website is expecting the paste
> into the textarea to fail, but for the paste event listener to fire. It does
> not.
> 
> Does that kinda make sense?

I see, yes this makes perfect sense.

It seems like in bug 1208217, we fixed this for nsClipboardCommand, but we forgot about the equivalent commands in EditorCommands.cpp (CopyCommand, CutCommand and PasteCommand.)  There are other variants of these commands (such as CutOrDeleteCommand, for example) which I think we can ignore since they're not bound to Ctrl+C/X/V.

Can you please take this, Michael?  Thanks!
Blocks: 1208217
Flags: needinfo?(ehsan) → needinfo?(michael)
I did some more tests and my understanding was completely incorrect. We are in fact firing the events when you use C-V.

I also tried performing the steps in chrome and it seemed like it didn't work there either, so I don't think that this is our fault.
Flags: needinfo?(michael)
In Chrome pasting works normally for me (Windows platform), everywhere on FB and in Slack.
Just a bit of an update. :mystor says the Chrome failure he reported above was Linux-only.  Under Windows it worked fine, which agrees with the bug reporter.

That does suggest possibly fail UA sniffing on Facebook's part...
Would be interesting if reporter could try spoofing a Chrome for Windows user agent.  If it works fine then this is an evang bug.
(In reply to nemo from comment #10)
> Just a bit of an update. :mystor says the Chrome failure he reported above
> was Linux-only.  Under Windows it worked fine, which agrees with the bug
> reporter.
> 
> That does suggest possibly fail UA sniffing on Facebook's part...
> Would be interesting if reporter could try spoofing a Chrome for Windows
> user agent.  If it works fine then this is an evang bug.

I have tested Firefox with a Chrome UA, and the paste works in that situation. I think this is a facebook problem.
(In reply to Michael Layzell [:mystor] [:mrl] from comment #11)
> (In reply to nemo from comment #10)
> > Just a bit of an update. :mystor says the Chrome failure he reported above
> > was Linux-only.  Under Windows it worked fine, which agrees with the bug
> > reporter.
> > 
> > That does suggest possibly fail UA sniffing on Facebook's part...
> > Would be interesting if reporter could try spoofing a Chrome for Windows
> > user agent.  If it works fine then this is an evang bug.
> 
> I have tested Firefox with a Chrome UA, and the paste works in that
> situation. I think this is a facebook problem.

Can you please contact them to confirm?  Thanks!
Based on the comments going to try to reclassify this. Expanded platform to "all" since it seems *more* broken on Linux apparently (in that Facebook breaks all browsers, not just Firefox)
Component: Drag and Drop → Add-ons
OS: Windows → All
Product: Core → Tech Evangelism
Version: 50 Branch → Firefox 50
Component: Add-ons → Desktop
platform-rel: --- → ?
Whiteboard: [platform-rel-Facebook]
platform-rel: ? → +
Rank: 9
(In reply to :Ehsan Akhgari from comment #12)
> (In reply to Michael Layzell [:mystor] [:mrl] from comment #11)
> > (In reply to nemo from comment #10)
> > > Just a bit of an update. :mystor says the Chrome failure he reported above
> > > was Linux-only.  Under Windows it worked fine, which agrees with the bug
> > > reporter.
> > > 
> > > That does suggest possibly fail UA sniffing on Facebook's part...
> > > Would be interesting if reporter could try spoofing a Chrome for Windows
> > > user agent.  If it works fine then this is an evang bug.
> > 
> > I have tested Firefox with a Chrome UA, and the paste works in that
> > situation. I think this is a facebook problem.
> 
> Can you please contact them to confirm?  Thanks!

Mike - I may have missed this (apologies if I have), but have we perhaps reached out via the DL?
Flags: needinfo?(miket)
I haven't yet seen this discussed on our internal mailing list...
It looks like we have not, ideally we can give FB more info than "this works when spoofing as Chrome" before reaching out.
Flags: needinfo?(miket)
Whiteboard: [platform-rel-Facebook] → [platform-rel-Facebook][needsdiagnosis]
Tom, can you take a look at why spoofing as Chrome gets paste to work here? Ideally we can at least point FB to the right file for them to investigate further.
Flags: needinfo?(twisniewski)
This is what I'm seeing, in a Windows 10 VM with Firefox 50.0.2 (fresh profile) and in the latest Chrome. Also in Gentoo Linux, with Firefox nightly and the latest Chrome stable build.

In Slack, I can ctrl-V to bring up the "Upload a file?" dialog in both browsers, with the image in my clipboard appearing there. In Chrome I can also right-click the message bar to get a "Paste" option which does the same thing. In Firefox the paste option is greyed-out.

But in Facebook, I can visit a friend's timeline page, click the "write something to X..." box (once the page fully loads), and then ctrl-V or paste the image from the context menu. I can also ctrl-V and menu-paste if I click their "Message" button to send them a PM in the little Messenger pop-up which appears above the timeline message box. I have no clue what the "letter icon" is, though, so I haven't confirmed that option yet.

Changing my UA to Chrome does not seem to change Firefox's behavior on either site, in either OS.

Looking over their markup, Slack is using a plain textarea. Facebook starts with a textarea for the "write something to X..." box, but changes it as the page loads into a contenteditable div. It's also using a contenteditable div for the PM pop-up. (Perhaps that changing from textarea to contentedtiable div explains why sometimes it seems to offer "Paste" in the menu, and other times it doesn't...?)

But unless I'm missing something, there isn't a problem with either site. Firefox is just not offering the "Paste" menu option when for images in textareas (even if they have a paste listener). Yet it does so for contenteditable divs.

In other words, my testing seems to match comment 4's assessment, and that if a paste handler/etc are detected, we could offer the paste option (even if the textarea itself won't end up with anything pasted in it).
Flags: needinfo?(twisniewski)
Hmm, so that means we should do comment 7, except that Michael had already looked into that...  Michael, can you think of where the disconnect here is?  Thank you!
Flags: needinfo?(michael)
If it helps, based on comment 7, I hacked a local build's EditorCommands.cpp PasteCommand::IsCommandEnabled() to always set aIsEnabled=true, and with that the textarea on Slack does what it does in Chrome.
(In reply to :Ehsan Akhgari from comment #19)
> Hmm, so that means we should do comment 7, except that Michael had already
> looked into that...  Michael, can you think of where the disconnect here is?
> Thank you!

So the problem which I was running into was whether or not Ctrl-V would work when in a textarea with a paste listener. When I was in firefox I found that facebook wouldn't handle that situation correctly, while when I spoofed chrome it did. I am not sure whether or not this is still the case.

I believe that you are correct about the menu option being not available in firefox within that text area, and that should be a simple fix. I'll try to have a patch up shortly.
Oops, I apparently forgot to post this patch. :S
Attachment #8825111 - Flags: review?(ehsan)
So... does this mean there's both a tech evang component (the website behaving differently on ctrl-v based on UA) and a firefox bug (this missing context menu) ?

If so, should this be two bugs?
Flags: needinfo?(michael)
(In reply to nemo from comment #23)
> So... does this mean there's both a tech evang component (the website
> behaving differently on ctrl-v based on UA) and a firefox bug (this missing
> context menu) ?
> 
> If so, should this be two bugs?

:twisniewski's comment seems to suggest that the tech evang component is no longer an issue so I figure we don't need to file a separate bug for it and can just use this bug for the context menu changes.
See Also: → 1329754
Comment on attachment 8825111 [details] [diff] [review]
Activate `paste` in menus when within <textarea> controls

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

Looks good, but you should also add tests.  r=me with the expectation that the patch will land with tests.
Attachment #8825111 - Flags: review?(ehsan) → review+
Component: Desktop → Editor
Product: Tech Evangelism → Core
Version: Firefox 50 → 50 Branch
Assignee: nobody → michael
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
ni?ing myself to make sure I write the tests.
Flags: needinfo?(michael)
Blocks: 1326344
I finally got around to writing some simple tests for this change.

MozReview-Commit-ID: FL1tRmyzJvr
Attachment #8827299 - Flags: review?(ehsan)
Flags: needinfo?(michael)
Blocks: SyncIPC
Comment on attachment 8827299 [details] [diff] [review]
Part 2: Add tests for new paste command behavior

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

Please fold these patches together when landing, there's no need for them to be separate.  r=me with that.
Attachment #8827299 - Flags: review?(ehsan) → review+
I had to back this (and the followup to disable the test on android) out for frequent clipboard test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=69679287&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/486075111a45
Flags: needinfo?(michael)
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79d6e9229fda
Activate `paste` in menus when within <textarea> controls, r=ehsan
Updated patch which should be safe to land without the windows only test failures.

MozReview-Commit-ID: 7YD8qtsC3u6
Attachment #8825111 - Attachment is obsolete: true
Attachment #8827299 - Attachment is obsolete: true
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f63b9d012c0
Activate `paste` in menus when within <textarea> controls, r=ehsan
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51116aebee59
Fix a windows devtools test failure on inbound, a=bustage
Whiteboard: [platform-rel-Facebook][needsdiagnosis] → [platform-rel-Facebook]
Any luck landing this yet?
ping?
(In reply to :Ehsan Akhgari from comment #38)
> ping?

I'm working on fixing the leak in my spare time. The cycle time has been long because I have to push to try every time I want to test a patch.
Flags: needinfo?(michael)
(In reply to Michael Layzell [:mystor] from comment #39)
> (In reply to :Ehsan Akhgari from comment #38)
> > ping?
> 
> I'm working on fixing the leak in my spare time. The cycle time has been
> long because I have to push to try every time I want to test a patch.

Thanks!  Just pinging because I keep seeing this issue coming up in various profiles.  :-)

FWIW https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Refcount_tracing_and_balancing is the systematic way of debugging leaks like this, in case you haven't come across it already.  If this only happens on try then you'd need to do look at the leak reports there.  testing/mochitest/runtests.py claims that we save a runtests_leaks.log file with that.  That's also where you want to define the environment variables in the MDN page.  Let me know if I can help!
MozReview-Commit-ID: 13zUBotz7Tr
Attachment #8840549 - Flags: review?(ehsan)
Comment on attachment 8840549 [details] [diff] [review]
Change the SetOpenerWindow tabgroup assertion to a DIAGNOSTIC_ASSERT

oops, wrong bug
Attachment #8840549 - Attachment is obsolete: true
Attachment #8840549 - Flags: review?(ehsan)
The leak seems to be being caused by this AddRef: http://searchfox.org/mozilla-central/rev/c9ccb8ec1f5acda78e6344ffb87aa3a409031e3d/widget/windows/nsDataObj.cpp#464 which spawns a timer which is not run before the browser shuts down. Basically, as the last reference is destroyed due to OleFlushKeyboard from the OS in nsClipboard::Observe, we queue up a timer to actually destroy it because we have a temp file which we want to destroy and that timer never fires. 

In the test run which I did to discover this, the timer was set at 12:40:28 in my test run in question, and the leakcheck occurs at 12:40:34, which is much less than the 500ms which the timer takes.

I'm looking into fixing this bug by creating an alternate nsIObserver class `TempFileDestroyer` which holds the last reference to the tempfile, and listens for either the xpcom shutdown event or the timer firing to delete the file from the filesystem. That way the object shouldn't be kept alive for longer than is necessary.
MozReview-Commit-ID: 90obWnqRSWk
Attachment #8842111 - Flags: review?(jmathies)
Comment on attachment 8842111 [details] [diff] [review]
Part 2: Fix shutdown leak when win32 holds nsDataObj with temp file until shutdown

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

::: widget/windows/nsDataObj.cpp
@@ +441,5 @@
>  	return m_cRef;
>  }
>  
> +namespace {
> +class TempFileDestroyer : public nsIObserver

nit, not a huge fan of 'TempFileDestroyer'. Can we give it a bit more professional name, something like 'RemoveTempFileHelper' or similar.
Attachment #8842111 - Flags: review?(jmathies) → review+
Attachment #8842111 - Attachment is obsolete: true
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39cba44c517d
Part 1: Activate `paste` in menus when within <textarea> controls, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eb8e1322979
Part 2: Fix shutdown leak when win32 holds nsDataObj with temp file until shutdown, r=jimm
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee9b08930b65
Part 3: Make RemoveTempFileHelper's constructor explicit, a=bustage
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac2f3894113f
Part 1: Activate `paste` in menus when within <textarea> controls, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a64d16d18e5
Part 2: Fix shutdown leak when win32 holds nsDataObj with temp file until shutdown, r=jimm
relanded it because it looks like it wasn't my fault (https://bugzilla.mozilla.org/show_bug.cgi?id=1337772)
Flags: needinfo?(michael)
https://hg.mozilla.org/mozilla-central/rev/ac2f3894113f
https://hg.mozilla.org/mozilla-central/rev/5a64d16d18e5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.