events not properly consumed

RESOLVED FIXED

Status

Core Graveyard
Embedding: GTK Widget
RESOLVED FIXED
12 years ago
5 years ago

People

(Reporter: Christian Persch (GNOME) (away; not receiving bug mail), Assigned: Christian Persch (GNOME) (away; not receiving bug mail))

Tracking

({fixed1.8})

Trunk
x86
Linux
fixed1.8

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

The DOM focus listener methods don't mark events as consumed if the handler
returns TRUE.
Created attachment 194654 [details] [diff] [review]
fix
Assignee: mpgritti → chpe
Status: NEW → ASSIGNED
Attachment #194654 - Flags: superreview?(roc)
Attachment #194654 - Flags: review?(mpgritti)

Comment 2

12 years ago
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
Attachment #194654 - Flags: review?(mpgritti) → review-
Created attachment 195005 [details] [diff] [review]
updated patch with review comments

I changed the comments, as discussed on irc.
Attachment #194654 - Attachment is obsolete: true
Attachment #195005 - Flags: superreview?(roc)
Attachment #195005 - Flags: review?(mpgritti)
Attachment #194654 - Flags: superreview?(roc)

Updated

12 years ago
Attachment #195005 - Flags: review?(mpgritti) → review+
Remind me why it makes sense to return PR_FALSE for an nsresult?
(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.
Created attachment 196299 [details] [diff] [review]
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.
Attachment #195005 - Attachment is obsolete: true
Attachment #196299 - Flags: superreview+
Attachment #196299 - Flags: review+
Attachment #196299 - Flags: approval1.8b5?
Attachment #195005 - Flags: superreview?(roc)
Comment on attachment 196299 [details] [diff] [review]
fix return value too

Sorry I was confused with other bugs, asking for sr first :)
Attachment #196299 - Flags: superreview?(roc)
Attachment #196299 - Flags: superreview+
Attachment #196299 - Flags: approval1.8b5?
Attachment #196299 - Flags: superreview?(roc) → superreview+
Make sure you get this on the branch!
Comment on attachment 196299 [details] [diff] [review]
fix return value too

gtk embedding only fix
Attachment #196299 - Flags: approval1.8b5?

Updated

12 years ago
Flags: blocking1.8b5+

Comment 10

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

Approved for 1.8b5 per bug meeting
Attachment #196299 - Flags: approval1.8b5? → approval1.8b5+

Comment 11

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

Updated

12 years ago
Flags: blocking1.8b5+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
Component: Embedding: GTK Widget → Embedding: GTK Widget
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.