Add handleDefault(nsIDOMEvent) method to nsIXTFElement

RESOLVED FIXED

Status

SeaMonkey
General
--
enhancement
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: smaug, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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:
Severity: normal → enhancement
Created attachment 161596 [details] [diff] [review]
v.1

Here is the patch.
Comment on attachment 161596 [details] [diff] [review]
v.1

Alex, does this look ok?
Attachment #161596 - Flags: review?(alex)
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-
Created attachment 161787 [details] [diff] [review]
v.2. Creates nsIDOMEvent if needed
Attachment #161596 - Attachment is obsolete: true
Attachment #161596 - Flags: superreview?(jst)
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

13 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?
Created attachment 161850 [details] [diff] [review]
v2 + comments
Attachment #161787 - Attachment is obsolete: true
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)
Attachment #161787 - Flags: superreview?(jst)
Comment on attachment 161850 [details] [diff] [review]
v2 + comments

sr=jst
Attachment #161850 - Flags: superreview?(jst) → superreview+
Status: UNCONFIRMED → RESOLVED
Last Resolved: 13 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.