FocusEvent interface is missing

RESOLVED FIXED in mozilla24

Status

()

defect
RESOLVED FIXED
6 years ago
9 months ago

People

(Reporter: erik, Assigned: ayang)

Tracking

({dev-doc-complete, site-compat})

unspecified
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(3 attachments, 11 obsolete attachments)

15.31 KB, patch
ayang
: review+
Details | Diff | Splinter Review
3.39 KB, patch
ayang
: review+
Details | Diff | Splinter Review
810 bytes, patch
ayang
: review+
Details | Diff | Splinter Review
Reporter

Description

6 years ago
focus, blur, focusin, focusout, DOMFocusIn, DOMFocusOut events should all be instance of the FocusEvent interface.
Component: DOM → DOM: Events
Assignee

Updated

6 years ago
Blocks: 687787
Assignee

Comment 1

6 years ago
This patch udpate focus event from Event to FocusEvent in [1].
The focusin/focusout is not support yet and will be fixed later in bug 687787 after fixing this bug.

Test case will adde later.

[1] http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-FocusEvent
Attachment #754340 - Flags: review?(bugs)
Assignee

Updated

6 years ago
Attachment #754340 - Flags: review?(bugs)
Assignee

Updated

6 years ago
Attachment #754340 - Attachment is obsolete: true
Assignee

Comment 2

6 years ago
1. Update focus/blur events. focusin/focusout will be fixed at bug 687787.
2. From my point of view, the nsFocusEvent inherited bases should be:

   nsFocusEvent <-- nsUIEvent <-- nsEvent


But nsDOMUIEvent uses nsGUIEvent, not nsUIEvent. And nsDOMFocusEvent needs to inherit nsUIEvent so I change the nsUIEvent's base class to nsGUIEvent.

   nsFocusEvent <-- nsUIEvent <-- nsGUIEvent <-- nsEvent


Another way is to multi-inherited, but it is ambiguous.

                <-- nsUIEvent   <-- nsEvent
   nsFocusEvent
                <--  nsGUIEvent <-- nsEvent
Attachment #754406 - Flags: review?(bugs)
Assignee

Updated

6 years ago
Attachment #754408 - Attachment is patch: true
Assignee

Updated

6 years ago
Attachment #754408 - Flags: review?(bugs)
Comment on attachment 754406 [details] [diff] [review]
Update focus event to FocusEvent webidl

You must add relatedTarget to cycle collection.
See traverse and unlink in nsDOMEvent.cpp


>+NS_IMETHODIMP nsDOMFocusEvent::GetRelatedTarget(nsIDOMEventTarget * *aRelatedTarget)
NS_IMETHODIMP to its own line.
nsIDOMEventTarget** aRelatedTarget


r- because of missing cycle collection stuff.
Attachment #754406 - Flags: review?(bugs) → review-
Also, make sure we actually use nsFocusEvent when dispatching focus/blur.
Hmm, we want event constructor as mentioned in https://dvcs.w3.org/hg/d4e/raw-file/tip/source_respec.htm.
So please add such to .webidl and add a test for it.
Comment on attachment 754408 [details] [diff] [review]
Test cases for FocusEvent properties.

We need test for the event ctor.
Attachment #754408 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 754406 [details] [diff] [review]
> Update focus event to FocusEvent webidl
> 
> You must add relatedTarget to cycle collection.
> See traverse and unlink in nsDOMEvent.cpp

There should be a test that we don't leak via relatedTarget when a cycle is created too.
Assignee

Comment 9

6 years ago
v2 change list

Major change:
1. add FocusEvent constructor in webidl
2. add Constructor, InitFocusEvent in nsDOMFocusEvent
3. add relatedTarget into cycle collection in nsDOMEvent

Minor change:
4. change name pFocusEvent to tmp in nsDOMFocusEvent::GetRelatedTarget
Attachment #754406 - Attachment is obsolete: true
Attachment #756352 - Flags: review?(bugs)
Assignee

Comment 10

6 years ago
v2 change list
1. add testcase for FocusEvent constructor
2. add cycle reference for leak test
Attachment #754408 - Attachment is obsolete: true
Attachment #756355 - Flags: review?(bugs)
Comment on attachment 756352 [details] [diff] [review]
Update focus event to FocusEvent webidl

::: content/events/src/nsDOMFocusEvent.cpp
>+NS_IMETHODIMP
>+nsDOMFocusEvent::InitFocusEvent(const nsAString& aType,

nsresult because this is not an idl method.

::: content/events/src/nsDOMFocusEvent.h
>+  NS_IMETHOD InitFocusEvent(const nsAString& aType,

Same above.
Assignee

Comment 12

6 years ago
v2.1 change
1. change function type to nsresult for non webidl function.
Attachment #756352 - Attachment is obsolete: true
Attachment #756352 - Flags: review?(bugs)
Attachment #756390 - Flags: review?(bugs)
Comment on attachment 756390 [details] [diff] [review]
Update focus event to FocusEvent webidl


>+nsDOMFocusEvent::nsDOMFocusEvent(mozilla::dom::EventTarget* aOwner,
>+                                 nsPresContext* aPresContext, nsFocusEvent* aEvent)
>+  : nsDOMUIEvent(aOwner, aPresContext, aEvent ?
>+                 static_cast<nsGUIEvent *>(aEvent) :
>+                 static_cast<nsGUIEvent *>(new nsFocusEvent(false, NS_FOCUS_CONTENT)))
>+{
>+  if (aEvent) {
>+    mEventIsInternal = false;
>+  } else {
>+    mEventIsInternal = true;
set mEvent->time here.

>+nsDOMFocusEvent::GetRelatedTarget()
>+{
>+  nsCOMPtr<mozilla::dom::EventTarget> relatedTarget;
>+  nsFocusEvent* tmp = static_cast<nsFocusEvent*>(mEvent);
>+
>+  if (tmp->relatedTarget) {
>+    relatedTarget = do_QueryInterface(tmp->relatedTarget);
do_QueryInterface is null safe, so no need for the 'if' 

>+nsDOMFocusEvent::InitFocusEvent(const nsAString& aType,
>+                            bool aCanBubble,
>+                            bool aCancelable,
>+                            nsIDOMWindow* aView,
>+                            int32_t aDetail,
>+                            nsIDOMEventTarget *aRelatedTarget)
align parameters.
nsIDOMEventTarget* aRelatedTarget

>+{
>+  nsresult rv = nsDOMUIEvent::InitUIEvent(aType, aCanBubble, aCancelable, aView, aDetail);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (aRelatedTarget) {
>+    static_cast<nsFocusEvent*>(mEvent)->relatedTarget = aRelatedTarget;
No need for the 'if'.


>+already_AddRefed<nsDOMFocusEvent>
>+nsDOMFocusEvent::Constructor(const mozilla::dom::GlobalObject& aGlobal,
>+                             const nsAString& aType,
>+                             const mozilla::dom::FocusEventInit& aParam,
>+                             mozilla::ErrorResult& aRv)
>+{
>+  nsCOMPtr<mozilla::dom::EventTarget> t = do_QueryInterface(aGlobal.Get());
>+  nsRefPtr<nsDOMFocusEvent> e = new nsDOMFocusEvent(t, nullptr, nullptr);
>+  e->SetIsDOMBinding();
Move SetIsDOMBinding() to the constructor of nsDOMFocusEvent. All the
FocusEvents should use the new bindings. (there are still few other events which use the old bindings)

>+interface FocusEvent : UIEvent {
>+  // Introduced in DOM Level 3:
>+  readonly attribute EventTarget?   relatedTarget;
>+};
>+
>+[Constructor(DOMString typeArg, optional FocusEventInit focusEventInitDict)]
>+partial interface FocusEvent {
>+};
Just add the Constructor to the interface. No need for this partial interface

>+class nsFocusEvent : public nsUIEvent
>+{
>+public:
>+  nsFocusEvent(bool isTrusted, uint32_t msg)
>+    : nsUIEvent(isTrusted, msg, 0),
>+      fromRaise(false),
>+      isRefocus(false)
>+  {
>+    eventStructType = NS_FOCUS_EVENT;
>+  }
>+
>+  /// The possible related target
>+  nsCOMPtr<nsISupports> relatedTarget;
You can actually make this nsCOMPtr<mozilla::dom::EventTarget> and then you don't need to QI in nsDOMFocusEvent.

All those fixed, r=me
Attachment #756390 - Flags: review?(bugs) → review+
Comment on attachment 756355 [details] [diff] [review]
Test cases for FocusEvent properties.


>+// Focus/Blur constructor test
>+//
>+var focus_event = new FocusEvent("focus",
>+                                 {bubbles: true,
>+                                  cancelable: true,
>+                                  relatedTarget: $("content")});
>+
>+var blur_event = new FocusEvent("blur",
>+                                {bubbles: true,
>+                                 cancelable: true,
>+                                 relatedTarget: $("content")});

Could you actually test that focus_event.relatedTarget == $("content")
and same with blur_event
Attachment #756355 - Flags: review?(bugs) → review+
Assignee

Comment 15

6 years ago
Hi Schien,
My L1 request is on-going. Could you please help me to test it on try-server?
Thanks.
Assignee: nobody → ayang
Attachment #756390 - Attachment is obsolete: true
Flags: needinfo?(schien)
Assignee

Comment 16

6 years ago
Attachment #756355 - Attachment is obsolete: true
Assignee

Comment 17

6 years ago
Comment on attachment 757201 [details] [diff] [review]
Test cases for FocusEvent properties.

Add relatedTarget test to $("content").

According to comment 14, carry r+.
Attachment #757201 - Flags: review+
Assignee

Comment 18

6 years ago
> Hi Schien,
> My L1 request is on-going. Could you please help me to test it on try-server?
> Thanks.

try: -b do -p linux64,macosx64,win32,android,unagi -u mochitests -t none --post-to-bugzilla Bug 855741
Assignee

Comment 19

6 years ago
> >+    mEventIsInternal = true;
> set mEvent->time here.
Done.
 
> >+  if (tmp->relatedTarget) {
> >+    relatedTarget = do_QueryInterface(tmp->relatedTarget);
> do_QueryInterface is null safe, so no need for the 'if' 
Done.
 
> >+                            nsIDOMEventTarget *aRelatedTarget)
> align parameters.
> nsIDOMEventTarget* aRelatedTarget
Done.

> >+  if (aRelatedTarget) {
> >+    static_cast<nsFocusEvent*>(mEvent)->relatedTarget = aRelatedTarget;
> No need for the 'if'.
Done.

> >+  e->SetIsDOMBinding();
> Move SetIsDOMBinding() to the constructor of nsDOMFocusEvent. All the
> FocusEvents should use the new bindings. (there are still few other events
> which use the old bindings)
Done.

> >+[Constructor(DOMString typeArg, optional FocusEventInit focusEventInitDict)]
> >+partial interface FocusEvent {
> >+};
> Just add the Constructor to the interface. No need for this partial interface
Done.
 
> >+  nsCOMPtr<nsISupports> relatedTarget;
> You can actually make this nsCOMPtr<mozilla::dom::EventTarget> and then you
> don't need to QI in nsDOMFocusEvent.
Done.

> All those fixed, r=me
Carry r+ from comment 13.
Attachment #757199 - Attachment is obsolete: true
Attachment #757213 - Flags: review+
Comment on attachment 757213 [details] [diff] [review]
Update focus event to FocusEvent webidl

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

::: content/events/src/nsDOMFocusEvent.cpp
@@ +13,5 @@
> +nsDOMFocusEvent::nsDOMFocusEvent(mozilla::dom::EventTarget* aOwner,
> +                                 nsPresContext* aPresContext, nsFocusEvent* aEvent)
> +  : nsDOMUIEvent(aOwner, aPresContext, aEvent ?
> +                 static_cast<nsGUIEvent *>(aEvent) :
> +                 static_cast<nsGUIEvent *>(new nsFocusEvent(false, NS_FOCUS_CONTENT)))

No space before *

@@ +34,5 @@
> +}
> +
> +/* readonly attribute nsIDOMEventTarget relatedTarget; */
> +NS_IMETHODIMP
> +nsDOMFocusEvent::GetRelatedTarget(nsIDOMEventTarget * *aRelatedTarget)

nsIDOMEventTarget** aRelatedTarget

@@ +47,5 @@
> +{
> +  nsCOMPtr<mozilla::dom::EventTarget> relatedTarget;
> +  nsFocusEvent* tmp = static_cast<nsFocusEvent*>(mEvent);
> +  relatedTarget = do_QueryInterface(tmp->relatedTarget);
> +  return relatedTarget.forget();

mozilla::dom::EventTarget*
nsDOMFocusEvent::GetRelatedTarget()
{
  return static_cast<nsFocusEvent*>(mEvent)->relatedTarget;
}

And then make the XPCOM method use

NS_IF_ADDREF(*aRelatedTarget = GetRelatedTarget());

::: content/events/src/nsDOMFocusEvent.h
@@ +35,5 @@
> +                                                       const mozilla::dom::FocusEventInit& aParam,
> +                                                       mozilla::ErrorResult& aRv);
> +protected:
> +  nsresult InitFocusEvent(const nsAString& aType,
> +                            bool aCanBubble,

Indentation

::: dom/interfaces/events/nsIDOMFocusEvent.idl
@@ +1,1 @@
> +/**

Missing MPL header

@@ +1,2 @@
> +/**
> + * The nsIFocusEvent interface is the datatype for all focus events

There's no nsIFocusEvent in sight.

@@ +2,5 @@
> + * The nsIFocusEvent interface is the datatype for all focus events
> + * in the Document Object Model.
> + *
> + * For more information on this interface please see
> + * http://www.w3.org/TR/DOM-Level-3-Events/

No references to TR/

@@ +7,5 @@
> + */
> +#include "nsIDOMUIEvent.idl"
> +
> +[scriptable, builtinclass, uuid(4faecbd6-1bcd-42d8-bc66-ec6b95050063)]
> +interface nsIDOMFocusEvent : nsIDOMUIEvent

Though, do we actually need this interface?
nsIDOMFocusEvent isn't strictly needed, but I'd kind of like to have it.
At least it doesn't have to be scriptable.
Assignee

Comment 24

6 years ago
Ms2ger, thanks for point out problems.
I've double check it several times. It should follow all coding styles.

> > +                 static_cast<nsGUIEvent *>(new nsFocusEvent(false, NS_FOCUS_CONTENT)))
> 
> No space before *
Done.
 
> nsIDOMEventTarget** aRelatedTarget
Done.
 
> mozilla::dom::EventTarget*
> nsDOMFocusEvent::GetRelatedTarget()
> {
>   return static_cast<nsFocusEvent*>(mEvent)->relatedTarget;
> }
> 
> And then make the XPCOM method use
> 
> NS_IF_ADDREF(*aRelatedTarget = GetRelatedTarget());
Done.
 
> > +  nsresult InitFocusEvent(const nsAString& aType,
> > +                            bool aCanBubble,
> 
> Indentation
Done.

> ::: dom/interfaces/events/nsIDOMFocusEvent.idl
> @@ +1,1 @@
> > +/**
> 
> Missing MPL header
Added.

> > + * The nsIFocusEvent interface is the datatype for all focus events
> 
> There's no nsIFocusEvent in sight.
Removed.

> > + *
> > + * For more information on this interface please see
> > + * http://www.w3.org/TR/DOM-Level-3-Events/
> 
> No references to TR/
Removed.
Attachment #757213 - Attachment is obsolete: true
Assignee

Comment 25

6 years ago
Add FocusEvent to interface test.
Attachment #757315 - Flags: review?(bugs)
Attachment #757315 - Flags: review?(bugs) → review+
Assignee

Comment 27

6 years ago
Updated checkin comment.
Carry r+ from comment 13.
Attachment #757314 - Attachment is obsolete: true
Attachment #758454 - Flags: review+
Assignee

Comment 28

6 years ago
Updated checkin comment.
Carry r+ from comment 14.
Attachment #757201 - Attachment is obsolete: true
Attachment #758457 - Flags: review+
Assignee

Comment 29

6 years ago
Updated checkin comment.
Carry r+ from comment 25.
Attachment #757315 - Attachment is obsolete: true
Attachment #758458 - Flags: review+
Assignee

Updated

6 years ago
Keywords: checkin-needed
Added version notes to the event references.
Thanks for the docs, Yoshino-san! Just putting back ddn as we still need this page: https://developer.mozilla.org/en-US/docs/Web/API/FocusEvent
You need to log in before you can comment on or make changes to this bug.