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

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

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

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.

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

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

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

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.

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

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

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

Comment on attachment 757201 [details] [diff] [review] Test cases for FocusEvent properties. Add relatedTarget test to $("content"). According to comment 14, carry r+.

> 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

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

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.

Add FocusEvent to interface test.

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

Updated checkin comment. Carry r+ from comment 13.

Updated checkin comment. Carry r+ from comment 14.

Updated checkin comment. Carry r+ from comment 25.

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

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

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

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

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