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)
Tracking
(tracking-b2g:backlog, firefox43 fixed)
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.
Comment 1•11 years ago
|
||
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
Comment 2•11 years ago
|
||
Moving to 1.5 since the UI work won't be ready to use the clipboard.
blocking-b2g: 1.4? → 1.5?
Comment 3•11 years ago
|
||
Adding to backlog, enabling text first.
Blocks: 908549
blocking-b2g: 1.5? → backlog
Updated•10 years ago
|
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
Comment 5•10 years ago
|
||
Still valid I think. We don't have non-text support (for example: image) for b2g now.
Flags: needinfo?(mtseng)
Comment 6•10 years ago
|
||
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
Updated•10 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → boris.chiou
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 15•9 years ago
|
||
This app has some local images and hyperlinks, and a content editable area where you can paste your image and hyperlinks.
Assignee | ||
Comment 16•9 years ago
|
||
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
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8638384 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
Handle text/html and image MIME types on gonk/nsClipboard
Attachment #8636527 -
Attachment is obsolete: true
Assignee | ||
Comment 19•9 years ago
|
||
Add CopyImage() into BrowserElementAPI
Attachment #8636528 -
Attachment is obsolete: true
Assignee | ||
Comment 20•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8638434 -
Flags: feedback?(mtseng)
Comment 21•9 years ago
|
||
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)
Updated•9 years ago
|
Attachment #8638434 -
Flags: feedback?(mtseng) → feedback+
Assignee | ||
Updated•9 years ago
|
Attachment #8638434 -
Flags: review?(kchen)
Comment 22•9 years ago
|
||
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)
Reporter | ||
Comment 23•9 years ago
|
||
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)
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 27•9 years ago
|
||
(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.
Assignee | ||
Updated•9 years ago
|
Attachment #8640395 -
Flags: feedback?(kchen)
Comment 28•9 years ago
|
||
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 29•9 years ago
|
||
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)
Assignee | ||
Comment 30•9 years ago
|
||
Attachment #8638434 -
Attachment is obsolete: true
Attachment #8640395 -
Attachment is obsolete: true
Comment hidden (obsolete) |
Assignee | ||
Comment 32•9 years ago
|
||
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 33•9 years ago
|
||
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+
Comment 34•9 years ago
|
||
Assignee | ||
Comment 35•9 years ago
|
||
Attachment #8640995 -
Attachment is obsolete: true
Attachment #8640997 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8641457 -
Flags: review?(kchen)
Assignee | ||
Updated•9 years ago
|
Attachment #8641456 -
Flags: review?(timdream)
Attachment #8641456 -
Flags: review?(kchen)
Assignee | ||
Comment 36•9 years ago
|
||
(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 :)
Assignee | ||
Updated•9 years ago
|
Attachment #8641456 -
Flags: review?(timdream) → review?(im)
Updated•9 years ago
|
Attachment #8641456 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8641456 -
Flags: review?(im)
Updated•9 years ago
|
Attachment #8641456 -
Flags: review?(kchen) → review+
Comment 37•9 years ago
|
||
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+
Assignee | ||
Comment 38•9 years ago
|
||
(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.
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8638381 -
Attachment is obsolete: true
Comment 40•9 years ago
|
||
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+
Comment 41•9 years ago
|
||
Also, I'd really like to see tests.
Assignee | ||
Comment 42•9 years ago
|
||
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.
Assignee | ||
Comment 43•9 years ago
|
||
(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.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 49•9 years ago
|
||
Handle text/html and image MIME types on gonk/nsClipboard
Fix HasDataMatchingFlavors() for my mochitest.
Attachment #8645578 -
Attachment is obsolete: true
Assignee | ||
Comment 50•9 years ago
|
||
Add a new context menu option, copy image.
Rebased.
Attachment #8645579 -
Attachment is obsolete: true
Attachment #8646838 -
Flags: review+
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Updated•9 years ago
|
Attachment #8646836 -
Flags: review?(fabrice)
Assignee | ||
Updated•9 years ago
|
Attachment #8646839 -
Flags: review?(fabrice)
Assignee | ||
Comment 53•9 years ago
|
||
(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.
Assignee | ||
Comment 54•9 years ago
|
||
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)
Assignee | ||
Comment 55•9 years ago
|
||
Updated•9 years ago
|
Attachment #8646836 -
Flags: review?(fabrice) → review+
Updated•9 years ago
|
Attachment #8647344 -
Flags: review?(fabrice) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 56•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/e00a383520f6
https://hg.mozilla.org/integration/b2g-inbound/rev/3e5d45dcd5c6
https://hg.mozilla.org/integration/b2g-inbound/rev/df6e7f025c65
Keywords: checkin-needed
Comment 58•9 years ago
|
||
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
Comment hidden (obsolete) |
Assignee | ||
Comment 60•9 years ago
|
||
(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.
Assignee | ||
Comment 61•9 years ago
|
||
Handle text/html and image MIME types on gonk/nsClipboard
Fix tests failed
Attachment #8646836 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8649206 -
Flags: review+
Comment hidden (obsolete) |
Assignee | ||
Comment 63•9 years ago
|
||
Comment 64•9 years ago
|
||
Backed out for B2G test_browserElement_inproc_CopyPaste.html failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=2546909&repo=b2g-inbound
https://hg.mozilla.org/integration/b2g-inbound/rev/3ef1a1856a3f
Comment 65•9 years ago
|
||
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.
Comment 66•9 years ago
|
||
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.
Assignee | ||
Comment 67•9 years ago
|
||
(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.
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 71•9 years ago
|
||
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+
Assignee | ||
Comment 72•9 years ago
|
||
Assignee | ||
Comment 73•9 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #72)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=00afeda8597c
Looks like pass all the mochitests.
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 74•9 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/0bd5e39bb91e
https://hg.mozilla.org/integration/b2g-inbound/rev/cc9bc0e573fd
https://hg.mozilla.org/integration/b2g-inbound/rev/1c1b9bf94e4e
Keywords: checkin-needed
Comment 75•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0bd5e39bb91e
https://hg.mozilla.org/mozilla-central/rev/cc9bc0e573fd
https://hg.mozilla.org/mozilla-central/rev/1c1b9bf94e4e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S6 (04Sep)
Comment hidden (obsolete) |
Assignee | ||
Comment 77•9 years ago
|
||
Hi, Tim
Looks like the implementation in gecko side was merged. Could you please merge the gaia pull request?
Thank you.
Flags: needinfo?(timdream)
Comment 78•9 years ago
|
||
Flags: needinfo?(timdream)
Assignee | ||
Updated•9 years ago
|
Comment 79•9 years ago
|
||
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.
Comment 80•9 years ago
|
||
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.
Comment 81•9 years ago
|
||
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
Comment 82•9 years ago
|
||
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.
Comment 83•9 years ago
|
||
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.
Comment 84•9 years ago
|
||
Uh, noting that in the stackoverflow examples, we would absolutely want to be using "insertText" if using execCommand and not "insertHTML".
Comment 85•9 years ago
|
||
I filed bug 1210151 to track just pasting the plain text in for email, will have a pull request up shortly.
Assignee | ||
Comment 86•9 years ago
|
||
(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.
Comment 87•9 years ago
|
||
(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!
You need to log in
before you can comment on or make changes to this bug.
Description
•