Add handleDefault(nsIDOMEvent) method to nsIXTFElement

RESOLVED FIXED

Status

--
enhancement
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: smaug, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

15 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a4) Gecko/20040927
Build Identifier: 

XTF elements should be able to be default handlers for the events.
Patch coming.

Reproducible: Always
Steps to Reproduce:
(Reporter)

Updated

15 years ago
Severity: normal → enhancement
(Reporter)

Comment 1

15 years ago
Created attachment 161596 [details] [diff] [review]
v.1

Here is the patch.
(Reporter)

Comment 2

15 years ago
Comment on attachment 161596 [details] [diff] [review]
v.1

Alex, does this look ok?
Attachment #161596 - Flags: review?(alex)
(Reporter)

Updated

15 years ago
Attachment #161596 - Flags: superreview?(jst)
Attachment #161596 - Flags: review?(bryner)
Attachment #161596 - Flags: review?(alex)
Comment on attachment 161596 [details] [diff] [review]
v.1

>--- content/xtf/src/nsXTFElementWrapper.cpp	7 Oct 2004 20:59:50 -0000	1.2
>+++ content/xtf/src/nsXTFElementWrapper.cpp	9 Oct 2004 20:06:09 -0000
>@@ -553,8 +555,33 @@
> PRBool
> nsXTFElementWrapper::HandledByInner(nsIAtom *attr) const
> {
>   PRBool retval = PR_FALSE;
>   if (mAttributeHandler)
>     mAttributeHandler->HandlesAttribute(attr, &retval);
>   return retval;
> }
>+
>+nsresult
>+nsXTFElementWrapper::HandleDOMEvent(nsPresContext* aPresContext,
>+                                    nsEvent* aEvent,
>+                                    nsIDOMEvent** aDOMEvent,
>+                                    PRUint32 aFlags,
>+                                    nsEventStatus* aEventStatus)
>+{
>+  nsresult rv = nsXTFElementWrapperBase::HandleDOMEvent(aPresContext, aEvent,
>+                                                        aDOMEvent, aFlags,
>+                                                        aEventStatus);
>+

Let's just return here if NS_FAILED(rv).

>+  if ((mNotificationMask & nsIXTFElement::NOTIFY_HANDLE_DEFAULT) &&
>+      NS_SUCCEEDED(rv) && aDOMEvent && *aDOMEvent && (nsEventStatus_eIgnore == *aEventStatus) &&

Null-checking *aDOMEvent here is wrong.  The problem is that the DOM event is
lazily created, so if it was never given to a DOM event handler, it won't be
there.	If *aDOMEvent is null, you need to force creation of the DOMEvent now
using nsIEventListenerManager::CreateEvent (see
nsGenericElement::HandleDOMEvent for an example).

{
>+    PRBool defaultHandled = PR_FALSE;
>+    nsIXTFElement * xtfElement = GetXTFElement();
>+    if (xtfElement)
>+      rv = xtfElement->HandleDefault(*aDOMEvent, &defaultHandled);
>+    if (defaultHandled)
>+      *aEventStatus = nsEventStatus_eConsumeDoDefault;

This should be eConsumeNoDefault, I think.  NoDefault means we handled the
event, and is propagated all the way out to the OS to tell it that we handled
the event.  Ignore means we didn't handle the event, and the OS default
handling should happen.  I've never really understood DoDefault, I think it
means we did something with the event but want the OS handling to happen, which
I don't think is ever really the case.
Attachment #161596 - Flags: review?(bryner) → review-
(Reporter)

Comment 4

15 years ago
Created attachment 161787 [details] [diff] [review]
v.2. Creates nsIDOMEvent if needed
Attachment #161596 - Attachment is obsolete: true
(Reporter)

Updated

15 years ago
Attachment #161596 - Flags: superreview?(jst)
(Reporter)

Updated

15 years ago
Attachment #161787 - Flags: superreview?(jst)
Attachment #161787 - Flags: review?(bryner)
Comment on attachment 161787 [details] [diff] [review]
v.2. Creates nsIDOMEvent if needed

>--- content/xtf/src/nsXTFElementWrapper.cpp	7 Oct 2004 20:59:50 -0000	1.2
>+++ content/xtf/src/nsXTFElementWrapper.cpp	11 Oct 2004 20:15:52 -0000
>@@ -553,8 +556,52 @@
> PRBool
> nsXTFElementWrapper::HandledByInner(nsIAtom *attr) const
> {
>   PRBool retval = PR_FALSE;
>   if (mAttributeHandler)
>     mAttributeHandler->HandlesAttribute(attr, &retval);
>   return retval;
> }
>+
>+nsresult
>+nsXTFElementWrapper::HandleDOMEvent(nsPresContext* aPresContext,
>+                                    nsEvent* aEvent,
>+                                    nsIDOMEvent** aDOMEvent,
>+                                    PRUint32 aFlags,
>+                                    nsEventStatus* aEventStatus)
>+{
>+  nsresult rv = nsXTFElementWrapperBase::HandleDOMEvent(aPresContext, aEvent,
>+                                                        aDOMEvent, aFlags,
>+                                                        aEventStatus);
>+
>+  if (NS_FAILED(rv) || !(mNotificationMask & nsIXTFElement::NOTIFY_HANDLE_DEFAULT) ||
>+      (nsEventStatus_eIgnore != *aEventStatus) || (aFlags & NS_EVENT_FLAG_SYSTEM_EVENT) ||
>+      (aFlags & NS_EVENT_FLAG_CAPTURE))

Ok, just one minor optimization:

replace

(aFlags & NS_EVENT_FLAG_SYSTEM_EVENT) || (aEvent & NS_EVENT_FLAG_CAPTURE)

with

(aFlags & (NS_EVENT_FLAG_SYSTEM_EVENT | NS_EVENT_FLAG_CAPTURE))

Also try to keep these lines under 80 columns, I think you just need to put a
newline after each ||

r=me with that fixed
Attachment #161787 - Flags: review?(bryner) → review+

Comment 6

15 years ago
Could you also please add a brief comment in nsIXTFElement.idl, explaining what
the return value does & at what point in the event processing this method will
be called?
(Reporter)

Comment 7

15 years ago
Created attachment 161850 [details] [diff] [review]
v2 + comments
Attachment #161787 - Attachment is obsolete: true
(Reporter)

Comment 8

15 years ago
Comment on attachment 161850 [details] [diff] [review]
v2 + comments

I hope the comment is fine.
I if this ok, could someone check this in?
Attachment #161850 - Flags: superreview?(jst)
Attachment #161850 - Flags: review?(bryner)
(Reporter)

Updated

15 years ago
Attachment #161787 - Flags: superreview?(jst)
Comment on attachment 161850 [details] [diff] [review]
v2 + comments

sr=jst
Attachment #161850 - Flags: superreview?(jst) → superreview+
(Reporter)

Updated

15 years ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Comment on attachment 161850 [details] [diff] [review]
v2 + comments

removing obsolete review request (the patch looks fine)
Attachment #161850 - Flags: review?(bryner)
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.