Event.getClientX/Y() redux

144 views
Skip to first unread message

Joel Webber

unread,
Jul 30, 2008, 2:56:24 PM7/30/08
to GWTcontrib, Ray Cromwell, Sam Gross, Fred Sauer
All,

First, a quick recap of the problem we've been experiencing with the browsers' varied and exciting coordinate systems. When it comes to mouse events and absolutely-positioned elements, there are essentially two separate coordinate systems -- that defined by mouse events, getAbsoluteLeft/Top(), and used with things like Document.elementFromPoint() (which I will call 'absolute coordinates'; and that defined by an absolutely-positioned element attached to document.body (which I will call 'body coordinates'). On some browsers (Safari and Opera) these are one and the same. On others (Mozilla and Firefox) they differ slightly.

Up to now, we've been sort of hackishly accounting for this discrepancy by adjusting the result of Event.getClientX/Y(). At Sam's prodding, I (and he) did a lot of investigating of the behavior of these properties, and it turns out that they're completely consistent on all browsers (and in quirks- and standards-mode) -- the origin is *always* coincident with the viewport (even on IE). What's *different* what happens when you position an element absolutely within the body. So adjusting Event.getClientX/Y() is clearly the wrong answer.

My proposal is as follows:
- Always return the native value for Event.getClientX/Y().
- Create the methods Document.getBodyOffsetLeft/Top(), that return the offset between the 'absolute' and 'body' coordinate systems.
- Modify RootPanel and PopupPanel to take these offsets into account when positioning children of the <body> element.
  - Note that this formalizes the idea that RootPanel and PopupPanel coordinates are specified in the 'absolute' coordinate system.

The end result of this is that anyone using PopupPanel.setPopupPosition() and/or RootPanel.setWidgetPosition() will continue to be able to use the results of Event.getClientX/Y() as always. The adjustment is just happening elsewhere. Anyone wanting to manually position an element that is a child of the <body> element will have to take Document.getBodyOffsetLeft/Top() into account, just like PopupPanel and RootPanel.

The attached patch implements this proposal. I'd like everyone who cares to take a look at it and see if it makes sense. I've got to resolve one bizarre test failure on IE before we would be able to commit it, but otherwise I think it's about ready to go. Opinions?

Thanks to everyone (Sam especially) for helping me bang on this problem and reach a sensible conclusion.

Thanks,
joel.
coordinateFun_r3341.patch

Ray Cromwell

unread,
Jul 30, 2008, 3:30:35 PM7/30/08
to Joel Webber, GWTcontrib, Sam Gross, Fred Sauer
Joel,
I've currently got (it appears working) code like this to compute
relative coordinates:

int relativeY = DOM.eventGetClientY(event) -
DOM.getAbsoluteTop(element) + Window.getScrollTop();

Will have I have to adjust this with body offset?

-Ray

Joel Webber

unread,
Jul 30, 2008, 4:17:49 PM7/30/08
to Ray Cromwell, GWTcontrib, Sam Gross, Fred Sauer
You won't have to change this code at all. Because getAbsoluteTop() is defined to be the same coordinate system as Event.getClientY() (minus the scrolling), this will work correctly. Sam also suggested offline that we consider adding Event.getPageX/Y(), which exist by default on some browsers and are equivalent to the expression (Event.getClientX/Y() + Window.getScrollTop()). This would be the exact same coordinate system as absoluteLeft/Top.

Joel Webber

unread,
Jul 30, 2008, 8:50:10 PM7/30/08
to GWTcontrib, John LaBanca, Ray Cromwell, Sam Gross, Fred Sauer
Alright, time for take 2!

This is basically the same patch, with none of the design changed. It accounts for a couple of issues:
- Sam pointed out to me that the absolute position calculation was off for FF3, and that it no longer needs to account for margins/border.
- I discovered a subtle issue with the position adjustment in PopupPanel (it was adjusting the position *before* saving the value for animation). This was breaking PopupTest on IE.
- IE's getAbsoluteLeft/Top() was incorrectly subtracting out clientLeft/Top.
- FF3 was not adding in scrollLeft/Top as it should have been.

All the tests seem to pass, and I've updated the Issue1932 museum entry to show the comparison between absolute position and popup positioning (PopupTest also checks this, but just to be sure...).

@jlabanca: Can you go ahead and start a formal review of this?
@all: Please let me know if you see any problems with this patch. This is pretty squirrely stuff that I'd like to have done once and for all!

Thanks,
joel.
coordinateFun_r3351_take2.patch

Sam Gross

unread,
Jul 30, 2008, 10:45:12 PM7/30/08
to Joel Webber, Google Web Toolkit Contributors
Hi Joel,

Overall, the approach looks good to me. I think you struck a nice balance with maintaining backward compatibility for the common use cases.

user/src/com/google/gwt/dom/client/DOMImplMozilla.java
  • I think DocumentRootImpl should be used instead of doc.documentElement.  In quirks mode the scroll is on the body in FF.

user/src/com/google/gwt/user/client/ui/PopupPanel.java
  • Do you want to mention that the PopupPanel should be added to the RootPanel in the JavaDocs?
  • getPopupLeft/Top and UIObject.getAbsoluteLeft/Top are redundant. Do you want to deprecate getPopupLeft/Top?

getBodyOffsetLeft/Top:
  • method name: It's only related to the <body> on IE quirks.  That said, I don't have a suggestion for a better name.
  • What are your thoughts on adding an example to the JavaDoc?
  • @return: seems to imply that it returns body.offsetLeft/Top
nits:
user/src/com/google/gwt/dom/client/DOMImpl.java
tab on line 75
user/src/com/google/gwt/dom/client/DOMImplMozilla.java
indentation on lines 31 and 51
user/src/com/google/gwt/dom/client/DOMImplIE6.java
lines 45 and 51: indentation; '+' should be on preceding line

Regards,
Sam

Scott Blum

unread,
Jul 31, 2008, 12:03:49 AM7/31/08
to Google-Web-Tool...@googlegroups.com, John LaBanca, Ray Cromwell, Sam Gross, Fred Sauer
Glad you guys finally got to the bottom of this!!

dom.client:
- DOMImpl(75) has a tab char in it
- Document(616,629): these lines should end with one space, not two
- Document: how about example code in the javadoc, I would have no idea whether to add or subtract the returned value, which would suck in the common case where it might be 0

RootPanel:
- Consider subclassing RootPanel with a private static inner class and returning the subclass from RootPanel.get(); this will most likely optimize out the runtime test in 99% of all cases.

Fred Sauer

unread,
Jul 31, 2008, 1:23:57 AM7/31/08
to Google-Web-Tool...@googlegroups.com
Joel, Sam,

Thanks for the hard work. The last positioning bug is squashed we'll have to celebrate big time!


Joel,

Using gwt-dnd demo app, and FF3/IE7/Safari 3/Opera, plus some crazy asymmetric margin/padding/border for HTML/BODY elements, I find that most everything appears to work with take2 of the patch.

I just found two issues:
  1. FF3 document scroll causes coordinate problems in quirks mode, but is fine in standards mode
  2. IE7 zoom (both quirks + standards) throws off coordinates

Thanks!
Fred Sauer
fr...@allen-sauer.com

Joel Webber

unread,
Jul 31, 2008, 9:08:49 AM7/31/08
to Google-Web-Tool...@googlegroups.com
Thanks for the good feedback, everyone. I've attached an updated patch that should deal with all of these issues, with the exception of the crazy IE zoom thing Fred discovered (see below).

On Wed, Jul 30, 2008 at 10:45 PM, Sam Gross <sgr...@google.com> wrote:
user/src/com/google/gwt/dom/client/DOMImplMozilla.java
  • I think DocumentRootImpl should be used instead of doc.documentElement.  In quirks mode the scroll is on the body in FF.
Good catch. Fixed.
 
user/src/com/google/gwt/user/client/ui/PopupPanel.java
  • Do you want to mention that the PopupPanel should be added to the RootPanel in the JavaDocs?
Yeah, we've needed that doc for a long time. I went ahead and added it.
  • getPopupLeft/Top and UIObject.getAbsoluteLeft/Top are redundant. Do you want to deprecate getPopupLeft/Top?
I thought about it, but there's no reason *not* to call getPopupLeft/Top(), as they'll work fine, and the semantics are a bit clearer. Plus, if it turns out we're wrong about some subtle detail in the future, we'll have one more hook point where we can account for it if necessary.

getBodyOffsetLeft/Top:
  • method name: It's only related to the <body> on IE quirks.  That said, I don't have a suggestion for a better name.
I can't say I care for it much either, but I'm at a loss as for what else to call it. I've been going on the premise that there are three coordinate systems:
- client (event.clientX/Y)
- absolute (client + scrolling) (this could also be called 'page' after the non-standard event.pageX/Y)
- body (the coordinate system implied by the absolute positioning behavior of body's children)
  • What are your thoughts on adding an example to the JavaDoc?
My thoughts are that it's a good idea :) Done.
  • @return: seems to imply that it returns body.offsetLeft/Top
Agreed. I made these more specific.
 
 nits:
user/src/com/google/gwt/dom/client/DOMImpl.java
tab on line 75
user/src/com/google/gwt/dom/client/DOMImplMozilla.java
indentation on lines 31 and 51
user/src/com/google/gwt/dom/client/DOMImplIE6.java
lines 45 and 51: indentation; '+' should be on preceding line

Done.

On Thu, Jul 31, 2008 at 1:23 AM, Fred Sauer <fr...@allen-sauer.com> wrote:
  1. FF3 document scroll causes coordinate problems in quirks mode, but is fine in standards mode

Sam caught this as well. Fixed, by using DocumentRootImpl.

  2. IE7 zoom (both quirks + standards) throws off coordinates

Yikes, I've never actually used the 'zoom' feature. It looks like it creates a dichotomy between logical and device coordinates, which kind of sucks because I'm not sure where to find that ratio. Do you know if the zoom ratio (I assume it's the zoom: style) is always applied to the body or documentElement? Unless this turns out to be trivially simple, I'd be in favor of getting these patches checked in and dealing with this separately.

I also found some documentation on IE8's equivalent feature, which implies that they realize they screwed it up in IE7:
[https://blogs.msdn.com/ie/archive/2008/03/25/internet-explorer-8-and-adaptive-zoom.aspx:]
DHTML properties:  In IE7 zoom, some properties were treated as physical (e.g.: mouse coordinates) while others were logical (offset). This implementation essentially required web developers to be aware of or manually calculate the zoom state based on the property being used. In IE8 zoom, all DHTML properties are now assumed to be logical. This enables some key scenarios such as fly-out menus and "drag-and-drop". There are a few known issues, such as the incorrect scaling with screen.width and screen.height, that were not addressed in Beta 1. These will be fixed in a future release.

On Wed, Jul 30, 2008 at 10:03 PM, Scott Blum <sco...@google.com> wrote:
Glad you guys finally got to the bottom of this!!

dom.client:
- DOMImpl(75) has a tab char in it
- Document(616,629): these lines should end with one space, not two
- Document: how about example code in the javadoc, I would have no idea whether to add or subtract the returned value, which would suck in the common case where it might be 0

Done, done, done. Please see the updated doc in Document.getBodyOffsetLeft().

RootPanel:
- Consider subclassing RootPanel with a private static inner class and returning the subclass from RootPanel.get(); this will most likely optimize out the runtime test in 99% of all cases.

Good idea. Also done. This feels less dirty :)

coordinateFun_r3362_take3.patch

Fred Sauer

unread,
Jul 31, 2008, 11:00:36 AM7/31/08
to Google-Web-Tool...@googlegroups.com



On Thu, Jul 31, 2008 at 7:08 AM, Joel Webber <j...@google.com> wrote:
  2. IE7 zoom (both quirks + standards) throws off coordinates

Yikes, I've never actually used the 'zoom' feature. It looks like it creates a dichotomy between logical and device coordinates, which kind of sucks because I'm not sure where to find that ratio. Do you know if the zoom ratio (I assume it's the zoom: style) is always applied to the body or documentElement? Unless this turns out to be trivially simple, I'd be in favor of getting these patches checked in and dealing with this separately.

I also found some documentation on IE8's equivalent feature, which implies that they realize they screwed it up in IE7:
[https://blogs.msdn.com/ie/archive/2008/03/25/internet-explorer-8-and-adaptive-zoom.aspx:]
DHTML properties:  In IE7 zoom, some properties were treated as physical (e.g.: mouse coordinates) while others were logical (offset). This implementation essentially required web developers to be aware of or manually calculate the zoom state based on the property being used. In IE8 zoom, all DHTML properties are now assumed to be logical. This enables some key scenarios such as fly-out menus and "drag-and-drop". There are a few known issues, such as the incorrect scaling with screen.width and screen.height, that were not addressed in Beta 1. These will be fixed in a future release.

I'm okay with punting on this one. It's much more important to get the rest in without further ado.

Fred

Scott Blum

unread,
Jul 31, 2008, 1:34:24 PM7/31/08
to Google-Web-Tool...@googlegroups.com
LGTM.

Joel Webber

unread,
Jul 31, 2008, 1:49:49 PM7/31/08
to Google-Web-Tool...@googlegroups.com
Thanks. Committed as r3364.

Fred Sauer

unread,
Jul 31, 2008, 11:56:58 PM7/31/08
to Google-Web-Tool...@googlegroups.com
Nice!

This will be merged into trunk soon as well, right?


Fred Sauer
fr...@allen-sauer.com

Emily Crutcher

unread,
Aug 1, 2008, 12:56:50 AM8/1/08
to Google-Web-Tool...@googlegroups.com
Does the IE zoom problem interact with the css zoom property? If so, we have zoom:1  all over the place in the standard.css.
--
"There are only 10 types of people in the world: Those who understand binary, and those who don't"

Joel Webber

unread,
Aug 1, 2008, 10:16:40 AM8/1/08
to Google-Web-Tool...@googlegroups.com
@Fred: Should be merged up pretty soon.
@ecc: I haven't investigated the interaction of the 'zoom' menu with the zoom: CSS property, but I believe they're related. The good news is that zoom:1, if anything, would make the problem less likely to occur. I suspect that the zoom menu changes the zoom: property on the documentElement or body, but that remains to be proven.
Reply all
Reply to author
Forward
0 new messages