387659 - popupshowing event needs coordinates set

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[koen]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1.4) Gecko/20070508 Iceweasel/2.0.0.4 (Debian-2.0.0.4-0etch1)
Build Identifier: XULRunner 1.9a7pre - 2007070319

In version 1.8.0.12 I used treeBoxObject.getCellAt to get the row+column of the item that was right-click (contextmenu).
In version 1.9a7pre treeBoxObject no longer exists and there appears to be no other way to get the row+column information.


xul-code

  <popupset>
    <popup id="treeActionMenu" onpopupshowing="changeTreePopup(event);">
    </popup>
  </popupset>

    <tree flex="1" id="treecluster">
      <treecols>
        <treecol id="resources" label="Resources" primary="true" flex="1" />
      </treecols>
      <treechildren context="treeActionMenu" id="clusterTree">
      </treechildren>
    </tree>

js-code:

function changeTreePopup(event) {
  var tree = event.rangeParent;
  if(!tree) return;

  var row = new Object();
  var col = new Object();
  var childElement = new Object();

  tree.treeBoxObject.getCellAt(event.clientX, event.clientY, row, col, childElement);


Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Adam Guthrie]

Are you sure? I see plenty of places in the tree where we're using this; e.g. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/components/places/content/sidebarUtils.js&rev=1.1&mark=41-44#39.

Comment 2

[koen]

I've made a small test. It appears the event passed to the popup-handler is wrong. Not the treeBoxObject.
In version 1.9a7pre, event.clientX and event.rangeParent is undefined in the popuphandler but it is in the mousemovehandler. (code below)

xulrunner 1.8.0.12:

M: treecluster 101 55
C: treecluster 103 57
1 [object TreeColumn]
M: treecluster 101 55

xulrunner 1.9a7pre:

M: treecluster 111 49
rangeParent undefined
C: treecluster undefined undefined
-1 null
M: treecluster 111 49


tree.xul

<?xml version="1.0"?>
<?xml-stylesheet href="chrome://global/skin/" type="text/css"?>

<window id="treebug" title="treebug" width="400" height="500"
        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
  
  <script src="tree.js"/>

  <popupset>
    <popup id="treeActionMenu" onpopupshowing="changeTreePopup(event);">
      <menuitem label="Hello world"/>
    </popup>
  </popupset>
  
  <tree flex="1" id="treecluster"  onmousemove="updateMouseCoordinates(event);">
    <treecols>
      <treecol id="resources" label="Resources" primary="true" flex="1" />
    </treecols>
    <treechildren context="treeActionMenu" id="clusterTree">
      <treeitem label="test 1"/>
      <treeitem label="test 2"/>
    </treechildren>
  </tree>
  
</window>


tree.js

function updateMouseCoordinates(event) {
  var tree = event.rangeParent;
  println("M: " + tree.id + " " + event.clientX + " " + event.clientY);  
}
    
function changeTreePopup(event) {  
  var tree = event.rangeParent;
  if(!tree) {
    println("rangeParent undefined");
    tree = event.explicitOriginalTarget.parentNode;
  }

  println("C: " + tree.id + " " + event.clientX + " " + event.clientY);  
    
  var row = new Object();
  var col = new Object();
  var childElement = new Object();
  tree.treeBoxObject.getCellAt(event.clientX, event.clientY, row, col, childElement);
  
  println(row.value + " " + col.value);  
}

function println(s) {
  dump(s + "\n");
}

Comment 3

[Neil Deakin]

The issue here is that the x/y coordinates are not being set in the popupshowing event. This apparently happened for some popups before but not all, whereas now it isn't happening for any of them.

Your example doesn't show how you open the popup though, so it's hard to tell.

Comment 4

[koen]

The 2 files I gave contain everything needed to generate the problem.

By setting 'context="treeActionMenu"' the popup is opened.

Comment 5

[Neil Deakin]

Attachment: use mouse events and get right coordinates in popupshowing event

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 272499 [details] [diff] [review]
use mouse events and get right coordinates in popupshowing event

I have to admit I'm not up on our event stuff.  In particular, I'd like eli to look at the refpoint/widget stuff (should we be using the same widget as the original event, e.g.?) and smaug to look at the general use of nsMouseEvent here.

Comment 7

[Eli Friedman]

Why are you adding a parameter to GetMouseLocation?  It doesn't appear to be used in this patch.

Your code to cache mCachedMousePoint is wrong; you need to translate into some fixed coordinate system.  Not sure what widgets are conveniently available from SetMouseLocation; maybe relative to the root frame/widget of the root prescontext?

You should make sure to add a test using overflow: auto or an XUL deck.

Comment 8

[Neil Deakin]

(In reply to comment #7)
> Why are you adding a parameter to GetMouseLocation?  It doesn't appear to be
> used in this patch.
> 

I'll remove that.

> Your code to cache mCachedMousePoint is wrong; you need to translate into some
> fixed coordinate system.  Not sure what widgets are conveniently available from
> SetMouseLocation; maybe relative to the root frame/widget of the root
> prescontext?
> 

Would it be sufficient to use GetEventCoordinatesRelativeTo to get the coordinates  relative to the root frame of the document containing the popup node? This is the widget used when the cached point is used again.

This seems to work for the case inside a deck or iframe.

Comment 9

[Eli Friedman]

(In reply to comment #8)
> Would it be sufficient to use GetEventCoordinatesRelativeTo to get the
> coordinates  relative to the root frame of the document containing the popup
> node? This is the widget used when the cached point is used again.
> 
> This seems to work for the case inside a deck or iframe.

Yes, that should be fine.  I wasn't sure if you had an easy way to get to it from there.

Comment 10

[Neil Deakin]

Attachment: get right widget coordinates

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 272546 [details] [diff] [review]
get right widget coordinates

Looks fine

Comment 12

[Eli Friedman]

Comment on attachment 272546 [details] [diff] [review]
get right widget coordinates

Should be AppUnitsToDevPixels, not AppUnitsToIntCSSPixels.  Otherwise looks fine.