Bug 25750 - Gmail popup menus are incorrectly placed when using full page zoom
Summary: Gmail popup menus are incorrectly placed when using full page zoom
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Adam Barth
: 24177 24357 (view as bug list)
Depends on:
Reported: 2009-05-13 09:03 PDT by Brett Wilson (Google)
Modified: 2009-06-02 20:59 PDT (History)
7 users (show)

See Also:

Preliminary patch (2.19 KB, patch)
2009-05-13 09:04 PDT, Brett Wilson (Google)
no flags Details | Formatted Diff | Diff
Testcase (687 bytes, text/html)
2009-05-14 07:56 PDT, Brett Wilson (Google)
no flags Details
Patch with layout test (9.45 KB, patch)
2009-05-29 16:42 PDT, Brett Wilson (Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brett Wilson (Google) 2009-05-13 09:03:04 PDT
The code for this popup uses getBoundingClientRect which doesn't account for full page zoom.
Comment 1 Brett Wilson (Google) 2009-05-13 09:04:54 PDT
Created attachment 30278 [details]
Preliminary patch

This is a preliminary patch. The code is messy and lacks a test, but I wanted to get feedback before moving on.
Comment 2 Brett Wilson (Google) 2009-05-13 09:06:24 PDT
Hyatt, could you take a quick look at this patch? There are several FIXMEs in there for things I don't understand.

I'm also not sure if the change to Element::getClientRects is necessary. Gmail does not trigger this code path, and I don't know how to test it.
Comment 3 Brett Wilson (Google) 2009-05-13 09:07:36 PDT
To clarify, the test case is to go to gmail and click on the "More Options" popup with full page zoom enabled.
Comment 4 Brett Wilson (Google) 2009-05-13 10:12:12 PDT
Don't click "Move to" or "Labels" because of bug 25751.
Comment 5 Erik Arvidsson 2009-05-13 13:07:36 PDT
Can this be tested in a layout test?
Comment 6 Eric Seidel (no email) 2009-05-13 16:29:23 PDT
Yes, you can build a layout test to use getBoundingClientRect.

I think adding a test to this patch would actually help me understand what you're trying to do better.

Also, all your copy/paste point manipulation could be moved into an inline function.  I don't know if this is the right approach until I can better see the results of this change (via a test).  But maybe Hyatt or Simon understand better.
Comment 7 Simon Fraser (smfr) 2009-05-13 16:33:08 PDT
I recall some discussion about whether getBoundingClientRect should change under zoom. iirc, hyatt said that in IE it did not, so WebKit doesn't either.
Comment 8 Erik Arvidsson 2009-05-13 17:51:19 PDT
(In reply to comment #7)
> I recall some discussion about whether getBoundingClientRect should change
> under zoom. iirc, hyatt said that in IE it did not, so WebKit doesn't either.

IE is also broken.

I don't think it makes sense for client code to have to care about full page zoom. It should just work.

Currently, offsetWidth, style.width and others take the zoom value into account but getBoundingClientRect does not. This should be made consistent or we will end up with bugs like the one Gmail is having.

We could tell the Gmail people to adjust their code to do something like this:

var dummyElement = document.createElement('div');
dummyElement.style.cssText = 'position:absolute;width:100px;height:100px;top:-1000px;overflow:hidden';
var rect = dummyElement.getBoundingClientRect();
var zoomFactor = (rect.bottom - rect.top) / 100;

and then tell them to multiply by the zoomFactor every single time they use getBoundingClientRect(). They would also have to detect zoom changes which requires more lines of hacky code.

...or we can just fix this bug :-)
Comment 9 Brett Wilson (Google) 2009-05-14 07:56:44 PDT
Created attachment 30340 [details]

I think it's pretty clear that this function not taking into account zoom is a bug. Otherwise, what use could it possibly be? Every other time the page sees a coordinate or a size, it is adjusted for full page zoom.

I've attached a testcase. When you load it, it will position the green square over the red one. Try this: load it at 100% zoom then do full page zoom, the green square will stay over the red one. Then hit reload, the green square will now be in some crazy place.
Comment 10 Brett Wilson (Google) 2009-05-16 15:58:15 PDT
In response to Eric's comment that the rectangle moving could use a function: I agree, but I wasn't sure if the heavily duplicated code was needed at all. Only the second function causes the Gmail problem. I was hoping to get somebody who knows layout to comment on that, since I don't understand the cases in which it gets called.
Comment 11 Dave Hyatt 2009-05-29 13:20:32 PDT
Yeah fixing it sounds good.  Seems like having a function to adjust a rect would be good rather than having four lines of code like that.

Comment 12 Brett Wilson (Google) 2009-05-29 16:42:47 PDT
Created attachment 30797 [details]
Patch with layout test
Comment 13 Adam Barth 2009-06-01 00:45:26 PDT
Will land.  I hate this bug.
Comment 14 Adam Barth 2009-06-01 01:12:12 PDT
Sending        LayoutTests/ChangeLog
Adding         LayoutTests/fast/transforms/bounding-rect-zoom.html
Adding         LayoutTests/platform/mac/fast/transforms/bounding-rect-zoom-expected.txt
Sending        WebCore/ChangeLog
Sending        WebCore/dom/Element.cpp
Transmitting file data .....
Committed revision 44315.
Comment 15 Eric Seidel (no email) 2009-06-02 13:44:44 PDT
*** Bug 24177 has been marked as a duplicate of this bug. ***
Comment 16 Beth Dakin 2009-06-02 13:50:44 PDT
*** Bug 24357 has been marked as a duplicate of this bug. ***