DragEvent constructor throws "TypeError: Illegal constructor."

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: robincafolla, Assigned: tvaleev, Mentored)

Tracking

35 Branch
mozilla39
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(2 attachments, 5 obsolete attachments)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150112203352

Steps to reproduce:

Any attempt to create a new DragEvent throws an error.

-----

var simulatedEvent = new DragEvent('dragstart');

-----

Firefox 35.0



Actual results:

Throws: TypeError: Illegal constructor.


Expected results:

simulatedEvent should contain a new drag event.
This does work to create a drag event:

var simulatedEvent = new MouseEvent('dragstart', {
    'clientX': 100,                               
    'clientY': 100                                
});                                               

but i'm pretty sure the spec says DragEvent should be a constructor:
http://www.w3.org/TR/2010/WD-html5-20101019/dnd.html#dragevent
https://html.spec.whatwg.org/multipage/interaction.html#the-dragevent-interface
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug][lang=c++]
Yes, there should be a constructor per HTML spec (we try to follow the one produced by WhatWG), but apparently the implementation hasn't been updated since the spec added Constructor to the interface.
Mentor: josh
Hi Josh. Can I try to fix this issue?
As I understand it's needed to add something like:
[Constructor(DOMString type, optional DragEventInit eventInitDict)]
to DragEvent.webidl.
Am I right?
By the way...looks like attached test case have one error.
el.dispatchEvent( simulatedEvent ) should be el.dispatchEvent( simulatedEvent2 )
Flags: needinfo?(josh)
(In reply to Timur Valeev from comment #5)
> Hi Josh. Can I try to fix this issue?
> As I understand it's needed to add something like:
> [Constructor(DOMString type, optional DragEventInit eventInitDict)]
> to DragEvent.webidl.
> Am I right?
> By the way...looks like attached test case have one error.
> el.dispatchEvent( simulatedEvent ) should be el.dispatchEvent(
> simulatedEvent2 )

Ah, appologies for the error in the test case.
Yep, that's right, and you'll need to implement a Constructor static method in dom/events/DragEvent.cpp as well.
Flags: needinfo?(josh)
I see. Thank you Josh. 

But for me is not clear what arguments should have DragEvent constructor. Because I cannot find description of this constructor (for example I found description of MouseEvent constructor https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/MouseEvent).

So on my own risk I create a draft patch of the DragEvent.webidl that describe the constructor :).
And if it is ok I will add cpp implementation.
Attachment #8569278 - Flags: review-
Flags: needinfo?(josh)
You should refer to the specification in comment 3 :)
Flags: needinfo?(josh)
Oh...sorry for the carelessness. I check a link only from comment 2.
I create new patch with webidl and cpp implemetation of the DragEvent constructor.
Attachment #8569798 - Flags: review+
Attachment #8569798 - Flags: review?(josh)
Comment on attachment 8569798 [details] [diff] [review]
1135627_add_dragevent_constructor_v0.2.patch

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

Nice! I'm redirecting the review to one of the DOM peers (per https://wiki.mozilla.org/Modules/Core#Document_Object_Model).
Attachment #8569798 - Flags: review?(josh)
Attachment #8569798 - Flags: review?(bugs)
Attachment #8569798 - Flags: feedback+
Note: you won't want xpcshell tests for the try run, since those don't run in a DOM global environment and the DragEvent DOM object won't be tested. You would probably want mochitests and web-platform-tests instead, since those run in regular web content environments.
Comment on attachment 8569798 [details] [diff] [review]
1135627_add_dragevent_constructor_v0.2.patch

>+DragEvent::Constructor(const GlobalObject& aGlobal,
>+                        const nsAString& aType,
>+                        const DragEventInit& aParam,
>+                        ErrorResult& aRv)
tiny nit, align the arguments. For some reason the latter 3 ones have one extra space before them.



>+  aRv = e->InitDragEvent(aType, aParam.mBubbles, aParam.mCancelable,
>+                    aParam.mView, aParam.mDetail, aParam.mScreenX,
>+                    aParam.mScreenY, aParam.mClientX, aParam.mClientY,
>+                    aParam.mCtrlKey, aParam.mAltKey, aParam.mShiftKey,
>+                    aParam.mMetaKey, aParam.mButton, aParam.mRelatedTarget,
>+                    aParam.mADataTransfer);
Align params.

>+  static already_AddRefed<DragEvent> Constructor(const GlobalObject& aGlobal,
>+                                                  const nsAString& aType,
>+                                                  const DragEventInit& aParam,
>+                                                  ErrorResult& aRv);
align


>+dictionary DragEventInit : MouseEventInit
>+{
>+  DataTransfer? aDataTransfer = null;
it is dataTransfer in the spec, not aDataTransfer


Could you add a simple test for DragEvent to dom/events/test/test_eventctors.html


Looking good, but could take a look at the new patch.
Attachment #8569798 - Flags: review?(bugs) → review-
I add simple test for DragEvent but frankly speaking I'm not sure that it's ok.
Attachment #8569278 - Attachment is obsolete: true
Attachment #8569798 - Attachment is obsolete: true
Attachment #8570961 - Flags: review?(bugs)
Comment on attachment 8570961 [details] [diff] [review]
1135627_add_dragevent_constructor_v0.3.patch

>+dictionary DragEventInit : MouseEventInit
>+{
>+  DataTransfer? DataTransfer = null;

The property name is dataTransfer in the spec, not DataTransfer
Attachment #8570961 - Flags: review?(bugs) → review-
Sorry for the carelessness.
Attachment #8570961 - Attachment is obsolete: true
Attachment #8571539 - Flags: review?(bugs)
Assignee: nobody → tvaleev
Attachment #8571539 - Flags: review?(bugs) → review+
Attachment #8571539 - Flags: checkin+
Hi Josh. Looks like try build is completed. But I see 4 unclassified warnings. Are they ok? And what my next steps?
Flags: needinfo?(josh)
The M-e10s 2 errors are all known intermittent failures (if you click on it, you'll see that there are suggestions of known intermittents for each listed failure). However, the W 2 errors are all failures that need to be addressed - you'll note that if you click on them, many of the error messages mention DragEvent :) In this case, these are tests unexpectedly passing; previously they were annotated as expected failures, and your changes have improved the sitatuation. You should update the expected test results in http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/meta/ - there are .ini files corresponding to each .html file that gets executed by the test harness, and you can just remove the lines that correspond to the new successful tests (ie. the [test name] line, and the one following).
Flags: needinfo?(josh)
Ok. I create new patch that update the expected test results.
Attachment #8571539 - Attachment is obsolete: true
Attachment #8572493 - Flags: review?(josh)
I delete only lines that correspond to the new successful DragEvent tests (there are some another warnings related to mouse and ui event tests). Am I right?
Flags: needinfo?(josh)
Well, those failures will prevent the patch from being committed, and they actually are testing properties of the DragEvent interface if you look at the actual test source :)
Flags: needinfo?(josh)
I see :). This is the new patch.
Attachment #8572493 - Attachment is obsolete: true
Attachment #8572493 - Flags: review?(josh)
Attachment #8572620 - Flags: review?(josh)
Hi Josh. I'm confused :). Try build shown that there are some issues (for example bc2 browser_test_new_window_from_content.js test exceeded the timeout threshold). 
So I repeat try build for linux64 https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f88587e0d88. And I can't find them. Is it ok?
Flags: needinfo?(josh)
Yes, all of those are known intermittent failures, and we have bugs tracking them (those are what show up in the blue box when you click on the orange test suites). This patch looks good! You can add the `checkin-needed` keyword to the `Keywords` field at the top of the bug, and it will be checked in by a sheriff.
Flags: needinfo?(josh)
Attachment #8572620 - Flags: review?(josh) → review+
I see. Thank you!
Keywords: checkin-needed
I had to add smaug's review to the commit message to get around the DOM peer hook. Hope that was the right thing to do here.
Flags: needinfo?(bugs)
yup, that is fine.
Flags: needinfo?(bugs)
https://hg.mozilla.org/mozilla-central/rev/4ed6545b8b5b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
I got the bug number wrong (was supposed to be Bug 1136757), sorry for push.
You need to log in before you can comment on or make changes to this bug.