nsIPopupBoxObject#setConsumeRollupEvent has no effect on Mac OS X

RESOLVED FIXED

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: Gomita, Assigned: Josh Aas)

Tracking

({dev-doc-complete})

Trunk
All
Mac OS X
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

1.09 KB, application/vnd.mozilla.xul+xml
Details
6.02 KB, patch
mstange
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; ja; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: 

In Firefox 3, nsIPopupBoxObject has a new setConsumeRollupEvent method which
enables to decide whether outside clicks of popup should be consumed or not.
However, the method has no effect on Mac OS X.

Reproducible: Always

Steps to Reproduce:
1.Open the subsequent testcase.
2.Click [ROLLUP_NO_CONSUME] button to open popup.
3.Click [TEST] button during popup display.

Actual Results:  
Platform-dependent.
- Windows: Popup closes and alert dialog opens.
- Linux  : Popup closes and alert dialog opens.
- MacOSX : Popup closes but alert dialog does not open.


Expected Results:  
Popup closes and alert dialog opens on all platforms.
Clicks outside of a popup should NOT be eaten when we use
nsIPopupBoxObject#setConsumeRollupEvent(Ci.nsIPopupBoxObject.ROLLUP_NO_CONSUME).
(Reporter)

Comment 1

11 years ago
Created attachment 301124 [details]
testcase
(Reporter)

Updated

11 years ago
Blocks: 279703
Version: unspecified → Trunk
Is this because the popup's widget is a nsChildView and not a nsCocoaWindow? nsCocoaWindow::CaptureRollupEvents seems to do something, at least.
Assignee: nobody → joshmoz
Component: XP Toolkit/Widgets: Menus → Widget: Cocoa
QA Contact: xptoolkit.menus → cocoa

Comment 3

11 years ago
No, rollup consuming just isn't currently implemented on the Mac.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #3)
> No, rollup consuming just isn't currently implemented on the Mac.

What does "rollup consuming isn't currently implemented on the Mac" mean, if not what I said?

Comment 5

10 years ago
The Mac implementation of CaptureRollupEvents ignores the consume rollups flag, and always consumes the event.

(Assignee)

Updated

9 years ago
Assignee: joshmoz → nobody

Comment 6

9 years ago
Neil or Josh:  any recommendation on how to go about fixing this?

Comment 7

9 years ago
(In reply to comment #6)
> Neil or Josh:  any recommendation on how to go about fixing this?

It's fairly easy. You just need to modify nsCocoaWindow::CaptureRollupEvents to not ignore the aConsumeRollupEvent argument. The implementations on Windows and Linux should show how this is done. They cache the value and check it when rolling up popups.

Comment 8

9 years ago
Neil, thanks for the tip on where to start!

for Linux, in mozilla/widget/src/gtk2/nsWindow.cpp, in nsWindow::OnButtonPressEvent(), I see where we checked the cached value gConsumeRollupEvent after rollup:

    PRBool rolledUp = check_for_rollup(aEvent->window, aEvent->x_root,
                                       aEvent->y_root, PR_FALSE);
    if (gConsumeRollupEvent && rolledUp)
            return;

For mac, in nsCocoaWindow.mm, is the event getting consumed in sendEvent()?  It looks like the NSLeftMouseDown gets converted into a NSLeftMouseUp for the child view.

If we're not consuming events, should I also generate a second event, a NSLeftMouseDown, after we call RollUpPopups()?

Comment 9

9 years ago
(In reply to comment #8)
> For mac, in nsCocoaWindow.mm, is the event getting consumed in sendEvent()?  It
> looks like the NSLeftMouseDown gets converted into a NSLeftMouseUp for the
> child view.
> 

Mac is probably a bit trickier as nsCocoaWindow implements CaptureRollupEvents but nsChildView also does the relevant rolling up you want.

See the calls to nsChildView maybeRollup. Likely the same check is needed in those places as the gtk code you show above.

Comment 10

9 years ago
Thanks for the follow up advice, Neil.  

That makes sense to me:  we might perform the rollup in maybeRollup() by calling gRollupListener->Rollup(), but if we are not consuming the event, we could return PR_FALSE, and then in mouseDown() we would not bail out here:

  if ([self maybeRollup:theEvent])
    return;
(Assignee)

Updated

9 years ago
Hardware: PowerPC → All
(Assignee)

Comment 11

9 years ago
Created attachment 376936 [details] [diff] [review]
fix v1.0

This works for me, but this code is tricky so I'm going to test some more. Seth - does this work for you?
Assignee: nobody → joshmoz

Comment 12

9 years ago
Josh, thanks for whipping up a patch!  I'll test it and let you know.

Comment 13

9 years ago
> I'll test it and let you know.

Josh, this patch works for me.  thanks again!
(Assignee)

Comment 14

9 years ago
Great! I'll clean the patch up and get the necessary reviews.
(Assignee)

Comment 15

9 years ago
Created attachment 377167 [details] [diff] [review]
fix v1.1

Cleans up the implementation some.
Attachment #376936 - Attachment is obsolete: true
Attachment #377167 - Flags: review?(smichaud)
(Assignee)

Updated

9 years ago
Attachment #377167 - Flags: review?(smichaud) → review?(mstange)
>--- a/widget/src/cocoa/nsCocoaWindow.mm	
...
> PRBool gCocoaWindowMethodsSwizzled = PR_FALSE;
> 
>+PRBool gConsumeRollupEvent;

In the previous version of the patch you initialized this field. Why did you change that?
(Assignee)

Comment 17

9 years ago
It doesn't matter what it is initialized to, I didn't want to give the impression that it mattered by picking a random value. It isn't initialized in gtk2 either.
Comment on attachment 377167 [details] [diff] [review]
fix v1.1

Makes sense.
Attachment #377167 - Flags: review?(mstange) → review+
(Assignee)

Updated

9 years ago
Attachment #377167 - Flags: superreview?(roc)
Attachment #377167 - Flags: superreview?(roc) → superreview+

Comment 19

9 years ago
I've tested the second patch as well, and it is working for me.  thanks again, josh!
(Assignee)

Comment 20

9 years ago
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/7d1b9ef54d27
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
This method wasn't previously documented; now it is, and mentions this fix in a note:

https://developer.mozilla.org/en/XUL/Method/setConsumeRollupEvent
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.