Closed
Bug 1199729
Opened 9 years ago
Closed 7 years ago
DataTransfer's data store mode remains readonly after drop had happened
Categories
(Core :: DOM: Events, defect)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: nika, Assigned: nika)
References
(Blocks 1 open bug)
Details
Attachments
(8 files, 1 obsolete file)
3.80 KB,
text/html
|
Details | |
11.01 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
2.35 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
7.88 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
9.10 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
5.07 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
1.98 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
11.40 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
According to the spec at https://html.spec.whatwg.org/multipage/interaction.html#drag-data-store-mode the drag data store should be able to exist in one of three modes, Read-Write, Read-Only and Protected. Currently our DataTransfer implementation always exists in either Read-Write or Read-Only.
Comment 1•7 years ago
|
||
The implication here is that properties like `dataTransfer.files.length` remains in tact in Gecko whereas Blink/WebKit changes to 0. So there's a serious interoperability concern here.
Summary: DataTransfer protected mode → DataTransfer's data store mode remains readonly after drop had happened
Comment 2•7 years ago
|
||
See steps 3 and 13 in https://html.spec.whatwg.org/multipage/dnd.html#fire-a-dnd-event: > 3. If e is dragstart, set the drag data store mode to the read/write mode. > If e is drop, set the drag data store mode to the read-only mode. ... > 13. Set the drag data store mode back to the protected mode if it was changed in the first step. Note that the definition of data store mode says, among other things, that data store mode is protected mode upon initialization: https://html.spec.whatwg.org/multipage/dnd.html#drag-data-store-mode "When a drag data store is created, ... its drag data store mode is protected mode".
Comment 3•7 years ago
|
||
I think we should see how Edge behaves here. If they agree with WebKit and Blink, we should probably modify our behavior to match them and the spec. Otherwise we should discuss with Ryosuke about the best behavior and try to get one side to agree to change. Andrew, any chance you can see if you can find someone to work on this please? Thanks!
Flags: needinfo?(overholt)
Comment 4•7 years ago
|
||
Michael is in a decent place to look into this. See also: https://github.com/whatwg/html/issues/2918
Flags: needinfo?(overholt) → needinfo?(michael)
Assignee | ||
Comment 5•7 years ago
|
||
I've written a patch which I _think_ adds in the protected mode state correctly, however it's been somewhat frustrating to test in an automated fashion. The test which we have for DataTransfer being readonly is http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/dom/events/test/test_DataTransferItemList.html. Unfortunately that test doesn't actually use the correct codepaths for dispatching Drag events, instead emulating them. This means that, for example, every callback for each of the dragstart/dragover/drop events has the same DataTransfer in that test, rather than separate ones, as is the correct implementation, so I cannot test some things :-/. I'm not sure how I'm going to test this because of that. Perhaps I'll resort to manual testing.
Flags: needinfo?(michael)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → michael
Assignee | ||
Comment 6•7 years ago
|
||
I've written a manual test for this behavior, which I will upload. The test currently tests for chrome's behavior (which matches the spec the most closely as far as I can tell). Safari and Chrome both seem to follow the spec (as far as this test is concerned), while both Firefox and Edge don't follow the spec (in different ways). Firefox doesn't have a protected state (or disconnected) state, and thus the DataTransfer becomes readonly after a dragstart event, but otherwise remains readonly indefinitely. Edge doesn't ever change the state of a DataTransfer after the event is done firing, so readwrite DataTransfers remain readwrite indefinitely, readonly ones remain readonly, and protected ones remain protected (as far as I can tell). annevk, should we be changing to match chrome's behavior here? We don't exactly follow the same model as the spec internally (and it would be a decently large amount of work to change that), but with some effort we can probably fake it.
Flags: needinfo?(annevk)
Assignee | ||
Comment 7•7 years ago
|
||
oops - uploaded the wrong file :-S
Attachment #8901278 -
Attachment is obsolete: true
Comment 8•7 years ago
|
||
At this point it's usually safer for us to be closer to Chrome and Safari than Edge, so I'd say we should go for that. And maybe have some kind of tracking bug for the remaining work since it'll become a problem sooner or later.
Flags: needinfo?(annevk)
Assignee | ||
Comment 9•7 years ago
|
||
Attachment #8905139 -
Flags: review?(amarchesini)
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8905140 -
Flags: review?(amarchesini)
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8905141 -
Flags: review?(amarchesini)
Assignee | ||
Comment 12•7 years ago
|
||
Attachment #8905142 -
Flags: review?(amarchesini)
Assignee | ||
Comment 13•7 years ago
|
||
Attachment #8905143 -
Flags: review?(amarchesini)
Assignee | ||
Comment 14•7 years ago
|
||
Attachment #8905144 -
Flags: review?(amarchesini)
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #8905145 -
Flags: review?(amarchesini)
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Michael Layzell [:mystor] from comment #12) > Created attachment 8905142 [details] [diff] [review] > Part 4: Update EventUtils to simulate drag events more accurately This is definitely the ugliest part of the patch, and the part which I ended up spending the most time on. We effectively have 2 implementations of event dispatch for drag events in gecko, one is used by the actual browser, and the other is this JS implementation in EventUtils which is only used in tests. It used to be not very accurate, and took advantage of the fact that you could read from a DataTransfer after the event had returned to fake the events after dragstart. Now that we clear the DataTransfer after each event, this test had to be changed in order to actually do what the spec requires. This new implementation tries to be much closer to the actual implementation used in firefox, sharing as much code as possible.
Updated•7 years ago
|
Attachment #8905139 -
Flags: review?(amarchesini) → review+
Updated•7 years ago
|
Attachment #8905140 -
Flags: review?(amarchesini) → review+
Updated•7 years ago
|
Attachment #8905141 -
Flags: review?(amarchesini) → review+
Comment 17•7 years ago
|
||
Comment on attachment 8905142 [details] [diff] [review] Part 4: Update EventUtils to simulate drag events more accurately Review of attachment 8905142 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/events/DataTransfer.cpp @@ +678,5 @@ > +already_AddRefed<DataTransfer> > +DataTransfer::MozCloneForEvent(const nsAString& aEvent, ErrorResult& aRv) > +{ > + nsCOMPtr<nsIAtom> atomEvt = NS_Atomize(aEvent); > + if (!atomEvt) { NS_WARN_IF @@ +686,5 @@ > + EventMessage eventMessage = nsContentUtils::GetEventMessage(atomEvt); > + > + RefPtr<DataTransfer> dt; > + nsresult rv = Clone(mParent, eventMessage, false, false, getter_AddRefs(dt)); > + if (NS_FAILED(rv)) { NS_WARN_IF
Attachment #8905142 -
Flags: review?(amarchesini) → review+
Updated•7 years ago
|
Attachment #8905143 -
Flags: review?(amarchesini) → review+
Comment 18•7 years ago
|
||
Comment on attachment 8905144 [details] [diff] [review] Part 6: Add some comments to DataTransfer to clarify use of methods Review of attachment 8905144 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/events/DataTransfer.h @@ +75,5 @@ > DataTransfer(); > > // this constructor is used only by the Clone method to copy the fields as > // needed to a new data transfer. > + // NOTE: Do not call this method directly. Well... you cannot, right? it's protected.
Attachment #8905144 -
Flags: review?(amarchesini) → review+
Updated•7 years ago
|
Attachment #8905145 -
Flags: review?(amarchesini) → review+
Comment 19•7 years ago
|
||
Pushed by michael@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/65372115ac3b Part 1: Add the Protected mode to DataTransfer, r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/00970264c7ea Part 2: Respect Protected mode in content documents, r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4ebd8f3dc7 Part 3: Clear the DataTransfer after handling Drag and Clipboard events, r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/5d920dc2ba45 Part 4: Update EventUtils to simulate drag events more accurately, r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/83f49da70519 Part 5: Update tests for new DataTransfer behaviour, r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee09b4795cb Part 6: Add some comments to DataTransfer to clarify use of methods, r=baku https://hg.mozilla.org/integration/mozilla-inbound/rev/5a69e957b198 Part 7: Add manual web-platform-tests for Drag Data Store status, r=baku
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/65372115ac3b https://hg.mozilla.org/mozilla-central/rev/00970264c7ea https://hg.mozilla.org/mozilla-central/rev/aa4ebd8f3dc7 https://hg.mozilla.org/mozilla-central/rev/5d920dc2ba45 https://hg.mozilla.org/mozilla-central/rev/83f49da70519 https://hg.mozilla.org/mozilla-central/rev/0ee09b4795cb https://hg.mozilla.org/mozilla-central/rev/5a69e957b198
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 21•7 years ago
|
||
Andrea and Michael, I have two questions: We needed to make changes to Thunderbird's dragging and dropping of messages into folders in bug 1398579. Basically we grabbed the transfer data (this._currentTransfer = aEvent.dataTransfer;) during the "dragover" event in a folder tree. When then used that copy at drop time. Somehow that stopped working, you must empty out the object presented to "dragover". We fixed our problem by grabbing the data again during the droop event. Could you please explain why the previous approach stopped working. Also I'd like to know that the protection status is during the "dragover". We're able to read the data, but I'm a little confused looking at https://hg.mozilla.org/mozilla-central/rev/65372115ac3b#l2.12 since "dragover" isn't listed in the case statement and hence would trigger DataTransfer::Mode::Protected with no access. What am I missing?
Flags: needinfo?(michael)
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 22•7 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #21) > Andrea and Michael, I have two questions: > > We needed to make changes to Thunderbird's dragging and dropping of messages > into folders in bug 1398579. > > Basically we grabbed the transfer data (this._currentTransfer = > aEvent.dataTransfer;) during the "dragover" event in a folder tree. When > then used that copy at drop time. Somehow that stopped working, you must > empty out the object presented to "dragover". We fixed our problem by > grabbing the data again during the droop event. Could you please explain why > the previous approach stopped working. This is the intended behaviour after this patch. Before this patch we incorrectly failed to disconnect the DataTransfer from the Drag Data Store after a drag event has been dispatched, while now we correctly disconnect it, causing the DataTransfer to appear to be empty. This new behavior is what is specified in the HTML spec, and what is implemented by chromium. > Also I'd like to know that the protection status is during the "dragover". > We're able to read the data, but I'm a little confused looking at > https://hg.mozilla.org/mozilla-central/rev/65372115ac3b#l2.12 since > "dragover" isn't listed in the case statement and hence would trigger > DataTransfer::Mode::Protected with no access. What am I missing? The DataTransfer::Mode::Protected mode does not have no access, it has limited access. ReadWrite (dragstart, cut, copy): - Can read all types - Can read all data - Can write all changes ReadOnly (paste, drop): - Can read all types - Can read all data Protected (everything else): - Can read all types - Can read all data (in chrome contexts) This means that it is possible to determine how much data, and what types of data, are available during dragover, but reading actual files and/or strings from the DataTransfer is not possible until the `drop` event.
Flags: needinfo?(michael)
Flags: needinfo?(amarchesini)
Comment 23•7 years ago
|
||
Hey Michael, thanks a lot for taking the time to explain, much appreciated.
You need to log in
before you can comment on or make changes to this bug.
Description
•