If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED WONTFIX

Status

()

Toolkit
XUL Widgets
RESOLVED WONTFIX
5 months ago
5 months ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Assignee)

Description

5 months ago
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 hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8859163 - Flags: review?(enndeakin)

Comment 2

5 months ago
mozreview-review
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?
(Assignee)

Comment 3

5 months ago
(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 hidden (mozreview-request)

Comment 5

5 months ago
mozreview-review
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
Attachment #8859163 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 6

5 months ago
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
Flags: needinfo?(enndeakin)
(Assignee)

Comment 7

5 months ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Flags: needinfo?(enndeakin)
Resolution: --- → WONTFIX
(Assignee)

Updated

5 months ago
Attachment #8859163 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.