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

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Wladimir Palant]

Attachment: Testcase

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).

Comment 1

[Gabriele Best [:Gabri]]

Attachment: Proposed patch (incomplete)

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).

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 3

[Gabriele Best [:Gabri]]

Yes I have. No mochitest fails with this patch.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 5

[Gabriele Best [:Gabri]]

Attachment: Patch v2

The previous patch was incomplete, sorry :(

Comment 6

[Gabriele Best [:Gabri]]

Attachment: Patch v2 without trailing spaces

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-            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.

Comment 8

[Neil Deakin]

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?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're right. Find the RootPresContext, get its viewmanager and then GetRootWidget.

Comment 10

[Neil Deakin]

Attachment: like so

Comment 11

[Gabriele Best [:Gabri]]

(In reply to comment #10)
> Created an attachment (id=398301) [details]
> like so

Wow! Good work! I hope it will be landed soon :P

Comment 12

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/78e908a79a9d

Comment 13

[Andreas Eibach]

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!

Comment 14

[Wladimir Palant]

Andreas, this fix will be in the next nightly (20090904). Feel free to test it and to verify that this bug was fixed.

Comment 15

[Andreas Eibach]

You bet I will :)

Comment 16

[Andreas Eibach]

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!

Comment 17

[Marco Bonardo [:mak]]

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.

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/40416b84ef2d