855741 - FocusEvent interface is missing

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Erik Arvidsson]

focus, blur, focusin, focusout, DOMFocusIn, DOMFocusOut events should all be instance of the FocusEvent interface.

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: Update focus event to FocusEvent webidl

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

Comment 2

[Alfredo Yang (:alfredo)]

Attachment: Update focus event to FocusEvent webidl

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

Comment 3

[Alfredo Yang (:alfredo)]

Attachment: Test cases for FocusEvent properties.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Also, make sure we actually use nsFocusEvent when dispatching focus/blur.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 754408 [details] [diff] [review]
Test cases for FocusEvent properties.

We need test for the event ctor.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 9

[Alfredo Yang (:alfredo)]

Attachment: Update focus event to FocusEvent webidl

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

Comment 10

[Alfredo Yang (:alfredo)]

Attachment: Test cases for FocusEvent properties.

v2 change list
1. add testcase for FocusEvent constructor
2. add cycle reference for leak test

Comment 11

[Masatoshi Kimura [:emk]]

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.

Comment 12

[Alfredo Yang (:alfredo)]

Attachment: Update focus event to FocusEvent webidl

v2.1 change
1. change function type to nsresult for non webidl function.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 15

[Alfredo Yang (:alfredo)]

Attachment: Update focus event to FocusEvent webidl

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

Comment 16

[Alfredo Yang (:alfredo)]

Attachment: Test cases for FocusEvent properties.

Comment 17

[Alfredo Yang (:alfredo)]

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

Add relatedTarget test to $("content").

According to comment 14, carry r+.

Comment 18

[Alfredo Yang (:alfredo)]

> 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

Comment 19

[Alfredo Yang (:alfredo)]

Attachment: Update focus event to FocusEvent webidl

> >+    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.

Comment 20

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

https://tbpl.mozilla.org/?tree=Try&rev=d176cb18c574

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

nsIDOMFocusEvent isn't strictly needed, but I'd kind of like to have it.

Comment 23

[Masatoshi Kimura [:emk]]

At least it doesn't have to be scriptable.

Comment 24

[Alfredo Yang (:alfredo)]

Attachment: Update focus event to FocusEvent webidl

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.

Comment 25

[Alfredo Yang (:alfredo)]

Attachment: Add interface to test_interface

Add FocusEvent to interface test.

Comment 26

[Alfredo Yang (:alfredo)]

try server result
https://tbpl.mozilla.org/?tree=Try&rev=b83b4ab42d8a

Comment 27

[Alfredo Yang (:alfredo)]

Attachment: Update focus event to FocusEvent webidl

Updated checkin comment.
Carry r+ from comment 13.

Comment 28

[Alfredo Yang (:alfredo)]

Attachment: Test cases for FocusEvent properties.

Updated checkin comment.
Carry r+ from comment 14.

Comment 29

[Alfredo Yang (:alfredo)]

Attachment: Add interface to test_interface

Updated checkin comment.
Carry r+ from comment 25.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81871f331faa
https://hg.mozilla.org/integration/mozilla-inbound/rev/d65a30f3db31
https://hg.mozilla.org/integration/mozilla-inbound/rev/542e1529a08b

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/81871f331faa
https://hg.mozilla.org/mozilla-central/rev/d65a30f3db31
https://hg.mozilla.org/mozilla-central/rev/542e1529a08b

Comment 32

[Kohei Yoshino]

Added to the site compatibility doc just in case:
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_24

These docs are already mentioning FocusEvent:
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/focus
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/blur

Comment 33

[Kohei Yoshino]

Added version notes to the event references.

Comment 34

[Jean-Yves Perrier [:teoli]]

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

Comment 35

[Jean-Yves Perrier [:teoli]]

The page has long been created:
https://developer.mozilla.org/en-US/docs/Web/API/FocusEvent
https://developer.mozilla.org/en-US/docs/Web/API/FocusEvent.relatedTarget
https://developer.mozilla.org/en-US/docs/Web/API/FocusEvent.FocusEvent