1357405 - Popup trigger nodes should use originaltarget rather than target from originating mouse events

Status: RESOLVED WONTFIX

(Toolkit :: UI Widgets, defect)

Description

[:Gijs (he/him)]

STR:

1. have a toolbarbutton element inside a <panelview> in a <panelmultiview> and open a context menu on it:

Expected:
I can work out what toolbarbutton the context menu is fired on

Actual:
I can't. The popup's triggerNode (and therefore document's popupNode) is the <panelmultiview>, and the rangeParent is the parent of the toolbarbutton. There's literally nothing on the event object to work this out.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1357405 - expose the original target of popups rather than only the target,

Review commit: https://reviewboard.mozilla.org/r/131202/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131202/

Comment 2

[Neil Deakin]

Comment on attachment 8859163 [details]
Bug 1357405 - expose the original target of popups rather than only the target,

https://reviewboard.mozilla.org/r/131202/#review133760

::: layout/xul/nsXULPopupManager.cpp:618
(Diff revision 1)
>    if (aTriggerContent) {
>      *aTriggerContent = nullptr;
>      if (aEvent) {
>        // get the trigger content from the event
> -      nsCOMPtr<nsIContent> target = do_QueryInterface(
> -        aEvent->InternalDOMEvent()->GetTarget());
> +      nsCOMPtr<nsIDOMEventTarget> target;
> +      aEvent->InternalDOMEvent()->GetOriginalTarget(getter_AddRefs(target));

You can just use the no-argument GetOriginalTarget, no?

Comment 3

[:Gijs (he/him)]

(In reply to Neil Deakin from comment #2)
> Comment on attachment 8859163 [details]
> Bug 1357405 - expose the original target of popups rather than only the
> target,
> 
> https://reviewboard.mozilla.org/r/131202/#review133760
> 
> ::: layout/xul/nsXULPopupManager.cpp:618
> (Diff revision 1)
> >    if (aTriggerContent) {
> >      *aTriggerContent = nullptr;
> >      if (aEvent) {
> >        // get the trigger content from the event
> > -      nsCOMPtr<nsIContent> target = do_QueryInterface(
> > -        aEvent->InternalDOMEvent()->GetTarget());
> > +      nsCOMPtr<nsIDOMEventTarget> target;
> > +      aEvent->InternalDOMEvent()->GetOriginalTarget(getter_AddRefs(target));
> 
> You can just use the no-argument GetOriginalTarget, no?

Yes, somehow I missed that. Updated patch in a second.

Comment 4

[:Gijs (he/him)]

Comment on attachment 8859163 [details]
Bug 1357405 - expose the original target of popups rather than only the target,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131202/diff/1-2/

Comment 5

[Neil Deakin]

Comment on attachment 8859163 [details]
Bug 1357405 - expose the original target of popups rather than only the target,

https://reviewboard.mozilla.org/r/131202/#review133770

Comment 6

[:Gijs (he/him)]

The trypush is... pretty orange. Neil, is there a better way to fix this, beyond just updating all the consumers (and hoping there aren't too many broken add-ons and/or that those'll update themselves) ?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6ad4e6d4c55&selectedJob=92357900

Comment 7

[:Gijs (he/him)]

I found a workaround (setting the context attribute for the popup inside the XBL binding seems to help, though don't ask me why) that we also used for the main panel menu, so that should be acceptable. I expect given the test breakage in the trypush that fixing this is not feasible, at least not before 57, so going to unassign and mark wontfix for now. We can re-evaluate if there are good reasons to do so.