415440 - nsIPopupBoxObject#setConsumeRollupEvent has no effect on Mac OS X

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Gomita]

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).

Comment 1

[Gomita]

Attachment: testcase

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Is this because the popup's widget is a nsChildView and not a nsCocoaWindow? nsCocoaWindow::CaptureRollupEvents seems to do something, at least.

Comment 3

[Neil Deakin]

No, rollup consuming just isn't currently implemented on the Mac.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(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

[Neil Deakin]

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

Comment 6

[Seth Spitzer]

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

Comment 7

[Neil Deakin]

(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

[Seth Spitzer]

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

[Neil Deakin]

(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

[Seth Spitzer]

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;

Comment 11

[Josh Aas]

Attachment: 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?

Comment 12

[Seth Spitzer]

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

Comment 13

[Seth Spitzer]

> I'll test it and let you know.

Josh, this patch works for me.  thanks again!

Comment 14

[Josh Aas]

Great! I'll clean the patch up and get the necessary reviews.

Comment 15

[Josh Aas]

Attachment: fix v1.1

Cleans up the implementation some.

Comment 16

[Markus Stange [: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?

Comment 17

[Josh Aas]

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 18

[Markus Stange [:mstange]]

Comment on attachment 377167 [details] [diff] [review]
fix v1.1

Makes sense.

Comment 19

[Seth Spitzer]

I've tested the second patch as well, and it is working for me.  thanks again, josh!

Comment 20

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/7d1b9ef54d27

Comment 21

[Eric Shepherd [:sheppy]]

This method wasn't previously documented; now it is, and mentions this fix in a note:

https://developer.mozilla.org/en/XUL/Method/setConsumeRollupEvent