877669 - XUL Context menu listeners are not removed when removing the context/contextmenu attribute/property

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[:Gijs (he/him)]

STR:

In Firefox, many buttons in the navbar don't have a contextmenu (eg. the downloads button, home button). context-clicking them opens the navbar context menu (allowing you to toggle toolbars and enter customization mode). For one such button which normally functions with that contextmenu:

1. Add a context attribute pointing somewhere (doesn't even need to be a valid menupopup, doesn't matter).
2. Remove the context attribute.

Now, the context menu that worked before, doesn't work anymore.

I suspect this happens because nsXULElement never removes the context menu listener it's added, and therefore the event gets swallowed by the button's listener. Alternatively, the bottom of nsXULPopupListener's handleEvent method should check the return value of LaunchPopup before stopping propagation and calling preventDefault:

http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULPopupListener.cpp#204

Comment 1

[[e-mail]]

Maybe removing the popup listener would work if we only actually had one attribute per listener instead of two as we do now.

Also LaunchPopup doesn't think that the lack of a popup is an error, so maybe it would be easier if it was made responsible for stopping propagation.

Comment 2

[:Gijs (he/him)]

(In reply to [e-mail] from comment #1)
> Maybe removing the popup listener would work if we only actually had one
> attribute per listener instead of two as we do now.
> 
> Also LaunchPopup doesn't think that the lack of a popup is an error, so
> maybe it would be easier if it was made responsible for stopping propagation.

My plan was actually to just change that: lacking a popup when you call a method called "LaunchPopup" should be an error. :S

Comment 3

[:Gijs (he/him)]

Attachment: Patch

Something like this?

Try builds: https://tbpl.mozilla.org/?tree=Try&rev=ff851369e106

Comment 4

[[e-mail]]

Comment on attachment 756219 [details] [diff] [review]
Patch

>   // Open the popup and cancel the default handling of the event.
Looks reasonable, but this comment has become out of date ;-)

Comment 5

[:Gijs (he/him)]

Attachment: Patch v2

This patch checks if the attribute exists rather than treating anything other than a valid ID (or _child with an appropriate popup candidate) as cause for letting the event propagate.

Comment 6

[:Gijs (he/him)]

m-c-based try push this time (ahem): https://tbpl.mozilla.org/?tree=Try&rev=8d53a8f15442

Comment 7

[:Gijs (he/him)]

Attachment: Patch v3

Per feedback on IRC, use the return value of GetAttr instead. This seems to work in my testing. Try build:

https://tbpl.mozilla.org/?tree=Try&rev=e7fa36c334bf

Comment 8

[[e-mail]]

Comment on attachment 756521 [details] [diff] [review]
Patch v3

>   nsIAtom* type = mIsContext ? nsGkAtoms::context : nsGkAtoms::popup;
>+  nsIAtom* fallbackType = mIsContext ? nsGkAtoms::contextmenu : nsGkAtoms::menu;
Nit: fallbackType is only needed in the empty type case.
[I suppose the compiler may be smart enough to rewrite the code to
   mElement->GetAttr(kNameSpaceID_None, mIsContext ? nsGkAtoms::contextmenu : nsGkAtoms::menu, identifier)
]

>+    bool hasFallbackAttr = mElement->GetAttr(kNameSpaceID_None, fallbackType,
>+                                             identifier);
>+    hasPopupAttr = hasPopupAttr || hasFallbackAttr;
[Could have written this as
     hasPopupAttr = mElement->GetAttr(kNameSpaceID_None, fallbackType, identifier) || hasPopupAttr;
 or
     if (mElement->GetAttr(kNameSpaceID_None, fallbackType, identifier)) hasPopupAttr = true;
 or even just use
   if (hasPopupAttr || hasFallbackAttr)
 Probably makes no difference though.]

>+  if (hasPopupAttr) {
>+    aEvent->StopPropagation();
>+    aEvent->PreventDefault();
>+  }
>+  if (identifier.IsEmpty())
>+    return rv;
Nit: empty line after } please. r=me with that fixed.

Comment 9

[:Gijs (he/him)]

(In reply to [e-mail] from comment #8)
> Comment on attachment 756521 [details] [diff] [review]
> Patch v3
> Nit: empty line after } please. r=me with that fixed.


Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/c5a4248bf250

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c5a4248bf250