Last Comment Bug 855741 - FocusEvent interface is missing
: FocusEvent interface is missing
Status: RESOLVED FIXED
: dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla24
Assigned To: Alfredo Yang (:alfredo)
:
Mentors:
http://www.w3.org/TR/DOM-Level-3-Even...
Depends on:
Blocks: 687787 962251
  Show dependency treegraph
 
Reported: 2013-03-28 09:44 PDT by Erik Arvidsson
Modified: 2015-05-10 23:12 PDT (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Update focus event to FocusEvent webidl (11.38 KB, patch)
2013-05-26 22:52 PDT, Alfredo Yang (:alfredo)
no flags Details | Diff | Splinter Review
Update focus event to FocusEvent webidl (11.38 KB, patch)
2013-05-27 03:49 PDT, Alfredo Yang (:alfredo)
bugs: review-
Details | Diff | Splinter Review
Test cases for FocusEvent properties. (2.34 KB, patch)
2013-05-27 03:51 PDT, Alfredo Yang (:alfredo)
bugs: review-
Details | Diff | Splinter Review
Update focus event to FocusEvent webidl (15.23 KB, patch)
2013-05-30 20:49 PDT, Alfredo Yang (:alfredo)
no flags Details | Diff | Splinter Review
Test cases for FocusEvent properties. (3.15 KB, patch)
2013-05-30 20:53 PDT, Alfredo Yang (:alfredo)
bugs: review+
Details | Diff | Splinter Review
Update focus event to FocusEvent webidl (15.23 KB, patch)
2013-05-30 22:56 PDT, Alfredo Yang (:alfredo)
bugs: review+
Details | Diff | Splinter Review
Update focus event to FocusEvent webidl (15.32 KB, patch)
2013-06-02 20:12 PDT, Alfredo Yang (:alfredo)
no flags Details | Diff | Splinter Review
Test cases for FocusEvent properties. (3.32 KB, patch)
2013-06-02 20:13 PDT, Alfredo Yang (:alfredo)
ayang: review+
Details | Diff | Splinter Review
Update focus event to FocusEvent webidl (15.32 KB, patch)
2013-06-02 21:16 PDT, Alfredo Yang (:alfredo)
ayang: review+
Details | Diff | Splinter Review
Update focus event to FocusEvent webidl (15.24 KB, patch)
2013-06-03 03:20 PDT, Alfredo Yang (:alfredo)
no flags Details | Diff | Splinter Review
Add interface to test_interface (734 bytes, patch)
2013-06-03 03:21 PDT, Alfredo Yang (:alfredo)
bugs: review+
Details | Diff | Splinter Review
Update focus event to FocusEvent webidl (15.31 KB, patch)
2013-06-05 02:31 PDT, Alfredo Yang (:alfredo)
ayang: review+
Details | Diff | Splinter Review
Test cases for FocusEvent properties. (3.39 KB, patch)
2013-06-05 02:38 PDT, Alfredo Yang (:alfredo)
ayang: review+
Details | Diff | Splinter Review
Add interface to test_interface (810 bytes, patch)
2013-06-05 02:39 PDT, Alfredo Yang (:alfredo)
ayang: review+
Details | Diff | Splinter Review

Description Erik Arvidsson 2013-03-28 09:44:03 PDT
focus, blur, focusin, focusout, DOMFocusIn, DOMFocusOut events should all be instance of the FocusEvent interface.
Comment 1 Alfredo Yang (:alfredo) 2013-05-26 22:52:35 PDT
Created attachment 754340 [details] [diff] [review]
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) 2013-05-27 03:49:06 PDT
Created attachment 754406 [details] [diff] [review]
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) 2013-05-27 03:51:08 PDT
Created attachment 754408 [details] [diff] [review]
Test cases for FocusEvent properties.
Comment 4 Olli Pettay [:smaug] (TPAC) 2013-05-29 12:54:35 PDT
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] (TPAC) 2013-05-29 12:55:53 PDT
Also, make sure we actually use nsFocusEvent when dispatching focus/blur.
Comment 6 Olli Pettay [:smaug] (TPAC) 2013-05-29 12:58:55 PDT
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] (TPAC) 2013-05-29 12:59:18 PDT
Comment on attachment 754408 [details] [diff] [review]
Test cases for FocusEvent properties.

We need test for the event ctor.
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-05-29 19:15:47 PDT
(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) 2013-05-30 20:49:46 PDT
Created attachment 756352 [details] [diff] [review]
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) 2013-05-30 20:53:05 PDT
Created attachment 756355 [details] [diff] [review]
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] 2013-05-30 21:03:13 PDT
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) 2013-05-30 22:56:46 PDT
Created attachment 756390 [details] [diff] [review]
Update focus event to FocusEvent webidl

v2.1 change
1. change function type to nsresult for non webidl function.
Comment 13 Olli Pettay [:smaug] (TPAC) 2013-05-31 05:21:39 PDT
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] (TPAC) 2013-05-31 05:25:08 PDT
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) 2013-06-02 20:12:04 PDT
Created attachment 757199 [details] [diff] [review]
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) 2013-06-02 20:13:39 PDT
Created attachment 757201 [details] [diff] [review]
Test cases for FocusEvent properties.
Comment 17 Alfredo Yang (:alfredo) 2013-06-02 20:20:12 PDT
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) 2013-06-02 20:31:11 PDT
> 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) 2013-06-02 21:16:50 PDT
Created attachment 757213 [details] [diff] [review]
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) 2013-06-02 23:38:40 PDT
https://tbpl.mozilla.org/?tree=Try&rev=d176cb18c574
Comment 21 :Ms2ger (⌚ UTC+1/+2) 2013-06-03 00:30:18 PDT
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] (TPAC) 2013-06-03 01:21:02 PDT
nsIDOMFocusEvent isn't strictly needed, but I'd kind of like to have it.
Comment 23 Masatoshi Kimura [:emk] 2013-06-03 02:02:53 PDT
At least it doesn't have to be scriptable.
Comment 24 Alfredo Yang (:alfredo) 2013-06-03 03:20:18 PDT
Created attachment 757314 [details] [diff] [review]
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) 2013-06-03 03:21:25 PDT
Created attachment 757315 [details] [diff] [review]
Add interface to test_interface

Add FocusEvent to interface test.
Comment 26 Alfredo Yang (:alfredo) 2013-06-05 02:15:42 PDT
try server result
https://tbpl.mozilla.org/?tree=Try&rev=b83b4ab42d8a
Comment 27 Alfredo Yang (:alfredo) 2013-06-05 02:31:03 PDT
Created attachment 758454 [details] [diff] [review]
Update focus event to FocusEvent webidl

Updated checkin comment.
Carry r+ from comment 13.
Comment 28 Alfredo Yang (:alfredo) 2013-06-05 02:38:02 PDT
Created attachment 758457 [details] [diff] [review]
Test cases for FocusEvent properties.

Updated checkin comment.
Carry r+ from comment 14.
Comment 29 Alfredo Yang (:alfredo) 2013-06-05 02:39:42 PDT
Created attachment 758458 [details] [diff] [review]
Add interface to test_interface

Updated checkin comment.
Carry r+ from comment 25.
Comment 32 Kohei Yoshino [:kohei] 2013-08-09 17:52:22 PDT
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 [:kohei] 2013-08-09 18:20:08 PDT
Added version notes to the event references.
Comment 34 Jean-Yves Perrier [:teoli] 2013-08-10 00:02:08 PDT
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

Note You need to log in before you can comment on or make changes to this bug.