306839 - events not properly consumed

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: GTK Widget, defect)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

The DOM focus listener methods don't mark events as consumed if the handler
returns TRUE.

Comment 1

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: fix

Comment 2

[Marco Pesenti Gritti]

Comment on attachment 194654 [details] [diff] [review]
fix

Patch looks fine. There are some inconsistencies in that file that would be
good to cleanup though, while we are at it.

 73 // All of the event listeners below return NS_OK to indicate that the
 74 // event should not be consumed in the default case.

I think we should kill this now, it's not true for all the events.

-  // return NS_OK to this function to mark this event as not
-  // consumed...

You are removing these for focus event but leaving them around for all other
events.
I'd say we can just keep this but s/NS_OK/FALSE

Comment 3

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: updated patch with review comments

I changed the comments, as discussed on irc.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Remind me why it makes sense to return PR_FALSE for an nsresult?

Comment 5

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #4)
> Remind me why it makes sense to return PR_FALSE for an nsresult?

It does not make sense, we should fix that in a follow-up.

Comment 6

[Christian Persch (GNOME) (away; not receiving bug mail)]

Attachment: fix return value too

Let's just put it in this patch too. I've looked in cvs history and it was
fixed for the other event listeners in the same steps as
PreventDefault/StopPropagation in bug 245529 so it makes sense to include this
here. Assuming r=marco, sr=roc still stands, asking for a1.8b5.

Comment 7

[Christian Persch (GNOME) (away; not receiving bug mail)]

Comment on attachment 196299 [details] [diff] [review]
fix return value too

Sorry I was confused with other bugs, asking for sr first :)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Make sure you get this on the branch!

Comment 9

[Christian Persch (GNOME) (away; not receiving bug mail)]

Comment on attachment 196299 [details] [diff] [review]
fix return value too

gtk embedding only fix

Comment 10

[Mike Schroepfer]

Comment on attachment 196299 [details] [diff] [review]
fix return value too

Approved for 1.8b5 per bug meeting

Comment 11

[Mike Schroepfer]

If you want this in for 1.5b2 you'll need to land before lockdown in 12 days.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

trunk:
Checking in embedding/browser/gtk/src/EmbedEventListener.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedEventListener.cpp,v  <-- 
EmbedEventListener.cpp
new revision: 1.8; previous revision: 1.7
done

MOZILLA_1_8_BRANCH:
Checking in embedding/browser/gtk/src/EmbedEventListener.cpp;
/cvsroot/mozilla/embedding/browser/gtk/src/EmbedEventListener.cpp,v  <-- 
EmbedEventListener.cpp
new revision: 1.7.28.1; previous revision: 1.7
done