Closed
Bug 413268
Opened 16 years ago
Closed 16 years ago
popupshowing from tooltip omits setting clientX/clientY properties
Categories
(Core :: XUL, defect, P2)
Core
XUL
Tracking
()
RESOLVED
FIXED
People
(Reporter: wa84it, Assigned: enndeakin)
References
Details
(Keywords: regression, testcase)
Attachments
(4 files, 1 obsolete file)
1.16 KB,
application/vnd.mozilla.xul+xml
|
Details | |
1.20 KB,
application/vnd.mozilla.xul+xml
|
Details | |
814 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
14.58 KB,
patch
|
smaug
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
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.
Keywords: regression,
testcase
Assignee | ||
Comment 2•16 years ago
|
||
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.
Comment 4•16 years ago
|
||
This is a regression from the "popupshowing is not a mouse event" changes.
Assignee | ||
Comment 5•16 years ago
|
||
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.
Comment 7•16 years ago
|
||
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
Comment 8•16 years ago
|
||
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Assignee | ||
Comment 9•16 years ago
|
||
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 10•16 years ago
|
||
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+
Comment 11•16 years ago
|
||
(Why) do we need to create a new mouse event?
Comment 12•16 years ago
|
||
Um, right. What was I thinking when reading the patch... Why do we need to create a new mouse event?
Updated•16 years ago
|
Attachment #304244 -
Flags: review+ → review?(Olli.Pettay)
Comment 13•16 years ago
|
||
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-
Comment 14•16 years ago
|
||
Better to just have a new version of ShowPopupAtScreen method which takes the necessary coordinates as parameters?
Assignee | ||
Comment 15•16 years ago
|
||
> (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 16•16 years ago
|
||
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|
Assignee | ||
Comment 17•16 years ago
|
||
(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 18•16 years ago
|
||
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+
Updated•16 years ago
|
Attachment #304493 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Flags: in-testsuite+
Comment 19•16 years ago
|
||
did this cause the win box mochitest going orange?
Reporter | ||
Comment 20•16 years ago
|
||
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.
Description
•