Closed Bug 387659 Opened 13 years ago Closed 13 years ago

popupshowing event needs coordinates set

Categories

(Core :: XUL, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: koen, Assigned: enndeakin)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Version: unspecified → Trunk
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");
}
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.
Summary: treeBoxObject.getCellAt function no longer works → popupshowing event needs coordinates set
The 2 files I gave contain everything needed to generate the problem.

By setting 'context="treeActionMenu"' the popup is opened.
Status: UNCONFIRMED → ASSIGNED
Depends on: 324963
Ever confirmed: true
Assignee: nobody → enndeakin
Attachment #272499 - Flags: superreview?(bzbarsky)
Attachment #272499 - Flags: review?(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.
Attachment #272499 - Flags: review?(sharparrow1)
Attachment #272499 - Flags: review?(bzbarsky)
Attachment #272499 - Flags: review?(Olli.Pettay)
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.
(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.

(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.
Attachment #272499 - Attachment is obsolete: true
Attachment #272546 - Flags: superreview?(bzbarsky)
Attachment #272546 - Flags: review?(sharparrow1)
Attachment #272499 - Flags: superreview?(bzbarsky)
Attachment #272499 - Flags: review?(sharparrow1)
Attachment #272499 - Flags: review?(Olli.Pettay)
Comment on attachment 272546 [details] [diff] [review]
get right widget coordinates

Looks fine
Attachment #272546 - Flags: superreview?(bzbarsky) → superreview+
Comment on attachment 272546 [details] [diff] [review]
get right widget coordinates

Should be AppUnitsToDevPixels, not AppUnitsToIntCSSPixels.  Otherwise looks fine.
Attachment #272546 - Flags: review?(sharparrow1) → review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.