385536 - a tree inside a panel doesn't respond to clicks

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Asaf Romano (gone)]

a tree inside a panel doesn't respond to clicks (mac only?).

Comment 1

[Asaf Romano (gone)]

Attachment: testcase

Comment 2

[Asaf Romano (gone)]

I see this on windows as well.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

roc and I aren't the right people to triage the blocking1.9? flag here.  Not sure how to fix that...

Comment 4

[Neil Deakin]

Attachment: use right coordinates

Comment 5

[Neil Deakin]

This bug occurs because clientX and clientY are 'unusual' when inside popups, and even more so when the popup contains a child with a widget, to the point where different values are returned for the same location just because a <deck> exists elsewhere inside a popup.

This patch makes it so that clientX/clientY within popups returns a value relative to the popup's topleft corner. Another possibility is to return the values relative to the document.

I don't really understand if this is the right patch, but it seems to work ok.

Thoughts?

Comment 6

[Eli Friedman]

Attachment: Fix clientX/Y

Here's a possibility.  I'd need approval to land this for 1.9, though.

Comment 7

[Eli Friedman]

I guess I ought to say a bit more.  The original reason for this mess of client coordinates was that the popup code used clientX/Y in a lot of places, and expected it to be in this funny not-quite-client-coordinate system.  With the popup rewrite, that's no longer the case, so it's probably safe to just make it behave the way you'd expect even if there are popups.

There is some chance that content using popups expects the old behavior, though, so fixing it properly is potentially a breaking change.

Comment 8

[[e-mail]]

(In reply to comment #7)
>I guess I ought to say a bit more.  The original reason for this mess of client
>coordinates was that the popup code used clientX/Y in a lot of places, and
>expected it to be in this funny not-quite-client-coordinate system.  With the
>popup rewrite, that's no longer the case, so it's probably safe to just make it
>behave the way you'd expect even if there are popups.
Which is? I'm hoping that event.clientX == event.screenX - event.target.ownerDocument.documentElement.boxObject.screenX
as per the XXXbz comment in mailWidgets.xml :-)

Comment 9

[Neil Deakin]

(In reply to comment #6)
> Created an attachment (id=274069) [details]
> Fix clientX/Y
> 
> Here's a possibility.  I'd need approval to land this for 1.9, though.
> 

Yes, this is probably a better idea.

Comment 10

[Eli Friedman]

(In reply to comment #8)
> (In reply to comment #7)
> Which is? I'm hoping that event.clientX == event.screenX -
> event.target.ownerDocument.documentElement.boxObject.screenX
> as per the XXXbz comment in mailWidgets.xml :-)

My patch makes that true.

Comment 11

[Eli Friedman]

Correction: my patch makes that true except in situations where zoom is involved.  (See the discussion in m.d.t.layout.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

I am so totally in favor of Eli's patch.  I do worry about extensions that might depend on the client* in popups like that, but since it would be broken pretty much whatever they tried to do with it (c.f. mailwidgets.xml), I think there aren't many, if any.

Comment 13

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

Seconded.

Comment 14

[Eli Friedman]

Comment on attachment 274069 [details] [diff] [review]
Fix clientX/Y

If we're going to get this in, I should ask for review.

Comment 15

[Eli Friedman]

Checked in.  (Might be a little difficult to write tests with popups; I guess we can do it with mochitests.)

Comment 16

[[e-mail]]

So does this mean we can remove all bz's hacks such as this one:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/base/resources/content/mailWidgets.xml&rev=1.76&root=/cvsroot&mark=1711-1713#1705

Comment 17

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

I think so!

Comment 18

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

One thing I thought of belatedly: getBoundingClientRect for an element inside an SVG foreignobject uses the nearest foreignobject as the root of its coordinate system. Event client coordinates probably should do.

Comment 19

[Eli Friedman]

(In reply to comment #18)
> One thing I thought of belatedly: getBoundingClientRect for an element inside
> an SVG foreignobject uses the nearest foreignobject as the root of its
> coordinate system. Event client coordinates probably should do.

Maybe... what exactly was the justification for getBoundingClientRect returning coordinates relative to the nearest foreignobject?  I don't think we want to force web authors to check whether the target content has a foreignobject parent before using clientX/Y.

Comment 20

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

The justification was that the obvious alternative behaviour --- taking the bounding rect of the SVG-transformed element(s) --- would confuse most uses of getBoundingClientRect.

I'm not sure what to do here.

Comment 21

[Bob Clary [:bc] (inactive)]

Please see bug 391024.

Comment 22

[Neil Deakin]

Attachment: automated test

Comment 23

[Neil Deakin]

Checked in the test