Closed Bug 1259517 Opened 8 years ago Closed 8 years ago

[e10s] Drag an image into editable area doesn't do anything

Categories

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P1)

Product:
Core
Component:
DOM: Copy & Paste and Drag & Drop
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
e10s + ---
firefox48 --- wontfix
firefox49 --- verified
firefox50 --- verified

People

(Reporter: enndeakin, Assigned: mrbkap)

References

(Depends on 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(2 files, 2 obsolete files)

Patch v2
14.65 KB, patch
baku
: review+
Details | Diff | Splinter Review
m-a
12.60 KB, patch
mrbkap
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
Steps:

1. Open a contenteditable area such as data:text/html,<body contenteditable>
2. Drag an image file from the filesystem to to content area

Expected:
  The image appears in the editable area. Selecting it shows a resizing box to resize and edit it.

Actual:
 The image doesn't appear at all.

Works ok in non-e10s. I tested on Mac.
Note that cut/paste works in a sense OK. It shows a placeholder image but does the same on e10s versus non-e10s. Only drag/drop is different.
Jeff, do you have a sense of where to prioritize this?
Assignee: nobody → mrbkap
Flags: needinfo?(jgriffiths)
I can repro, I think it should be a p1 as it's a regression but a rarer use case.

Chrome, fyi, just opens the file:// uri to the dragged image. Unsure spec-wise if this drag behaviour is well-defined or not.
Flags: needinfo?(jgriffiths)
Priority: -- → P2
Priority: P2 → P1
Whiteboard: btpp-active
Assignee: mrbkap → jimmyw22
Assignee: jimmyw22 → mrbkap
Attached patch WIP (obsolete) — — Splinter Review 
Working with blobs in C++ is awful. Firstly, as far as I can tell, it is basically impossible to do this synchronously. Blobs just work very hard to block the main thread. Secondly, I tried keeping this in C++ and got half the way through re-implementing FileReader in editor before realizing that it just wasn't worth it. There would have been a lot of code duplicated, and some of it is tricky input/output stream stuff. So, this seemed like the path of least resistance.

This patch allows for a small chance we could run into web compat bugs (pages assuming that a drop event will synchronously drop the data: URL into the target). In that case, we can go back to a model where we delay sending the dragend(?) event in the child until we slurp up the blob data and stick that somewhere in the drag event.

Andrea, is it OK to pass a null parent to Blob::Create? I'll add more error checking, but do you see things here that could be simplified (or are otherwise wrong)?
Attachment #8756143 - Flags: feedback?(amarchesini)
> Working with blobs in C++ is awful

This is not true :)

> basically impossible to do this synchronously.

Oh yes! And this is a feature!

> Andrea, is it OK to pass a null parent to Blob::Create?

Better to have a parent object. Any reason why you don't have it? Probably it's better to use BlobImpl until the Blob object is really needed and at that point create the Blob object.

I'm reviewing your code now.
Comment on attachment 8756143 [details] [diff] [review]
WIP

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

Instead doing this, what about if you create and use a blob URL?
Then you just do: aOutput.AssignLiteral("<IMG src=\""); + the blob URL + "\" alt="\"\">");
This approach uses a lot of memory and it requires to have the whole blob stored into a string.

::: editor/libeditor/nsHTMLDataTransfer.cpp
@@ +1013,5 @@
>  
> +namespace {
> +
> +nsresult
> +ImgFromData(const nsACString& aType, const nsACString& aData, nsString& aOutput)

nsAString& aOutput

@@ +1039,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIEDITORBLOBLISTENER
> +
> +private:
> +  ~BlobReader() {

{ in a new line

@@ +1042,5 @@
> +private:
> +  ~BlobReader() {
> +  }
> +
> +  nsCOMPtr<BlobImpl> mBlob;

RefPtr

@@ +1064,5 @@
> +  , mSourceDoc(aSourceDoc)
> +  , mDestinationNode(aDestinationNode)
> +  , mDestOffset(aDestOffset)
> +  , mDoDeleteSelection(aDoDeleteSelection)
> +{

MOZ_ASSERT(aBlobImpl);
MOZ_ASSERT(aEditor);
some other MOZ_ASSERTs
Attachment #8756143 - Flags: feedback?(amarchesini)
> Instead doing this, what about if you create and use a blob URL?
> Then you just do: aOutput.AssignLiteral("<IMG src=\""); + the blob URL + "\"
> alt="\"\">");

I was thinking about this, but don't we have to worry about leaking the blob for ever and ever since there's no good time to revokeObjectURL? There's also a compat risk since it's possible that sites expect to be able to use the data: URI.

I'm happy to implement it though. It'd be much easier :-)
Flags: needinfo?(amarchesini)
About compat risk, I don't know, but blob URLs have been out there since ages.
About leaking, in order to create a blob URL you have to register it to some global. And you have a document, so probably you can do something like:

  nsAutoCString url;
  nsresult rv = nsHostObjectProtocolHandler::AddDataEntry(NS_LITERAL_CSTRING("blob"), theBlobObject,
                                                          document->NodePrincipal(), url);
  the_nsIGlobalObject_of_the_document->RegisterHostObjectURI(url);
Flags: needinfo?(amarchesini)
I would expect there are cases where one would take the innerHTML of the editable area and submit it to a remote site and expect any images to be accessible. A data url would contain a usable image, whereas a blob would not, no?
(In reply to Neil Deakin from comment #9)
> I would expect there are cases where one would take the innerHTML of the
> editable area and submit it to a remote site and expect any images to be
> accessible. A data url would contain a usable image, whereas a blob would
> not, no?

Yeah, given that editable areas are more concerned with the source being usable, I'm going to stick with the current approach. It would be possible for a site to handle the drop event and do something better with it (like inserting an image with a blob URL) and I would imagine that most serious contenteditable-using sites do so.
Attached patch Patch v1 (obsolete) — — Splinter Review 

        Attachment #8761404 -
        Flags: review?(amarchesini)

        Attachment #8756143 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8761404 [details] [diff] [review]
Patch v1

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

Looks good to me. The only thing is that if we fail, I would like to report the error to the console.
Can you introduce an errorCallback in nsIEditorUtils? Then in BlobReader, you can just call nsContentUtils::ReportToConsole().

::: editor/libeditor/EditorUtils.js
@@ +20,5 @@
> +
> +  slurpBlob(aBlob, aScope, aListener) {
> +    let reader = new aScope.FileReader();
> +    reader.addEventListener("load", (event) => {
> +      aListener.onResult(event.target.result);

do we want to propagate the error as well?

        Attachment #8761404 -
        Flags: review?(amarchesini)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2
        — 
          — Splinter Review
      

    


  
So, like this?

        Attachment #8762623 -
        Flags: review?(amarchesini)

        Attachment #8761404 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8762623 [details] [diff] [review]
Patch v2

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

do you think you can provide a test? For success and failure?

::: editor/libeditor/nsHTMLDataTransfer.cpp
@@ +1032,5 @@
> +class BlobReader final : public nsIEditorBlobListener
> +{
> +public:
> +  BlobReader(BlobImpl* aBlob, nsHTMLEditor* aEditor,
> +             bool aIsSafe, nsIDOMDocument *aSourceDoc,

nsIDOMDocument*<space>aSourceDoc

@@ +1033,5 @@
> +{
> +public:
> +  BlobReader(BlobImpl* aBlob, nsHTMLEditor* aEditor,
> +             bool aIsSafe, nsIDOMDocument *aSourceDoc,
> +             nsIDOMNode *aDestinationNode, int32_t aDestOffset,

here too.

@@ +1056,5 @@
> +
> +NS_IMPL_ISUPPORTS(BlobReader, nsIEditorBlobListener)
> +
> +BlobReader::BlobReader(BlobImpl* aBlob, nsHTMLEditor* aEditor,
> +                       bool aIsSafe, nsIDOMDocument *aSourceDoc,

same here.

@@ +1057,5 @@
> +NS_IMPL_ISUPPORTS(BlobReader, nsIEditorBlobListener)
> +
> +BlobReader::BlobReader(BlobImpl* aBlob, nsHTMLEditor* aEditor,
> +                       bool aIsSafe, nsIDOMDocument *aSourceDoc,
> +                       nsIDOMNode *aDestinationNode, int32_t aDestOffset,

same here.

        Attachment #8762623 -
        Flags: review?(amarchesini) → review+

    
    

    
  
(In reply to Andrea Marchesini (:baku) from comment #14)
> do you think you can provide a test? For success and failure?

Testing drag/drop turned out to be harder than I thought :-( I'm going to punt on that for now and just get this patch checked in.

    
    

    
  
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29bf50e2f7cd
Make dropping images in editors work in e10s. r=baku

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/29bf50e2f7cd
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox50:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

    
    

    
  
Blake, should we uplift this e10s bug fix from Nightly 50 to Aurora 49 or even Beta 48? We plan to enable e10s by default for some release channel users with Firefox 48.

          status-firefox48:
          --- → ?

          status-firefox49:
          --- → ?
Flags: needinfo?(mrbkap)

    
    

    
  
I would like to uplift this but I don't know how the uplift works given that this changes a .properties file. Is the uplift possible?
Flags: needinfo?(mrbkap)
Flags: needinfo?(l10n)

    
    

    
  
Uplift is possible, but strongly discouraged. What's the user-impact of this string? Just something in the error console?

In that case, I don't think that l10n should block, but I'm still not sure how much impact this has on users, and what the risk profile is. Up to release management.
Flags: needinfo?(l10n)

    
    

    
  
(In reply to Axel Hecht [:Pike] from comment #20)
> Uplift is possible, but strongly discouraged. What's the user-impact of this
> string? Just something in the error console?
> 
> In that case, I don't think that l10n should block, but I'm still not sure
> how much impact this has on users, and what the risk profile is. Up to
> release management.

Agreed on the l10n front - this is a developer-facing string - strings like these apparently have a low risk of being translated anyway, or at least that's my impression. IMO we should only block uplift for technical risks.

    
    

    
  
Comment on attachment 8762623 [details] [diff] [review]
Patch v2

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Dropping images onto contenteditable elements in child processes doesn't do anything.
[Describe test coverage new/current, TreeHerder]: None :(
[Risks and why]: Low-to-medium risk. This is probably not a very common action, most sites that allow dropping images probably handle the drop themselves (the way our editor handles this is kind of crazy), so there could be latent bugs hiding in the code
[String/UUID change made/needed]: One string exposed in the error console in the case of failure (I don't know how it's possible for this to happen, maybe something like the file being deleted after the drag starts but before it's dropped?).

        Attachment #8762623 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Passing by note: given the target and visibility, it would be even cleaner to just have a patch for Aurora that hardcodes the string without modifying dom.properties. 

It would have no impact for l10n (no warnings for missing strings mid cycle), it would be harder for devs since you need to cook a specific patch for aurora. Your choice based also on the amount of work requested to create this kind of patch.

    
    

    
  
Blake would you mind hard coding the string as flod suggests?
Flags: needinfo?(mrbkap)

    
    

    
  
Baku, can you help out here, so we can get this at least into aurora? Not sure Blake is around.
Flags: needinfo?(amarchesini)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        m-a
        — 
          — Splinter Review
      

    


  
Flags: needinfo?(amarchesini)

    
    

    
  
Comment on attachment 8769873 [details] [diff] [review]
m-a

I wrote the same patch locally before seeing that Andrea had beaten me to it.

Approval Request Comment: see above.
Flags: needinfo?(mrbkap)

        Attachment #8769873 -
        Flags: review+

        Attachment #8769873 -
        Flags: approval-mozilla-aurora?

        Attachment #8762623 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 8769873 [details] [diff] [review]
m-a

Thanks Blake and Andrea! Good to have image drag work better with e10s.

        Attachment #8769873 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
Setting status-firefox48=wontfix. I assume we won't uplift to Beta 48 because Blake only requested Aurora 49 uplift in comment 22.

          status-firefox48:
          ? → wontfix
Flags: qe-verify+

    
    

    
  
Reproduced this issue on Mac OS X 10.9.5 using Fx 48.0.1.

Confirming the fix using latest 50.0a2 Aurora (build ID 20160822004020) and Fx 49.0b5 (build ID 20160818050015) across platforms.
Status: RESOLVED → VERIFIED

          status-firefox49:
          fixed → verified

          status-firefox50:
          fixed → verified
Flags: qe-verify+

    
    

    
  
Is there a follow-up for landing a test for this bug?  It turns out that the fix didn't really work (because of bug 1311610) unless if you were using a local build.  A test would have uncovered this issue.  :(
Flags: needinfo?(mrbkap)

    
  
Depends on: 1311610
Depends on: 1329053

    
    

    
  
I filed bug 1329053 on comment 32.
Flags: needinfo?(mrbkap)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: