82597 - event.client[X|Y] not filled when creating tooltip popup

Status: VERIFIED WONTFIX

(Core :: XUL, defect)

Description

[Jan Varga [:janv]]

Testcase is coming...

Comment 1

[Jan Varga [:janv]]

Attachment: testcase

Comment 2

[Jan Varga [:janv]]

Attachment: initial fix

Comment 3

[Jan Varga [:janv]]

There is still a small problem to fix.
Tooltip should disappear on mouse move.

Mike, can you take a look ?

Comment 4

[Mike Pinkerton (not reading bugmail)]

cool, thanks!

Comment 5

[Mike Pinkerton (not reading bugmail)]

oops, sorry, thought this was on my list.

Comment 6

[Mike Pinkerton (not reading bugmail)]

tooltips go away when the mouse leaves the frame associated with the content node 
that the mouse was over when it appeared (event.target). they don't go away on 
any ol' mouse move. since outliner is one frame, the tip won't go away until you 
leave the outliner.

Comment 7

[Jan Varga [:janv]]

Attachment: diff in content

Comment 8

[Jan Varga [:janv]]

Attachment: diff in layout

Comment 9

[Jan Varga [:janv]]

I've attached better fix.

Mike, note this:

-        self->LaunchPopup ( self->mMouseClientX, self->mMouseClientY+21);
+        self->LaunchPopup ( self->mMouseClientX, self->mMouseClientY+21,
+                            self->mMouseClientX, self->mMouseClientY );

Comment 10

[Håkan Waara]

Mike, we need this for the folder_outliner branch which now is nearly ready for
landing. Can you review the attached fix?

Comment 11

[Jan Varga [:janv]]

Attachment: new diff in content

Comment 12

[Jan Varga [:janv]]

Attachment: new diff in layout

Comment 13

[Mike Pinkerton (not reading bugmail)]

as we discussed before, the +21 is a hack and i can guarantee won't work in all
cases since it's probably the offset of this particular frame in its view. You
really need to find a programmatic way to come up with this 21 value, probably
by walking the parent chain (not sure view or widget) and adding the offsets. 

Comment 14

[Jan Varga [:janv]]

Mike, did you take a look at last patches ?
It works for me with those patches.

Comment 15

[Mike Pinkerton (not reading bugmail)]

yes, i looked at the patches. i don't like this:

+  else if ( popupType == eXULPopupType_tooltip ) {
     type.AssignWithConversion("tooltip");
+    yDelta += 21;
+  }

you need to come up with a programmatic way of computing the 21.

Comment 16

[Jan Varga [:janv]]

But it was hardcoded this way also before:
-        self->LaunchPopup ( self->mMouseClientX, self->mMouseClientY+21);
+        self->LaunchPopup ( self->mMouseClientX, self->mMouseClientY )

I'm sorry but I'm not sure what do you mean by "programmatic way" ?

Comment 17

[Mike Pinkerton (not reading bugmail)]

oh dear lord, you're right. sorry about that. we really need to explain the
origin of that constant, but that can be left for me to do.

I'll look at the patch again later today, and this time i'll promise to turn my
brain on first ;)

Comment 18

[Mike Pinkerton (not reading bugmail)]

r=pink on the code. sorry for the delay and being so braindead.

I would prefer a name other than |xDelta| and |yDelta| for the params to
createPopup. Any ideas? How about |xAdjust| and |yAdjust| since that's really
what these params are doing: adjusting the popup position for various circumstances.

Comment 19

[Jan Varga [:janv]]

Attachment: final diff in content

Comment 20

[Jan Varga [:janv]]

Attachment: final diff in layout

Comment 21

[Jan Varga [:janv]]

I've renamed delta to adjust according to Mike's suggestion.

Comment 22

[David Hyatt]

I do not agree with this patch. (Sorry!)  I don't believe any additional
arguments should be added to createPopup.  The "create" and "destroy" events are
not intended to be mouse events either, since they can be invoked via the
keyboard or programmatic means.

In the interest of understanding this, could you explain
(a) Why the create/destroy events would ever need mouse coords?
(b) What is the purpose of the adjust args that have been added?

Comment 23

[Jan Varga [:janv]]

>(a) Why the create/destroy events would ever need mouse coords?

We have tooltip popup in folderPane.xul:
<popup id="folderTooltip" class="tooltip" oncreate="return
FillInFolderTooltip(event);">
  <text id="folderTooltipText"/>
<popup>

When this tooltip is created we need to know mouse coordinates to find out
corresponding row index of outliner:
function FillInFolderTooltip(event) {
  ...
  getCellAt(event.clientX, event.clientYdocument.tooltipNode, row, col, elt)
  var folderResource = GetFolderResource(row.value);
  var msgFolder =   
  folderResource.QueryInterface(Components.interfaces.nsIMsgFolder);
  var unreadCount = msgFolder.getNumUnread(false);
  var totalCount = msgFolder.getTotalMessages(false); 
  var folderTooltip = GetFolderAttribute(folderResource, 'Name');
  folderTooltip += " ("  + unreadCount + "/" + totalCount +")";
  textNode.setAttribute('value', folderTooltip);
  return true;
}

Before it was easier because we used "document.tooltipNode" to access
corresponding <treecell>. But now there is only one frame <outlinerbody>



>(b) What is the purpose of the adjust args that have been added?
One problem was to get mouse coordinates, another problem was that those
coordinates was slightly "adjusted". Fox example y position is adjusted by 21
pixels to display *tooltip* popups and context popups are adjusted by 2 pixels
in both directions.
This have been done *before* passing to method createPopup (in nsXULPopupListener)
Therefore we had to add new parameters xAdjust and yAdjust to fill mouse event
with correct values



I don't know if there is better solution

Comment 24

[Jan Varga [:janv]]

sorry, I messed my comment a little bit 
-getCellAt(event.clientX, event.clientYdocument.tooltipNode, row, col, elt)
+getCellAt(event.clientX, event.clientY, row, col, elt)

Comment 25

[Jan Varga [:janv]]

It seems that we have a better solution.
Marking as WONTFIX.

Comment 26

[Jan Varga [:janv]]

marked as fixed by mistake

Comment 27

[Jan Varga [:janv]]

wontfix

Comment 28

[John Morrison]

Which way to the better solution? (just curious, bug number?)

Comment 29

[Jan Varga [:janv]]

See last comments in 73865.