Closed Bug 287357 Opened 19 years ago Closed 9 years ago

showPopup() could display a popup at the wrong position

Categories

(Core :: XUL, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: antonus, Unassigned)

References

(Depends on 1 open bug, )

Details

Attachments

(1 file)

1.15 KB, application/vnd.mozilla.xul+xml
Details
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.6) Gecko/20050223 Firefox/1.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.6) Gecko/20050223 Firefox/1.0.1

I want to show a popup at the mouse pointer position with the function showPopup.
the position is calculated with
(event.clientX+document.documentElement.boxObject.screenX,
event.clientY+document.documentElement.boxObject.screenY).

After the bug as occured, it act like if event.clientX =
event.clientX+document.documentElement.boxObject.screenX, the same for clientY
and screenY.

so, the real (buggy) position becomes
(event.clientX+document.documentElement.boxObject.screenX+document.documentElement.boxObject.screenX,
event.clientY+document.documentElement.boxObject.screenY+document.documentElement.boxObject.screenY)

Reproducible: Always

Steps to Reproduce:
1. double click : the popup is correct
2. right click, the context popup is correct
3. double click again : the popup is incorrect

Actual Results:  
the popup is not at the mouse pointer

Expected Results:  
the popup is at the mouse pointer
Actually, the first one is in the middle of the screen for me, too... But did
you bring this up because a page isn't working because of this?  Or are you
trying to create a page?  Either way, it's possible that Firefox doesn't support
it... Try the page in IE and let us know.
Alan, this bug is about popups in XUL.  IE is not relevant here.

Neil, roc, any idea what's going on here?

Antoine, is this an issue with the coords the event reports, or with the
showPopup call itself?
(In reply to comment #2)
> Antoine, is this an issue with the coords the event reports, or with the
> showPopup call itself?

I added the calculated coords in the test page when ondblclick occurs.
In both case the the coords are the same, so it is the showPopup that is broken.
The first time, the placement is relative to the screen, the second time the
placement is/seems relative to the document.

OK, this is ridiculous.

First problem:  the popup box object api doesn't document whether the coords are
in the client coord space or in screen coord space.  The code assumes the former.

Second problem:  The code tries to handle coordinate transformations caused by
subdocuments via nsMenuPopupFrame::AdjustClientXYForNestedDocuments.  This
method doesn't actually take a node or anything (which it's caller,
nsMenuPopupFrame::SyncViewWithFrame, doesn't have anyway).  Instead, it tries to
determine the "target" document as follows:

  // Find the widget associated with the target's document.
  // For tooltips, we check the document's tooltipNode (which is set by
  // nsXULTooltipListener).  For regular popups, use popupNode (set by
  // nsXULPopupListener).

I'm not sure why this code thinks that getting the document of the node set as
the popupNode on the document that's the "popup" document should give the
"target" document... but there it is.  Apparently, calling showPopup() on a
<popup> doesn't set the popupNode.  Opening the context menu does, and doesn't
unset it.  So the second time we showPopup() we get the context menu's popup as
the popupNode and end up with coords relative to the document instead of
relative to screen (because we have a document to be relative to, all of a sudden).

Neil, any idea what coords we _should_ be working in here, to start with?  Given
that, what's the deal with popupNode?

And in any case, is all this code really needed?  ;)
Status: UNCONFIRMED → NEW
Component: Layout → XP Toolkit/Widgets: XUL
Ever confirmed: true
QA Contact: layout
(In reply to comment #4)
>Neil, any idea what coords we _should_ be working in here, to start with?
Should? It appears to want event coordinates. At least, that's most convenient
to the popup and context menu listeners.

>Given that, what's the deal with popupNode?
tooltipNode is set by a XUL element's tooltip listener whenever it decides it
needs to show a tooltip. popupNode is supposed to be set by the context menu
code, although for some reason the tooltip code manages to set it as well, for
reasons that I haven't managed to determine, but it caused bug 102551.
> It appears to want event coordinates.

OK.  We can go with that.  But event coordinates in which coordinate system, in
the case of subframes, which is the case this broken code seems to be trying to
address?

> popupNode is supposed to be set by the context menu code

The problem is that popupNode is relied on for all popups, not just the context
menu code.  And the context menu code never _un_sets it.  So once you put up a
context menu, the popupNode is always the context menu's popupNode, no matter
what popup you're actually opening.

I suppose we could change popup.xml to set the popupNode.  Thing is, what would
that break?

And finally, what exact node is the popupNode supposed to be?  The DOM node of
the popup?  Or the DOM node the popup is opening for?

Or put another way, I'd really like to move away from the current
document-adjusting code...  I can't figure out why that code is working at all,
or what objects it's operating on and why.
(In reply to comment #6)
>>It appears to want event coordinates.
>But event coordinates in which coordinate system
In the system used by an event that bubbled up to showPopup's srcContent.
OK. So what we really want to do is to convert from the "document" coordinates
of srcContent to the "document" coordinates of whatever document the mContent of
the popup frame lives in, right?
The popup or tooltip should be in the same document as the srcContent because it
is located by id (or is document body anonymous content for tooltiptext).
Ok... So is the popupNode allowed to be in a different document, then?
Ah right, now I understand your question.

The popupNode or tooltipNode is the target (which may be in a subdocument) of
the event that bubbled up to the srcContent thus triggering the popup/tooltip.
OK.  So in other words, right now it's the responsibility of every single caller
to nsIPopupBoxObject::ShowPopup to set the popupNode correctly before making the
call?

Perhaps showPopup should take the node to set as the popupNode as the argument?

Alternately, perhaps the code in popupframe should only mess with coordinates if
it has both nodes it needs?  That way not setting popupNOde would still be ok as
long as you're all within a single document.  Though that would still break if
there are _any_ subdocuments around, since context menus never clear the
popupNode they set.  Shouldn't the popup coming down, at least, clear the popupNode?
I don't think I understand your question.

The popup/tooltip listeners want to show their popup near the mouse cursor.
The information they have is:
* The popup to be shown
* The mouse event that triggered the popup, which includes
* Client X and Y co-ordinates
* Target element
* Current target (which may be in a different document).

But I don't know which element the X and Y coordinates are relative to.
> I don't think I understand your question.

I think that's because we're talking about different things...

> The popup/tooltip listeners want to show their popup near the mouse cursor.

Sure.  That's fine.  But I'm talking about popups shown by explicitly calling
the showPopup() method on an object implementing the nsIPopupBoxObject
interface.  that's what this bug is filed on, after all.

The way the code is currently written, it seems that properly opening a popup
requires 3 DOM nodes:

1)  The popup's DOM node (a XUL <popup> element).
2)  The "source node" (this is the thing triggering the popup?)
3)  The target node (the node that's right-clicked for a context menu popup).

The showPopup() method takes _two_ nodes as arguments.  It takes a source node
as the first argument and the popup's DOM node as the second argument (which
seems to just duplicate its mContent, but that's not the issue).  Therefore,
this method can't possibly properly open a popup on its own.  So either all
callers need to set the document's popupNode to the target node before calling
showPopup(), or showPopup() needs another argument, or something else needs to
happen.
So we need three nodes because the event's coordinates are relative to the
target node's document?
We need three nodes because the popup back end assumes that the passed-in
coordinates are relative to the document of the popupNode of the XUL document
the popup is in.

Nowhere is it documented that any of this has anything to do with events.

But yes, for the special case of context menu popups that seems to be the reason.
(In reply to comment #16)
>We need three nodes because the popup back end assumes that the passed-in
>coordinates are relative to the document of the popupNode of the XUL document
>the popup is in.
Is that because that's how our event coordinates behave?

> Nowhere is it documented that any of this has anything to do with events.
Documented? You're being over-optimistic :-P

>But yes, for the special case of context menu popups that seems to be the reason.
Don't forget tooltips.
OK.  I see no quick fix here, and further after some thought I've decided that
our client* thing for events and current popup setup is fundamentally broken. 
I've filed bug 291083 on suggested changes that would "fix" this bug (lead to
consistent behavior, basically).
Depends on: 291083
Attached file Working testcase
I modified the XUL file and now it works :-)
I wrote a comment to the related/duplicate bug https://bugzilla.mozilla.org/show_bug.cgi?id=362403#c4 . Will this bug be fixed or should it be made dependent on the bug #279703?
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.widgets
No longer relevant.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: