Closed Bug 413268 Opened 16 years ago Closed 16 years ago

popupshowing from tooltip omits setting clientX/clientY properties

Categories

(Core :: XUL, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: wa84it, Assigned: enndeakin)

References

Details

(Keywords: regression, testcase)

Attachments

(4 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2

Being able to construct a tooltip based on which tree cell is hovered over is extremely useful.

Reproducible: Always

Steps to Reproduce:
1.see testcase
2.
3.
Actual Results:  
Tooltip filled with incorrect row.

Expected Results:  
Tooltip filled with correct row.
Attached file testcase
Keywords: regression, testcase
Could be a regression from the coordinate changes.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Just to make it entirely clear that the bug does not only affect the row index.
This is a regression from the "popupshowing is not a mouse event" changes.
This is because the mouse event that holds the coordinates is no longer available when the tooltip is being shown (the tooltip is opened on a timer), so no coordinates are available within a tooltip's popupshowing event.

You could work around it for now by caching the last coordinates from a mousemove event.
 
(In reply to comment #5)
> 
> You could work around it for now by caching the last coordinates from a
> mousemove event.
> 
Yeah, thanks, I already did that; it's always work around it first, file bug report later if at all....  I filed this one since the workaround takes so many CPU cycles, which admittedly is only a real problem if the user has other background tasks running.  It may however create user flak of the 'how come you're using so much of my CPU' variety.

As Neil seems to insist that this one is the new bug #306202 lets get it more to the point.

Actually popupshowing for all tooltips is affected from this regression.
It is totally unrelated to trees, however one of the main use cases seems to be populating tooltips with data from trees.

This seems to break a lot of extension/third-party application code.
It was reported twice additionally to this bug and also mentioned on m.d.extensions.
Because of this, and recent announcements that extension compatibility issues are taken (more) seriously, requesting blocking-1.9


I'd really really like to see this fixed as the mouse-move "workaround" is plain ugly IMHO. 
Component: XP Toolkit/Widgets: Trees → XP Toolkit/Widgets: Menus
Flags: blocking1.9?
OS: Windows XP → All
QA Contact: xptoolkit.trees → xptoolkit.menus
Hardware: PC → All
Summary: no dynamically filled tree(treechildren) tooltip in FF3 → popupshowing from tooltip omits setting clientX/clientY properties
Version: unspecified → Trunk
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Attached patch let's try this patch (obsolete) — Splinter Review
Update the tooltip test to check the coordinates as well.
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attachment #304244 - Flags: superreview?(neil)
Attachment #304244 - Flags: review?(Olli.Pettay)
Comment on attachment 304244 [details] [diff] [review]
let's try this patch

>+        if ((event->eventStructType == NS_MOUSE_EVENT || 
>+             event->eventStructType == NS_MOUSE_SCROLL_EVENT) &&
>+            !(static_cast<nsGUIEvent *>(event))->widget) {
>+          // no widget, so just use the client point if available
>+          nsCOMPtr<nsIDOMMouseEvent> mouseEvent = do_QueryInterface(aEvent);
>+          mouseEvent->GetClientX(&mCachedMousePoint.x);
>+          mouseEvent->GetClientY(&mCachedMousePoint.y);
>+          return;
>+        }
>+
>         nsIDocument* doc = aPopup->GetCurrentDoc();
>         if (doc) {
>           nsIPresShell* presShell = doc->GetPrimaryShell();
>           if (presShell) {
>             nsPresContext* presContext = presShell->GetPresContext();
>             nsIFrame* rootFrame = presShell->GetRootFrame();
>             if (rootFrame && presContext) {
>               nsPoint pnt =
Couldn't you have the new |if| as a 'else if' after |if (rootFrame && presContext) {| ?

Did you test the patch with some page zoom?

With those addressed/explained, r=me
Attachment #304244 - Flags: review?(Olli.Pettay) → review+
(Why) do we need to create a new mouse event?
Um, right. What was I thinking when reading the patch...
Why do we need to create a new mouse event?
Attachment #304244 - Flags: review+ → review?(Olli.Pettay)
Comment on attachment 304244 [details] [diff] [review]
let's try this patch

We don't want to create those events.
(Did I somehow skip this part of the patch when reading it first time or what happened? Anyway, sorry)
Attachment #304244 - Flags: superreview?(neil)
Attachment #304244 - Flags: review?(Olli.Pettay)
Attachment #304244 - Flags: review-
Better to just have a new version of ShowPopupAtScreen method which takes the
necessary coordinates as parameters?
> (Why) do we need to create a new mouse event?

Because I made a silly mistake when I tried it and thought it didn't work :(
Attachment #304244 - Attachment is obsolete: true
Attachment #304493 - Flags: superreview?(neil)
Attachment #304493 - Flags: review?(Olli.Pettay)
Comment on attachment 304493 [details] [diff] [review]
updated patch, don't create extra event and add conversion to device pixels

>           nsIPresShell* presShell = doc->GetPrimaryShell();
>-          if (presShell) {
>+          if (presShell && presShell->GetPresContext()) {
>             nsPresContext* presContext = presShell->GetPresContext();
>             nsIFrame* rootFrame = presShell->GetRootFrame();
So here you get rootFrame, which isn't necessarily used at all?
Any reason for that? Perhaps better to move that to the |else|
(In reply to comment #16)
> So here you get rootFrame, which isn't necessarily used at all?
> Any reason for that? Perhaps better to move that to the |else|

Yes, I've move that into the other block.
Comment on attachment 304493 [details] [diff] [review]
updated patch, don't create extra event and add conversion to device pixels

With that r=me
I tested with zoom, and testcases seemed to work.
I guess we don't have CSSPixelsToDevPixels or something like that.
Attachment #304493 - Flags: review?(Olli.Pettay) → review+
Attachment #304493 - Flags: superreview?(neil) → superreview+
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
did this cause the win box mochitest going orange?
Works for original reporter in original real case with current nightly - thanks.
Component: XP Toolkit/Widgets: Menus → XUL
QA Contact: xptoolkit.menus → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.