popupshowing event doesn't set event.clientX/Y correctly inside a frame

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
10 years ago
6 years ago

People

(Reporter: gaubugzilla, Assigned: enndeakin)

Tracking

(Blocks 1 bug, {regression})

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Posted file Testcase (obsolete) —
I noticed that latest Minefield builds no longer set event.clientX/event.clientY correctly for the "popupshowing" event - if the event happens inside a frame the client coordinates are calculated relative to the top document rather than the frame area. This doesn't happen for the "click" event however, there the coordinates are calculated correctly. Firefox 3.5 also calculates client coordinates relative to the frame.

Testcase is attached. It needs to be opened as a chrome window - download it and then run the following command from Error Console:

openDialog("file:///.../popupshowing_test.xul", "", "chrome")

file:///.../popupshowing_test.xul should be replaced by the download location of the testcase of course. Right-click inside the bordered rectangle (the frame), the client coordinates of your click will be displayed. Firefox 3.5.2 displays them relative to the rectangle (frame) border, current Minefield builds relative to the window border.

The regression range is 2009072104 to 2009072204 so this was most likely regressed by compositor landing (see bug 374980 comment 24).
Assignee: nobody → roc
Flags: blocking1.9.2?
Blocks: 244371
Posted patch Proposed patch (incomplete) (obsolete) — Splinter Review
This bug was caused by attachment 388193 [details] [diff] [review] of bug 352093, that also caused "regression" of bug 495002 infact this patch uses "CSSPixelsToDevPixels" again instead of the longer "AppUnitsToDevPixels(nsPresContext::CSSPixelsToAppUnits())" (see bug 495002).
Attachment #396739 - Flags: review?(roc)
I don't think this is right. It will break popup coordinates in IFRAMEs. IIRC there's a mochitest for that, have you run mochitests?
Yes I have. No mochitest fails with this patch.
OK, but I don't understand why this would be correct. clientPt is relative to the viewport of the iframe that received the event. mCachedMousePoint is supposed to be relative to the viewport of the root content document. So why do you think this is correct?
Attachment #396739 - Flags: review?(roc)
Posted patch Patch v2 (obsolete) — Splinter Review
The previous patch was incomplete, sorry :(
Attachment #396739 - Attachment is obsolete: true
Attachment #397857 - Attachment is obsolete: true
Attachment #396739 - Attachment description: Proposed patch → Proposed patch (incomplete)
-            nsIFrame* rootDocumentRootFrame = rootDocPresContext->
-                PresShell()->FrameManager()->GetRootFrame();
+            nsIFrame* rootDocumentRootFrame = presShell->GetRootFrame();

rootDocumentRootFrame is no longer the root document's root frame. Instead it's the popup's document root frame.

And you still have the problem that you're storing clientPt, which is relative to the document that received the event, in mCachedMousePoint, which is supposed to be relative to the root content document.
I think the coordinates are being set correctly here, relative to the root document's root frame.

The event's widget though is being set to the one for the child frame:

  presShell->GetViewManager()->GetRootWidget(getter_AddRefs(event.widget));

So the coordinates are being offset by the child position twice. It should be the setting the root document's root widget I think here. What's the best way to do this?
You're right. Find the RootPresContext, get its viewmanager and then GetRootWidget.
Posted patch like soSplinter Review
Assignee: roc → enndeakin
Attachment #394069 - Attachment is obsolete: true
Attachment #397858 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #398301 - Flags: review?(roc)
(In reply to comment #10)
> Created an attachment (id=398301) [details]
> like so

Wow! Good work! I hope it will be landed soon :P
http://hg.mozilla.org/mozilla-central/rev/78e908a79a9d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Good work everybody!

Let me introduce myself here, I'm the initial reporter who reported this behavior to W. Palant in his ABP forum, and I slowly began to doubt this is ABP-related, but rather something underneath.

Can it be expected to get this fix into the next 3.7a1 Minefield builds too the next days? Because that's what I'm using right now.

Thanks again!
Andreas, this fix will be in the next nightly (20090904). Feel free to test it and to verify that this bug was fixed.
You bet I will :)
OK, new nightly is out and yes, this is the very first time in 3 months that the test builds will show the internal context menu in AdBlock Plus.
(if you have ABP installed, this menu appears if you press CTRL+SHIFT+V to get the "Blockable items on current page" pane, then right-click the mouse)

Thank you for your hard work everyone!
Attachment #398301 - Flags: approval1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
Comment on attachment 398301 [details] [diff] [review]
like so

this is now blocking, no more need for approval, i'll land it asap on 1.9.2.
Attachment #398301 - Flags: approval1.9.2?
Depends on: tooltips
Blocks: tooltips
No longer depends on: tooltips
You need to log in before you can comment on or make changes to this bug.