Bug 25750 – Gmail popup menus are incorrectly placed when using full page zoom

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Brett Wilson (Google)]

The code for this popup uses getBoundingClientRect which doesn't account for full page zoom.

[Brett Wilson (Google)]

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.

[Brett Wilson (Google)]

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.

[Brett Wilson (Google)]

To clarify, the test case is to go to gmail and click on the "More Options" popup with full page zoom enabled.

[Brett Wilson (Google)]

Don't click "Move to" or "Labels" because of bug 25751.

[Erik Arvidsson]

Can this be tested in a layout test?

[Eric Seidel (no email)]

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.

[Simon Fraser (smfr)]

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.

[Erik Arvidsson]

(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';
document.body.appendChild(dummyElement);
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 :-)

[Brett Wilson (Google)]

Created attachment 30340 [details]
Testcase

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.

[Brett Wilson (Google)]

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.

[Dave Hyatt]

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.

[Brett Wilson (Google)]

Created attachment 30797 [details]
Patch with layout test

[Adam Barth]

Will land.  I hate this bug.

[Adam Barth]

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.

[Eric Seidel (no email)]

*** Bug 24177 has been marked as a duplicate of this bug. ***

[Beth Dakin]

*** Bug 24357 has been marked as a duplicate of this bug. ***