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)

Product:
Firefox OS Graveyard
Component:
Runtime
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(tracking-b2g:backlog, firefox43 fixed)

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

People

(Reporter: janjongboom, Assigned: boris)

References

Details

Attachments

(5 files, 24 obsolete files)

CopyImage test app
158.60 KB, application/zip
Details
[gaia] BorisChiou:Bug952456 > mozilla-b2g:master
46 bytes, text/x-github-pull-request
kanru
: review+
timdream
: review+
Details | Review
Part 2: Support copy image in BrowserElement (v4, carry kchen's r+)
5.09 KB, patch
boris
: review+
Details | Diff | Splinter Review
Part 3: Enable test_copyimage.html on gonk/cocoa (v2)
5.80 KB, patch
fabrice
: review+
Details | Diff | Splinter Review
Part 1: Implement gonk/nsClipboard for rich text and raw image. (v8, carry fabrice's r+)
17.41 KB, 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 → ---
tracking-b2g: --- → backlog
Blocks: 1121463
Assignee: nobody → boris.chiou
Depends on: 1181467
Status: NEW → ASSIGNED
Attached file CopyImage test app — 
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) — 
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

    
    

    
  
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)

    
    

    
      
  

    
    

    
  
Hi, Tim

Looks like the implementation in gecko side was merged. Could you please merge the gaia pull request?
Thank you.
Flags: needinfo?(timdream)

    
  
See Also:  → 1201425
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

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: