Closed Bug 952456 Opened 11 years ago Closed 9 years ago

Implement non-text/rich text support for B2G clipboard

Categories

(Firefox OS Graveyard :: Runtime, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(tracking-b2g:backlog, firefox43 fixed)

RESOLVED FIXED
FxOS-S6 (04Sep)
tracking-b2g backlog
Tracking Status
firefox43 --- fixed

People

(Reporter: janjongboom, Assigned: boris)

References

Details

Attachments

(5 files, 24 obsolete files)

(deleted), application/zip
Details
(deleted), text/x-github-pull-request
kanru
: review+
timdream
: review+
Details
(deleted), patch
boris
: review+
Details | Diff | Splinter Review
(deleted), patch
fabrice
: review+
Details | Diff | Splinter Review
(deleted), patch
boris
: review+
Details | Diff | Splinter Review
In bug 946646 we added a clipboard module to gonk, but it does only text.
Depends on: 946646
We should finish this where we want to ship bug 946646.
blocking-b2g: --- → 1.4?
Summary: Implement non-text support for B2G clipboard → Implement non-text/unicode support for B2G clipboard
Moving to 1.5 since the UI work won't be ready to use the clipboard.
blocking-b2g: 1.4? → 1.5?
Adding to backlog, enabling text first.
Blocks: 908549
blocking-b2g: 1.5? → backlog
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
Is this bug still valid?
Flags: needinfo?(mtseng)
Still valid I think. We don't have non-text support (for example: image) for b2g now.
Flags: needinfo?(mtseng)
Update summary to make the bug more specific.
Component: Gaia::Keyboard → Runtime
Summary: Implement non-text/unicode support for B2G clipboard → Implement non-text/rich text support for B2G clipboard
blocking-b2g: backlog → ---
Blocks: 1121463
Assignee: nobody → boris.chiou
Depends on: 1181467
Status: NEW → ASSIGNED
Attached file CopyImage test app (deleted) —
This app has some local images and hyperlinks, and a content editable area where you can paste your image and hyperlinks.
Attached image Flow chart - Copy Image (obsolete) (deleted) —
1. Implement new BrowserElement API - CopyImage() 2. Add cmd_copyImage into COMMAND_MAP 3. Set HTMLImageElement when long pressing 4. Implement gonk/nsClipboard.cpp for kHTMLMime, kNativeImageMime, and other image formats. 5. Add new class, mozilla::GonkClipboardData, to save clipboard data on Gonk
Blocks: 1187226
Attachment #8638384 - Attachment is obsolete: true
Handle text/html and image MIME types on gonk/nsClipboard
Attachment #8636527 - Attachment is obsolete: true
Add CopyImage() into BrowserElementAPI
Attachment #8636528 - Attachment is obsolete: true
Comment on attachment 8638433 [details] [diff] [review] Part 1: Implement gonk/nsClipboard for rich text and raw image (v4) Review of attachment 8638433 [details] [diff] [review]: ----------------------------------------------------------------- Hi froydnj, I implement nsClipboard::GetData() & nsClipboard::SetData() for text/html and image/[png|jpeg|jpg|gif] mime types, and add a new class, GonkClipboardData, to store the clipboard data on gonk. Could you please review this patch? Thanks.
Attachment #8638433 - Flags: review?(nfroyd)
Attachment #8638434 - Flags: feedback?(mtseng)
Comment on attachment 8638433 [details] [diff] [review] Part 1: Implement gonk/nsClipboard for rich text and raw image (v4) Review of attachment 8638433 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, Boris. I've just been the reviewer for some refactoring commits, so I don't think I'm the right person to review this new feature. Fabrice looks to be the other primary reviewer, but he's out for the next several days. Jan, since you wrote the code initially, do you feel comfortable reviewing? If Jan doesn't want to or isn't able to, :mwu might be another reasonable choice.
Attachment #8638433 - Flags: review?(nfroyd) → review?(janjongboom)
Attachment #8638434 - Flags: feedback?(mtseng) → feedback+
Attachment #8638434 - Flags: review?(kchen)
Comment on attachment 8638434 [details] [diff] [review] Part 2: Add new BrowserElementAPI to support copy image (v4) Review of attachment 8638434 [details] [diff] [review]: ----------------------------------------------------------------- Could we implement this as a new system context menu entry? When the context menu is selected, _recvFireCtxCallback should be able to use cmd_copyImage without the new API.
Attachment #8638434 - Flags: review?(kchen)
Comment on attachment 8638433 [details] [diff] [review] Part 1: Implement gonk/nsClipboard for rich text and raw image (v4) I don't feel comfortable reviewing this. As suggested in Comment 21, forwarding to mwu.
Attachment #8638433 - Flags: review?(janjongboom) → review?(mwu)
(In reply to Kan-Ru Chen [:kanru] from comment #22) > Comment on attachment 8638434 [details] [diff] [review] > Part 2: Add new BrowserElementAPI to support copy image (v4) > > Review of attachment 8638434 [details] [diff] [review]: > ----------------------------------------------------------------- > > Could we implement this as a new system context menu entry? When the context > menu is selected, _recvFireCtxCallback should be able to use cmd_copyImage > without the new API. Hi Kanru, In attachment 8640395 [details] [diff] [review], I didn't create new browse element API, and reuse contextMenuItemSelected() as the callback function while press "Copy Image" option in the context menu. HTML img tags don't have the ctxMenuId, so I don't have a good method to make it as a special context menu. Please also check the related gaia usage (attachment 8640402 [details] [diff] [review]). Could you please give me some feedback about this? Thank you.
Attachment #8640395 - Flags: feedback?(kchen)
Comment on attachment 8640395 [details] [diff] [review] [WIP] Part 2: Support copy image in BrowserElement Review of attachment 8640395 [details] [diff] [review]: ----------------------------------------------------------------- In _getSystemCtxMenuData() we already entry for <img> tags. I think you could create the special menu item in _buildMenuObj if there is <img> tag and ignore ctxMenuId.
Attachment #8640395 - Flags: feedback?(kchen)
Comment on attachment 8638433 [details] [diff] [review] Part 1: Implement gonk/nsClipboard for rich text and raw image (v4) Review of attachment 8638433 [details] [diff] [review]: ----------------------------------------------------------------- I'm not familiar with the clipboard code. We should probably wait for fabrice.
Attachment #8638433 - Flags: review?(mwu) → review?(fabrice)
Attachment #8638434 - Attachment is obsolete: true
Attachment #8640395 - Attachment is obsolete: true
Comment on attachment 8640995 [details] [diff] [review] [WIP] Part 2: Support copy image in BrowserElement (v2) Review of attachment 8640995 [details] [diff] [review]: ----------------------------------------------------------------- Hi Kanru, How about this implementation? I added "Copy Image" as a special context menu option. Thanks.
Attachment #8640995 - Flags: feedback?(kchen)
Comment on attachment 8640995 [details] [diff] [review] [WIP] Part 2: Support copy image in BrowserElement (v2) Review of attachment 8640995 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: dom/browser-element/BrowserElementChildPreload.js @@ +868,5 @@ > + > + // Enable copy image option > + if (!hasImgElement && elem.nodeName == 'IMG') { > + hasImgElement = true; > + } !hasImgElement is always true
Attachment #8640995 - Flags: feedback?(kchen) → feedback+
Attachment #8640995 - Attachment is obsolete: true
Attachment #8640997 - Attachment is obsolete: true
Attachment #8641457 - Flags: review?(kchen)
Attachment #8641456 - Flags: review?(timdream)
Attachment #8641456 - Flags: review?(kchen)
(In reply to Kan-Ru Chen [:kanru] from comment #33) > Comment on attachment 8640995 [details] [diff] [review] > [WIP] Part 2: Support copy image in BrowserElement (v2) > > Review of attachment 8640995 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. > > ::: dom/browser-element/BrowserElementChildPreload.js > @@ +868,5 @@ > > + > > + // Enable copy image option > > + if (!hasImgElement && elem.nodeName == 'IMG') { > > + hasImgElement = true; > > + } > > !hasImgElement is always true I removed this check in the newer patch. Thanks for your feedback :)
Attachment #8641456 - Flags: review?(timdream) → review?(im)
Attachment #8641456 - Flags: review?(im)
Attachment #8641456 - Flags: review?(kchen) → review+
Comment on attachment 8641457 [details] [diff] [review] Part 2: Support copy image in BrowserElement (v3) Review of attachment 8641457 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/BrowserElementChildPreload.js @@ +1250,5 @@ > + menuObj.items.push({id: 'copy-image'}); > + } else if (!menu) { > + // menu is null and hasImgElement is false > + return null; > + } return null changes behavior. Originally we always return menuObj which may or may not have zero items and I think we should still do this.
Attachment #8641457 - Flags: review?(kchen) → review+
(In reply to Kan-Ru Chen [:kanru] from comment #37) > Comment on attachment 8641457 [details] [diff] [review] > Part 2: Support copy image in BrowserElement (v3) > > Review of attachment 8641457 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/browser-element/BrowserElementChildPreload.js > @@ +1250,5 @@ > > + menuObj.items.push({id: 'copy-image'}); > > + } else if (!menu) { > > + // menu is null and hasImgElement is false > > + return null; > > + } > > return null changes behavior. Originally we always return menuObj which may > or may not have zero items and I think we should still do this. Sure, I will upload a new one to fix this part. Thanks.
Attachment #8638381 - Attachment is obsolete: true
Comment on attachment 8638433 [details] [diff] [review] Part 1: Implement gonk/nsClipboard for rich text and raw image (v4) Review of attachment 8638433 [details] [diff] [review]: ----------------------------------------------------------------- That looks fine, I'm setting f+ for now just because I'd like to know about the mImgTool use. ::: widget/gonk/GonkClipboardData.cpp @@ +47,5 @@ > + > +void > +GonkClipboardData::SetImage(gfx::DataSourceSurface* aDataSource) > +{ > + // Clone a new DataSourceSurface and store it nit: add a full stop at the end of comments. (not just this one) ::: widget/gonk/nsClipboard.h @@ +1,2 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* vim:set ts=2 sw=2 sts=2 et: */ let's not fight on the mode-line please :) @@ +26,5 @@ > ~nsClipboard() {} > + > +private: > + mozilla::UniquePtr<mozilla::GonkClipboardData> mClipboard; > + nsCOMPtr<imgITools> mImgTool; I'm not sure how costly it is to recreate an instance, but if it's cheap I don't see any good reason to tie that to nsClipboard.
Attachment #8638433 - Flags: review?(fabrice) → feedback+
Also, I'd really like to see tests.
Comment on attachment 8638433 [details] [diff] [review] Part 1: Implement gonk/nsClipboard for rich text and raw image (v4) Review of attachment 8638433 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/GonkClipboardData.cpp @@ +47,5 @@ > + > +void > +GonkClipboardData::SetImage(gfx::DataSourceSurface* aDataSource) > +{ > + // Clone a new DataSourceSurface and store it OK. ::: widget/gonk/nsClipboard.h @@ +1,2 @@ > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ > +/* vim:set ts=2 sw=2 sts=2 et: */ OK :) @@ +26,5 @@ > ~nsClipboard() {} > + > +private: > + mozilla::UniquePtr<mozilla::GonkClipboardData> mClipboard; > + nsCOMPtr<imgITools> mImgTool; I think it wouldn't cost too much according to imgITools.h. I will make it as a local variable.
(In reply to [:fabrice] Fabrice Desré from comment #41) > Also, I'd really like to see tests. OK. I wrote a gaia marionette test for "copy image" in attachment 8644902 [details] [diff] [review] (Bug 1121463) already, and will write a mochitest to test nsClipboard::SetData()/GetData() to make sure that this module works on Firefox OS. Thanks for your review.
Blocks: 904183
Blocks: 931116
Handle text/html and image MIME types on gonk/nsClipboard Fix HasDataMatchingFlavors() for my mochitest.
Attachment #8645578 - Attachment is obsolete: true
Add a new context menu option, copy image. Rebased.
Attachment #8645579 - Attachment is obsolete: true
Attachment #8646838 - Flags: review+
Attachment #8646836 - Flags: review?(fabrice)
Attachment #8646839 - Flags: review?(fabrice)
(In reply to Boris Chiou [:boris] from comment #51) > Created attachment 8646839 [details] [diff] [review] > Part 3: Enable test_copyimage.html on gonk/cocoa (v1) > > Enable dom/base/test/test_copyimage.html on b2g emulators and macos I also enable test_copyimage.html on cocoa to avoid other bugs like Bug 1181467.
Enable dom/base/test/test_copyimage.html on b2g emulators and macos
Attachment #8646839 - Attachment is obsolete: true
Attachment #8646839 - Flags: review?(fabrice)
Attachment #8647344 - Flags: review?(fabrice)
Attachment #8646836 - Flags: review?(fabrice) → review+
Attachment #8647344 - Flags: review?(fabrice) → review+
Keywords: checkin-needed
This has caused perma fails in B2G ICS Emulator opt Mochitest Mochitest M(2) 11. '/tools/buildbot/bin/python scripts/scripts/b2g_emulator_unittest.py ...' warnings 51m 44s 31ms 3213 21:42:58 INFO - 578 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: content editable div) 3215 21:43:06 INFO - 579 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data 3218 21:43:06 INFO - 580 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: content editable div) 3233 21:43:13 INFO - 590 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: normal div with designMode:on) 3235 21:43:20 INFO - 591 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data 3238 21:43:20 INFO - 592 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: normal div with designMode:on) 3270 21:43:45 INFO - 614 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: content editable div) 3272 21:43:52 INFO - 615 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data 3275 21:43:52 INFO - 616 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: content editable div) 3290 21:44:00 INFO - 626 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: normal div with designMode:on) 3292 21:44:07 INFO - 627 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data 3295 21:44:07 INFO - 628 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: normal div with designMode:on) 3324 21:44:32 INFO - 650 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: content editable div) 3326 21:44:40 INFO - 651 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data 3329 21:44:40 INFO - 652 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: content editable div) 3344 21:44:48 INFO - 662 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: normal div with designMode:on) 3346 21:44:56 INFO - 663 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data 3349 21:44:56 INFO - 664 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: normal div with designMode:on) 3381 21:45:24 INFO - 686 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: content editable div) 3383 21:45:32 INFO - 687 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data 3386 21:45:32 INFO - 688 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: content editable div) 3401 21:45:41 INFO - 698 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | paste command works (test: normal div with designMode:on) 3403 21:45:49 INFO - 699 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | Timed out while polling clipboard for pasted data 3406 21:45:49 INFO - 700 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_inproc_CopyPaste.html | cut function works (test: normal div with designMode:on) 3677 21:51:07 INFO - 794 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | paste command works (test: normal div with designMode:on) 3679 21:51:14 INFO - 795 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | Timed out while polling clipboard for pasted data 3682 21:51:14 INFO - 796 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | cut function works (test: normal div with designMode:on) 3724 21:51:46 INFO - 830 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | paste command works (test: normal div with designMode:on) 3726 21:51:53 INFO - 831 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | Timed out while polling clipboard for pasted data 3729 21:51:53 INFO - 832 INFO TEST-UNEXPECTED-FAIL | dom/browser-element/mochitest/test_browserElement_oop_CopyPaste.html | cut function works (test: normal div with designMode:on) 27366 22:01:02 ERROR - 08-18 04:58:40.620 F/libc ( 775): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1) 27367 22:01:02 ERROR - This usually indicates the B2G process has crashed 27459 22:01:02 ERROR - Return code: 1 https://treeherder.mozilla.org/logviewer.html#?job_id=2548452&repo=b2g-inbound
(In reply to nigelbabu@gmail.com [:nigelb] from comment #58) > This has caused perma fails in B2G ICS Emulator opt Mochitest Mochitest M(2) > > https://treeherder.mozilla.org/logviewer.html#?job_id=2548452&repo=b2g- > inbound Could you please back out these patches in b2g-inbound? I will fix this problem soon and request check-in needed again. Thanks.
Handle text/html and image MIME types on gonk/nsClipboard Fix tests failed
Attachment #8646836 - Attachment is obsolete: true
Attachment #8649206 - Flags: review+
One other issue this patch encountered: https://treeherder.mozilla.org/logviewer.html#?job_id=2549898&repo=b2g-inbound This one wasn't directly your fault. What happened is that the newly-reenabled test_copyimage.html changed the chunking boundaries such that test_CrossSiteXHR_origin.html went from the end of M(16) to the beginning of M(17). This caused the already-slow test to be even more slowed-down by the emulator startup to the point where it was timing out 90+% of the time. I've since pushed a test change to request a longer timeout to hopefully avoid this problem in the future (since we're effectively one test away from hitting it again on any other branch too), but you'll need to rebase your patch on top of mozilla-central tip to find out. Please do that and push to Try again. |try: -b d -p emulator -u mochitests -t none| will give you a targeted run for only B2G emulator debug mochitests.
Looks like the Try push from comment 63 is still hitting the test_browserElement_inproc_CopyPaste.html failures it was originally backed out for anyway.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #65) > One other issue this patch encountered: > https://treeherder.mozilla.org/logviewer.html#?job_id=2549898&repo=b2g- > inbound > > This one wasn't directly your fault. What happened is that the > newly-reenabled test_copyimage.html changed the chunking boundaries such > that test_CrossSiteXHR_origin.html went from the end of M(16) to the > beginning of M(17). This caused the already-slow test to be even more > slowed-down by the emulator startup to the point where it was timing out > 90+% of the time. I've since pushed a test change to request a longer > timeout to hopefully avoid this problem in the future (since we're > effectively one test away from hitting it again on any other branch too), > but you'll need to rebase your patch on top of mozilla-central tip to find > out. > > Please do that and push to Try again. |try: -b d -p emulator -u mochitests > -t none| will give you a targeted run for only B2G emulator debug mochitests. Ok, I will Try again after fixing test_browserElement_inproc_CopyPaste.html. Thanks for your reminder.
Handle text/html and image MIME types on gonk/nsClipboard. A minor fix for test_browserElement_inproc_CopyPaste.html. Note: Call EmptyClipboard() first in nsClipboard::SetData();
Attachment #8649206 - Attachment is obsolete: true
Attachment #8650301 - Flags: review+
(In reply to Boris Chiou [:boris] from comment #72) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=00afeda8597c Looks like pass all the mochitests.
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S6 (04Sep)
Hi, Tim Looks like the implementation in gecko side was merged. Could you please merge the gaia pull request? Thank you.
Flags: needinfo?(timdream)
Depends on: 1203726
I'm really upset because it's the first time I hear about this feature being done for 2.5. Maybe it's me and I just missed a mail, but I think whoever was responsible for this feature should have been a little more proactive and look in Gaia _in advance_ to see what could break, so that we could plan ahead. So now, big surprise, it's breaking the SMS app. The main reason is that we use contenteditable in a lot of places, and we try to control what's happening in it. I'm sure the SMS app is not the only app to be broken by the change and the lack of planning. I think we should put the functionality behind a pref for 2.5, because it's already quite late in the 2.5 cycle. I don't think we should backout, but just hide behind a pref, so that it's easy to find and fix the issues we have, and plan for a post-2.5 schedule.
I agree with Julien here, perhaps I missed an email to the mailing list, but is the first time I heard about this feature. Right now we are having problems with this feature in SMS: https://bugzilla.mozilla.org/show_bug.cgi?id=1207083 https://bugzilla.mozilla.org/show_bug.cgi?id=1209900 Which makes the SMS app broken, we need to check if other apps like email, contacts, etc. are also affected by this. I do agree with Julien, we should add a pref, we are getting close to the end of feature landing, and we are adding pressure to other teams having their apps broken.
No one is sending emails each time they land something. Be realistic there guys and assume the best. It's unfortunate that it broke the sms app, but now we have options: - figure out what it takes to make that work in the sms app (I think it's nice to copy/paste images to send MMS for instance!) - disable that in 2.5 with a pref. Boris, can you get a patch ready with a pref toggle? We'll let it on in m-c and will turn it off if needed once we branch 2.5
I tried the steps in bug 1209900 to copy an image from a google images search page, then paste that in an email Compose message body. Email uses contenteditable for the compose body, but currently only handles text. The pasting worked, in that I could see it in the compose body. However, the blinking cursor display was over the image, and pressing Enter in this state created new lines above the image. The image filled the width of the compose, maybe that has something to do with the cursor behavior. The Send did not produce an error in the log, but the received message was only the text I put in the body, no image. I believe this is expected, as the email back end only assumes text-only sends at the moment Cc'ing asuth who can provide more background if needed, but end result: email definitely would need at least one bug filed if rich text pasting is expected to work there. Given where 2.5 is at on its timeline, I expect this would be a post-2.5 fix.
I think for the email app (and I guess SMS too?) we want to just add our own explicit "paste" handle, prevent the default paste, and explicitly insert the plaintext ourselves. This makes the limitations clear to the user, although it may make them sad/frustrated and saves us from the many complicated privacy and problem domain issues. (See http://stackoverflow.com/questions/12027137/javascript-trick-for-paste-as-plain-text-in-execcommand for example implementations of this.) We can then spin-off bugs as appropriate to get fancy. For example, email has to deal with the whole multipart/mixed and cid protocol issues if we're doing actual ininline stuff. Plus there's potential sanitization, etc. For now the only viable use-case for the user sharing an image into the email app is to use the browser to generate a "share" activity.
Uh, noting that in the stackoverflow examples, we would absolutely want to be using "insertText" if using execCommand and not "insertHTML".
I filed bug 1210151 to track just pasting the plain text in for email, will have a pull request up shortly.
Blocks: 1210265
(In reply to [:fabrice] Fabrice Desré from comment #81) > No one is sending emails each time they land something. Be realistic there > guys and assume the best. > > It's unfortunate that it broke the sms app, but now we have options: > - figure out what it takes to make that work in the sms app (I think it's > nice to copy/paste images to send MMS for instance!) > - disable that in 2.5 with a pref. > > Boris, can you get a patch ready with a pref toggle? We'll let it on in m-c > and will turn it off if needed once we branch 2.5 Sure. I created Bug 1210265 to handle this case.
(In reply to Boris Chiou [:boris] from comment #86) > (In reply to [:fabrice] Fabrice Desré from comment #81) > > No one is sending emails each time they land something. Be realistic there > > guys and assume the best. I would usually agree with that comment, but not in cases for features that could potentially break multiple apps. Anyway, agree again, let's stop whining and get this shorted :) > > > > It's unfortunate that it broke the sms app, but now we have options: > > - figure out what it takes to make that work in the sms app (I think it's > > nice to copy/paste images to send MMS for instance!) > > - disable that in 2.5 with a pref. Sounds perfect, having the pref, release the pressure for 2.5 and we can plan how to integrate this. > > > > Boris, can you get a patch ready with a pref toggle? We'll let it on in m-c > > and will turn it off if needed once we branch 2.5 > > Sure. I created Bug 1210265 to handle this case. \o/ Thanks a lot!
Depends on: 1211695
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: