Bug 25750

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.

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.

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

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

Can this be tested in a layout test?

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.

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.

(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 :-)

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.

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.

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.

Created attachment 30797 [details] Patch with layout test

Will land. I hate this bug.

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.

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

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