486990 - Context Menu can be disabled by stopping propagation (cancelEvent=true or stopPropagation)

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Glen Mailer]

Attachment: Simple test case

The pref for allowing the disabling of the context menu might as well not exist, this method is used by the major javascript utility libraries, as well as in google apps.

Simply put, only preventDefault() is actually prevented from cancelling a contextmenu event, halting the bubble stops it every time.

Note, other bugs on this issue exist in the annals of bugzilla, but they are either old, filed in the wrong place, or lacking a decent testcase - or all of the above.

Comment 1

[Marien Zwart]

Confirming using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3)
Gecko/20090312 Gentoo Firefox/3.1b3 and adjusting platform appropriately.

With js allowed to disable the context menu all three approaches in the test case work (this is correct). With js supposedly not allowed to disable or replace the menu the stopPropagation() and cancelBubble versions still work. In RadialContext (https://addons.mozilla.org/en-US/firefox/addon/5739) I try to make this work by listening during event capture instead of bubble if we do not want the page to interfere. I have not recently looked at the context menu code in mozilla, so not sure how hard it would be to do something similar there.

Comment 2

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

Attachment: patch

This is what I'd like to do.
Neil, any comments? This disables system event group in content XBL, expect
when bound to native anonymous content.

Comment 3

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

Ok, Neil pointed bug 215928.
I need to figure out how to fix that properly.

Comment 4

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

Attachment: better

This cleans up scrollbar event handling.
I need to mochitestify my tests.
Neil(s), comments?

Comment 5

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

Comment on attachment 371303 [details] [diff] [review]
better

This passes chrome/browser-chrome/mochitest.

Comment 6

[[e-mail]]

(In reply to comment #4)
> This cleans up scrollbar event handling.
Well, as I see it, the patch
* Unnecessarily moves the declaration of getPreventDefault
* Unnecessarily moves the group of the preventDefault call of scrollbar.xml
* Unnecessarily moves the scrollbar event blocking into C++
The remaining 11 lines of code actually fix this bug.

Comment 7

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

(In reply to comment #6)
> * Unnecessarily moves the declaration of getPreventDefault
Well, I want that all the script handlers can check if preventDefault is called.
I could do that in some other bug too.

> * Unnecessarily moves the group of the preventDefault call of scrollbar.xml
Right, with the new don't-propagate-event-from-native-anon, the change
isn't needed. Left-over from the previous patch. If the C++ change isn't there,
content would see the event in capture phase and could stop propagation before
the XBL handler is called.

> * Unnecessarily moves the scrollbar event blocking into C++
Really? How else would you prevent content to see the events in capture phase?
Firefox/Seamonkey UI code could do that by calling .stopPropagation in
capture phase, but that would be an ugly hack.

> The remaining 11 lines of code actually fix this bug.
Yeah, the patch does fix bunch of other stuff too, like bug 215928 for
capture phase.

Comment 8

[[e-mail]]

(In reply to comment #7)
> (In reply to comment #6)
> > * Unnecessarily moves the declaration of getPreventDefault
> Well, I want that all the script handlers can check if preventDefault is
> called.
I see now that preventDefault() is in nsIDOMEvent so moving getPreventDefault() to nsIDOMNSUIEvent makes sense.

> > * Unnecessarily moves the group of the preventDefault call of scrollbar.xml
> Right, with the new don't-propagate-event-from-native-anon, the change
> isn't needed. Left-over from the previous patch. If the C++ change isn't there,
> content would see the event in capture phase and could stop propagation before
> the XBL handler is called.
Would it help to move the call to preventDefault() to the system group? That way it couldn't be, well, prevented ;-)

> Yeah, the patch does fix bunch of other stuff too, like bug 215928 for
> capture phase.
Yeah, that's what I'm complaining about :-)

I still think that the non-context popup listener should happen in the system group too.

Comment 9

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

(In reply to comment #8)
> Would it help to move the call to preventDefault() to the system group?
That is what the patch does.

> > Yeah, the patch does fix bunch of other stuff too, like bug 215928 for
> > capture phase.
> Yeah, that's what I'm complaining about :-)
Well, I'd like to keep all that stuff in the same patch.

> I still think that the non-context popup listener should happen in the system
> group too.
Ok, I'll do that.

Comment 10

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

Attachment: add also the other popup listener to system event group + some tests

Neils, what you think about this?

Comment 11

[Neil Deakin]

Comment on attachment 371891 [details] [diff] [review]
add also the other popup listener to system event group + some tests

>+    ok(prevented, "Default handling should not have been prevented.");

do you mean 'should have been'?

>+    is(popupshowingcount, 1, "Should have got 'popupShowing' event!");

'popupshowing' actually ;)

Seems ok to me, but the other Neil will probably have better review comments.

Comment 12

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

(In reply to comment #11)
> do you mean 'should have been'?
Ah, right

Comment 13

[[e-mail]]

Let me see if I've got this right:

The bit that fixes this bug is the change in nsXULElement that adds the listener to the system event group so that stopPropagation() doesn't affect it.

We then have a change to allow the scrollbar binding to handle events from the system event group. Presumably group="system" is normally ignored in content? Should this run off the binding URI rather than the bound document URI?

We then have an extra change to prevent content from being able to see certain events targetted at native anonymous scrollbars. Does this change affect the system phase? If so, the XUL element's listeners wouldn't fire, thus making it unnecessary for the scrollbar binding to prevent the event, thus making it unnecessary to use system event group handlers. I'm worried here that we have code assuming that those events won't bubble from explicit scrollbars either.

We also have a change to make getPreventDefault available on all events.

By the way, in nsXULElement::PreHandleEvent you have an else after return.

Comment 14

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

(In reply to comment #13)
> The bit that fixes this bug is the change in nsXULElement that adds the
> listener to the system event group so that stopPropagation() doesn't affect it.
Right

> Presumably group="system" is normally ignored in content?
Yes after this patch.

> Should this run off the binding URI rather than the bound document URI?
IMO no, because otherwise someone could apply chrome binding to some element and
that way get the chrome behavior. For example bind scrollbar binding to the
root element but then hiding (display:none) all the anonymous elements.
That would prevent context menu.

> We then have an extra change to prevent content from being able to see certain
> events targetted at native anonymous scrollbars. Does this change affect the
> system phase?
It does. System event group uses the same event target chain as default group.

> If so, the XUL element's listeners wouldn't fire, thus making it
> unnecessary for the scrollbar binding to prevent the event, thus making it
> unnecessary to use system event group handlers.
For the contextmenu event I kept the handler (though in system group) so that
contextmenu is never shown for <scrollbar>.

> I'm worried here that we have
> code assuming that those events won't bubble from explicit scrollbars either.
Ok. What if I don't change scrollbar.xml at all.
nsXULElement::PreHandlerEvent should take care of the native anon <scrollbar>
(and I could remove the 'else')

Comment 15

[[e-mail]]

Comment on attachment 371891 [details] [diff] [review]
add also the other popup listener to system event group + some tests

OK, I think that sounds reasonable to me. sr=me without the scrollbar.xml changes.

Comment 16

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

Attachment: + comments

Comment 17

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

http://hg.mozilla.org/mozilla-central/rev/dfd65697f114

Comment 18

[Bill Gianopoulos [:WG9s]]

This appears to have caused bug 490158.

Comment 19

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

I'll backout this.

Comment 20

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

Backed out
http://hg.mozilla.org/mozilla-central/rev/84cd39e69c34

Comment 21

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

Attachment: doesn't break menu's contextmenu

The only change is in popup.xml.
Based on CVS log it was added in Bug 18726, but IMO it is wrong and shouldn't be
needed anymore, I hope.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla%2Fxpfe%2Fglobal%2Fresources%2Fcontent%2FAttic%2FxulBindings.xml&rev=&cvsroot=%2Fcvsroot&mark=805-807#800

Comment 22

[Neil Deakin]

(In reply to comment #21)
> Based on CVS log it was added in Bug 18726, but IMO it is wrong and shouldn't
> be
> needed anymore, I hope.

I would think it would be needed to prevent a context menu defined outside a menu from appearing when context-clicking inside it.

Comment 23

[Neil Deakin]

Comment on attachment 377164 [details] [diff] [review]
doesn't break menu's contextmenu

But I suppose that's the old behaviour too anyway ;)

Comment 24

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

I tried to land this, but it caused /tests/toolkit/content/tests/widgets/test_menulist_keynav.xul | open menulist down selectedItem - got [object XULElement], expected [object XULElement]

Comment 25

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

So the problem is that the test is not a chrome test, but it relies on the
behavior where a XBL listener is in system group.
I wonder how to fix that...

Comment 26

[[e-mail]]

Not just the test but the menulist itself would misbehave if its event listener was not added to the system event group. Or do we not care about content XUL?

Comment 27

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

Yes, those were the questions I was thinking, and also how I could change
menu handling so that content xul would still work ok.

Comment 28

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

Attachment: maybe this way

The only functional change is in nsXBLBinding. This allows system event group
handlers in content if they are defined in a chrome binding.
Did pass unit tests on tryserver.

Note, I did not want to change nsContentUtils::IsChromeDoc callers at all, so
I just renamed the method and made it take nsINode* so that nsXBLBinding
can use it for nsIContent*.

Comment 29

[[e-mail]]

Comment on attachment 383921 [details] [diff] [review]
maybe this way

>+    PRBool isChromeBinding =
>+      nsContentUtils::IsChromeNode(mPrototypeBinding->GetBindingElement());
Is this the same as mPrototypeBinding->IsChrome() ?

Comment 30

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

There is no such thing. nsXBLDocumentInfo has IsChrome and I could use that, if I
add PRBool IsChrome() { return mXBLDocInfoWeak->IsChrome(); } to
nsXBLPrototypeBinding.
But otherwise, does the approach sound ok to you?

Comment 31

[[e-mail]]

Oh? what happened to the version in bug 304027?

https://bugzilla.mozilla.org/attachment.cgi?id=192998&action=diff#mozilla/content/xbl/src/nsXBLPrototypeBinding.h_sec1

[Finding deleted code is really hard :-( ]

Comment 32

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

The method was removed in Bug 350334

Comment 33

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

Attachment: v6

Comment 34

[[e-mail]]

Comment on attachment 383952 [details] [diff] [review]
v6

Thanks, I don't feel so bad asking you to reinstate it now ;-)

Comment 35

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

Landed again http://hg.mozilla.org/mozilla-central/rev/c575412d976a
Crossing fingers.