The default bug view has changed. See this FAQ.

FocusEvent interface is missing

RESOLVED FIXED in mozilla24

Status

()

Core
DOM: Events
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Erik Arvidsson, Assigned: alfredo)

Tracking

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

unspecified
mozilla24
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 11 obsolete attachments)

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

Description

4 years ago
focus, blur, focusin, focusout, DOMFocusIn, DOMFocusOut events should all be instance of the FocusEvent interface.
(Reporter)

Updated

4 years ago
Component: DOM → DOM: Events
(Assignee)

Updated

4 years ago
Blocks: 687787
(Assignee)

Comment 1

4 years ago
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
Attachment #754340 - Flags: review?(bugs)
(Assignee)

Updated

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

Updated

4 years ago
Attachment #754340 - Attachment is obsolete: true
(Assignee)

Comment 2

4 years ago
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
Attachment #754406 - Flags: review?(bugs)
(Assignee)

Comment 3

4 years ago
Created attachment 754408 [details] [diff] [review]
Test cases for FocusEvent properties.
(Assignee)

Updated

4 years ago
Attachment #754408 - Attachment is patch: true
(Assignee)

Updated

4 years ago
Attachment #754408 - Flags: review?(bugs)

Comment 4

4 years ago
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-

Comment 5

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

Comment 6

4 years ago
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

4 years ago
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

4 years ago
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
Attachment #754406 - Attachment is obsolete: true
Attachment #756352 - Flags: review?(bugs)
(Assignee)

Comment 10

4 years ago
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
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

4 years ago
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.
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

4 years ago
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.
Assignee: nobody → ayang
Attachment #756390 - Attachment is obsolete: true
Flags: needinfo?(schien)
(Assignee)

Comment 16

4 years ago
Created attachment 757201 [details] [diff] [review]
Test cases for FocusEvent properties.
Attachment #756355 - Attachment is obsolete: true
(Assignee)

Comment 17

4 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

4 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

4 years ago
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.
Attachment #757199 - Attachment is obsolete: true
Attachment #757213 - Flags: review+
https://tbpl.mozilla.org/?tree=Try&rev=d176cb18c574
Flags: needinfo?(schien)
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

4 years ago
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.
Attachment #757213 - Attachment is obsolete: true
(Assignee)

Comment 25

4 years ago
Created attachment 757315 [details] [diff] [review]
Add interface to test_interface

Add FocusEvent to interface test.
Attachment #757315 - Flags: review?(bugs)

Updated

4 years ago
Attachment #757315 - Flags: review?(bugs) → review+
(Assignee)

Comment 26

4 years ago
try server result
https://tbpl.mozilla.org/?tree=Try&rev=b83b4ab42d8a
(Assignee)

Comment 27

4 years ago
Created attachment 758454 [details] [diff] [review]
Update focus event to FocusEvent webidl

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

Comment 28

4 years ago
Created attachment 758457 [details] [diff] [review]
Test cases for FocusEvent properties.

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

Comment 29

4 years ago
Created attachment 758458 [details] [diff] [review]
Add interface to test_interface

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

Updated

4 years ago
Keywords: checkin-needed
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
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/81871f331faa
https://hg.mozilla.org/mozilla-central/rev/d65a30f3db31
https://hg.mozilla.org/mozilla-central/rev/542e1529a08b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Keywords: dev-doc-needed
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
Keywords: dev-doc-needed → dev-doc-complete, site-compat
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
Keywords: dev-doc-complete → dev-doc-needed
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
Keywords: dev-doc-needed → dev-doc-complete
Blocks: 962251
You need to log in before you can comment on or make changes to this bug.