Closed Bug 855741 Opened 8 years ago Closed 7 years ago

FocusEvent interface is missing

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: erik, Assigned: ayang)

References

()

Details

(Keywords: dev-doc-complete, site-compat)

Attachments

(3 files, 11 obsolete files)

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
focus, blur, focusin, focusout, DOMFocusIn, DOMFocusOut events should all be instance of the FocusEvent interface.
Component: DOM → DOM: Events
Blocks: 687787
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)
Attachment #754340 - Flags: review?(bugs)
Attachment #754340 - Attachment is obsolete: true
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)
Attachment #754408 - Attachment is patch: true
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.
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)
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.
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+
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)
Attachment #756355 - Attachment is obsolete: true
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+
> 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.
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.
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
Attached patch Add interface to test_interface (obsolete) — Splinter Review
Add FocusEvent to interface test.
Attachment #757315 - Flags: review?(bugs)
Attachment #757315 - Flags: review?(bugs) → review+
Updated checkin comment.
Carry r+ from comment 13.
Attachment #757314 - Attachment is obsolete: true
Attachment #758454 - Flags: review+
Updated checkin comment.
Carry r+ from comment 14.
Attachment #757201 - Attachment is obsolete: true
Attachment #758457 - Flags: review+
Updated checkin comment.
Carry r+ from comment 25.
Attachment #757315 - Attachment is obsolete: true
Attachment #758458 - Flags: review+
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.