263649 - Add handleDefault(nsIDOMEvent) method to nsIXTFElement

Status: RESOLVED FIXED

(SeaMonkey :: General, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

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:

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v.1

Here is the patch.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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

Alex, does this look ok?

Comment 3

[Brian Ryner (not reading)]

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.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v.2. Creates nsIDOMEvent if needed

Comment 5

[Brian Ryner (not reading)]

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

Comment 6

[Alex Fritze]

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?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2 + comments

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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

I hope the comment is fine.
I if this ok, could someone check this in?

Comment 9

[Johnny Stenback (:jst)]

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

sr=jst

Comment 10

[Brian Ryner (not reading)]

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

removing obsolete review request (the patch looks fine)