Closed Bug 486990 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: XUL, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: glenjamin+bmo, Assigned: smaug)

References

Details

Attachments

(3 files, 5 obsolete files)

Attached file 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.
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.
OS: Windows Vista → All
Hardware: x86 → All
Component: DOM: Events → XUL
QA Contact: events → xptoolkit.widgets
Attached patch patch (obsolete) — Splinter Review
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.
Ok, Neil pointed bug 215928.
I need to figure out how to fix that properly.
Attached patch better (obsolete) — Splinter Review
This cleans up scrollbar event handling.
I need to mochitestify my tests.
Neil(s), comments?
Assignee: nobody → Olli.Pettay
Attachment #371241 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Comment on attachment 371303 [details] [diff] [review]
better

This passes chrome/browser-chrome/mochitest.
(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.
(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.
(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.
(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.
Neils, what you think about this?
Attachment #371303 - Attachment is obsolete: true
Attachment #371891 - Flags: superreview?(neil)
Attachment #371891 - Flags: review?(enndeakin)
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.
Attachment #371891 - Flags: review?(enndeakin) → review+
(In reply to comment #11)
> do you mean 'should have been'?
Ah, right
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.
(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 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.
Attachment #371891 - Flags: superreview?(neil) → superreview+
Attached patch + comments (obsolete) — Splinter Review
Attachment #371891 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/dfd65697f114
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 490158
This appears to have caused bug 490158.
I'll backout this.
Backed out
http://hg.mozilla.org/mozilla-central/rev/84cd39e69c34
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch doesn't break menu's contextmenu (obsolete) — Splinter Review
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
Attachment #377164 - Flags: superreview?(neil)
Attachment #377164 - Flags: review?(enndeakin)
Attachment #377164 - Flags: superreview?(neil) → superreview+
(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 on attachment 377164 [details] [diff] [review]
doesn't break menu's contextmenu

But I suppose that's the old behaviour too anyway ;)
Attachment #377164 - Flags: review?(enndeakin) → review+
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]
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...
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?
Yes, those were the questions I was thinking, and also how I could change
menu handling so that content xul would still work ok.
Attached patch maybe this waySplinter Review
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*.
Attachment #373627 - Attachment is obsolete: true
Attachment #377164 - Attachment is obsolete: true
Attachment #383921 - Flags: superreview?(neil)
Attachment #383921 - Flags: review?(enndeakin)
Comment on attachment 383921 [details] [diff] [review]
maybe this way

>+    PRBool isChromeBinding =
>+      nsContentUtils::IsChromeNode(mPrototypeBinding->GetBindingElement());
Is this the same as mPrototypeBinding->IsChrome() ?
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?
The method was removed in Bug 350334
Attached patch v6Splinter Review
Attachment #383952 - Flags: superreview?(neil)
Attachment #383952 - Flags: review?(enndeakin)
Attachment #383952 - Flags: superreview?(neil) → superreview+
Comment on attachment 383952 [details] [diff] [review]
v6

Thanks, I don't feel so bad asking you to reinstate it now ;-)
Attachment #383921 - Flags: superreview?(neil)
Attachment #383921 - Flags: review?(enndeakin)
Attachment #383952 - Flags: review?(enndeakin) → review+
Landed again http://hg.mozilla.org/mozilla-central/rev/c575412d976a
Crossing fingers.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Duplicate of this bug: 407658
Duplicate of this bug: 407658
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Flags: in-testsuite? → in-testsuite+
Duplicate of this bug: 323047
Duplicate of this bug: 387461
Duplicate of this bug: 467484
You need to log in before you can comment on or make changes to this bug.