Closed Bug 1311610 Opened 8 years ago Closed 8 years ago

[e10s] Can not drag an image into editable area

Categories

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

Product:
Core
Component:
DOM: Copy & Paste and Drag & Drop
Version:
52 Branch
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox49 --- wontfix
firefox50 + verified
firefox51 + verified
firefox52 + verified
firefox53 blocking verified

People

(Reporter: over68, Assigned: overholt)

References

(Blocks 1 open bug)

Details

(Keywords: multiprocess, regression)

Attachments

(5 files)

bug1311610-contenteditableimagednd.PNG
1.27 MB, image/png
Details
bug1311610.patch
2.79 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
patch for aurora
2.79 KB, patch
gchang
: approval-mozilla-aurora+
Details | Diff | Splinter Review
patch for beta
2.79 KB, patch
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review
patch for release
2.79 KB, patch
ritu
: approval-mozilla-release+
Details | Diff | Splinter Review 
Steps to reproduce:

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


Actual results:

Can not drag an image into editable area.
I thought it was already fixed (bug 1259517). Maybe a new regression.
I noticed this just yesterday and tried to find a regression range but couldn't find when it worked. Works in beta though.
Keywords: regression
The problem here is that the patch in the bug 1259517 does not fix the bug on Windows.

So this is not a regression, this bug is specific to Windows.
It doesn't work on Mac for me.
Status: UNCONFIRMED → NEW
status-firefox52: --- → affected
Ever confirmed: true
Keywords: multiprocess
It seems that the patch in bug 1259517 does not work as expected on all platforms.
Indeed, I tried the Windows nightly immediately after bug 1259517 landed and the STR from this bug don't work. Blake, can you please take a look or suggestion another owner for this e10s regression?
status-firefox49: --- → wontfix
status-firefox50: --- → wontfix
status-firefox51: --- → affected
Flags: needinfo?(mrbkap)
(In reply to Neil Deakin from comment #4)
> It doesn't work on Mac for me.

Can you give better STR? On hg revision 9888f1a23 this WFM with the STR from comment 0 (in a debug build on OSX).

I don't have a Windows computer to debug this on. I'm happy to help out if someone wants to debug and see what's up there.
Flags: needinfo?(mrbkap)
I reproduced on Win10 using just the STR from c0. With e10s off, the image shows up in the contenteditable area. With e10s on, nothing happens.
For some reason I was able to reproduce easily with a clean nightly profile of a downloaded 64-bit build but not in a locally-built 32-bit build. I must have done something wrong.
mconley helped me (thanks!) track down that Components.classes["@mozilla.org/editor-utils;1"].getService(Components.interfaces.nsIEditorUtils) is failing (in both the parent and content processes). It seems this is causing HTMLEditor::InsertObject to bail on http://searchfox.org/mozilla-central/source/editor/libeditor/HTMLEditorDataTransfer.cpp#1074 and thus SlurpBlob isn't being called. Oddly, this works in my local build. Blake, any ideas?
Flags: needinfo?(mrbkap)
We just determined that mconley's local build *does* have it (like mine) but that his downloaded build (like mine) *doesn't*.
Does something need to be in browser/installer/package-manifest.in?
It does need to be added there, as well as the mobile version.

And we should have a bug to ensure that some build check is done to ensure that added files are added to the installer packages, as this often causes subtle easily missed errors.
Attached patch bug1311610.patch — — Splinter Review 
I don't know how these files is ordered nor do I know if BINPATH or RESPATH should be used but a try build mystor did for me with BINPATH worked (https://treeherder.mozilla.org/#/jobs?repo=try&revision=231bff8897ca00d14a63555c6f4224c06303af02) :)

I'll file a followup to automate checking for missing additions like this.
Flags: needinfo?(mrbkap)
Attachment #8814497 - Flags: review?(enndeakin)
Assignee: nobody → overholt
I filed bug 1320390 to automate checking for added files in the installer packages.
Attachment #8814497 - Flags: review?(enndeakin) → review+
Blocks: 1259517
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c77fbafe811
Add EditorUtils.* to package-manifest; r=enndeakin
Given how trivial this fix has turned out to be, I'd like to get this back on the radar as a possible 50.1 ride-along.

Also, do we really have no automated tests for EditorUtils?
status-firefox50: wontfix → fix-optional
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7af3fd1de51
Add EditorUtils.* to package-manifest: Use RESPATH instead of BINPATH to fix OS X bustage. r=bustage-fix
https://hg.mozilla.org/mozilla-central/rev/0c77fbafe811
https://hg.mozilla.org/mozilla-central/rev/b7af3fd1de51
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Hello there, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(over68)
Verified locally that it works now. Please request Aurora/Beta/Release approval on this when you get a chance.
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Flags: needinfo?(overholt)
Confirm this bug has been fixed in the latest Nightly build.
Flags: needinfo?(over68)
[Tracking Requested - why for this release]:
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Track 51+/52+ as regression.
tracking-firefox51: ? → +
tracking-firefox52: ? → +
tracking-firefox53: ? → blocking
Attached patch patch for aurora — — Splinter Review 
Approval Request Comment
[Feature/Bug causing the regression]: missing package-manifest.in entry
[User impact if declined]: dragging and dropping images onto content editable areas won't work in e10s
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: see comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: I don't think so
[Why is the change risky/not risky?]:
[String changes made/needed]:
Flags: needinfo?(overholt)
Attachment #8815966 - Flags: approval-mozilla-aurora?
Attached patch patch for beta — — Splinter Review 
Approval Request Comment
[Feature/Bug causing the regression]: missing package-manifest.in entry
[User impact if declined]: dragging and dropping images onto content editable areas won't work in e10s
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: yes, see comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: I don't think so
[Why is the change risky/not risky?]: just adding missing files to the packaged builds
[String changes made/needed]: none
Attachment #8815967 - Flags: approval-mozilla-beta?
[Feature/Bug causing the regression]: missing package-manifest.in entry
[User impact if declined]: dragging and dropping images onto content editable areas won't work in e10s
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: yes, see comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: I don't think so
[Why is the change risky/not risky?]: just adding missing files to the packaged builds
[String changes made/needed]: none
Attachment #8815969 - Flags: approval-mozilla-release?
Comment on attachment 8815967 [details] [diff] [review]
patch for beta

Fix an issue related to e10s about dragging image. Beta51+ and Aurora52+. Should be in 51 beta 6.
Attachment #8815967 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8815966 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
Comment on attachment 8815969 [details] [diff] [review]
patch for release

Critical regression, let's include in 50.1.0
Attachment #8815969 - Flags: approval-mozilla-release? → approval-mozilla-release+
This issue is VERIFIED FIXED in Firefox Aurora 52.0a2 (id: 20161228004005), Firefox Beta 51.b10 (id: 20161222080852) and Firefox Release 50.1.0 (id: 20161208153507) on a WINDOWS 10 x64 machine.
status-firefox50: fixed → verified
status-firefox51: fixed → verified
status-firefox52: fixed → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: