Closed Bug 1199729 Opened 5 years ago Closed 3 years ago

DataTransfer's data store mode remains readonly after drop had happened

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

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)

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.
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
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".
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)
Michael is in a decent place to look into this. See also: https://github.com/whatwg/html/issues/2918
Flags: needinfo?(overholt) → needinfo?(michael)
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: nobody → michael
Attached file manual test (obsolete) —
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)
Attached file manual test
oops - uploaded the wrong file :-S
Attachment #8901278 - Attachment is obsolete: true
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)
(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.
Attachment #8905139 - Flags: review?(amarchesini) → review+
Attachment #8905140 - Flags: review?(amarchesini) → review+
Attachment #8905141 - Flags: review?(amarchesini) → review+
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+
Attachment #8905143 - Flags: review?(amarchesini) → review+
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+
Attachment #8905145 - Flags: review?(amarchesini) → review+
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
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)
(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)
Hey Michael, thanks a lot for taking the time to explain, much appreciated.
Depends on: 1398883
You need to log in before you can comment on or make changes to this bug.