Take pageScaleFactor into account for MouseRelatedEvents.
Mouse-related events currently take account of the zoom factor, but they also need to take account of the page scale factor so that pageX and pageY event coordinates are properly determined.
Created attachment 106317 [details] Patch
Comment on attachment 106317 [details] Patch Clearing flags on attachment: 106317 Committed r94536: <http://trac.webkit.org/changeset/94536>
All reviewed patches have been landed. Closing bug.
Comment on attachment 106317 [details] Patch Why doesn't frameView->windowToContents() already take the page scale factor into account? I think it should. Page scale factor is implemented as a transform on the RenderView's layer.
I noticed that this change unintentionally modified the MouseEvent's absoluteLocation, so I have reverted the patch (with https://bugs.webkit.org/show_bug.cgi?id=67836 ) for now. Simon, can you take a look at the layout test in this patch and let me know if that makes sense to you? If we leave the mouse where it is, scale the page out and then click again, we do expect the pageX and pageY to change because now the mouse is over a different area of the web page.
(In reply to comment #5) > (From update of attachment 106317 [details]) > Why doesn't frameView->windowToContents() already take the page scale factor into account? I think it should. Actually I may be wrong there. The test does make sense to me; it's a good test.
Reopening as my original fix had problems.
Created attachment 107327 [details] Patch
The new patch uploads some extended test cases. It would be really helpful to get feedback on whether these are correct or not. Interestingly, the test involving the iframe already seems to correctly take into account pageScaleFactor (as set by EventSender) and the CSS style transform ("-webkit-transform"). The layout tests also demonstrate that my previous fix is an incomplete in that it breaks the currently passing iframe test. If everybody agrees that on the expected result for the iframe test, I can submit that separately since no new code is needed to make it pass.
Comment on attachment 107327 [details] Patch Attachment 107327 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9659528 New failing tests: fast/events/page-scaled-mouse-click.html
Comment on attachment 107327 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107327&action=review > LayoutTests/fast/events/script-tests/page-scaled-mouse-click-iframe.js:2 > +description("This tests that page scaling does not affect mouse event pageX and pageY coordinates for " + > + "content embedded in an iframe."); It looks like it's also testing CSS transforms? > LayoutTests/fast/events/script-tests/page-scaled-mouse-click-iframe.js:4 > +var html = document.getElementsByTagName("html")[0]; This can be document.documentElement. > LayoutTests/fast/events/script-tests/page-scaled-mouse-click.js:3 > +var html = document.getElementsByTagName("html")[0]; Ditto
Created attachment 107653 [details] Patch
Thanks for your review, Simon. I've made the changes you suggested. Yes, the patch also tests CSS transforms as it seems related. When in an iframe, both page scaling and CSS transform works correctly, but when in the main document neither work. I think that this could be a clue to a proper fix.
Comment on attachment 107653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=107653&action=review I'm confused about the state of the code here. It looks like you made a fix to MouseRelatedEvent and then backed it out. This patch only contains test cases. Do these tests pass or fail with current builds? > LayoutTests/fast/events/page-scaled-mouse-click-iframe.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> These should be just <!DOCTYPE html>
Thanks for the review again. The status is that right now, all I have is are these two test cases. I do not have a proposed fix yet. My initial fix was incorrect - your comment about FrameView::windowToContents and page scaling being implemented as a transform on the RenderView's layer made me investigate further. Fady Samuel also suggested to test the iframe case. Right now, the iframe test case is passes, but the plain document test case does not. What's happening is that FrameView::windowToContents == ScrollView::windowToContents appears not to be applying any transforms if there is no parent ScrollView.
Comment on attachment 107653 [details] Patch These tests seem fine, but they can't be checked in without an associated code fix, so r-. You should include them with the final patch.
For the future: I think itβs OK to land tests with failing expected results and then later land the bug fix and revise successful expected results.
Created attachment 108494 [details] Patch
This patch takes into account page scaling for mouse events, so that the pageX and pageY are correctly calculated. This patch preserves the existing correct behaviour of iframes (which work correctly with CSS transforms and page scaling), and fixes the behaviour of plain documents with respect to page scaling. MouseEvents still need to take into account CSS transforms for main documents (when an iframe is not involved), and I have created bug https://bugs.webkit.org/show_bug.cgi?id=68703 to address this problem.
Simon any comments on this patch would be much appreciated. My take on the windowToContents question is that it should not take into account the page scale factor, since the windowLocation is in the same coordinate space as the scrollPosition, which is post scaled (see https://bugs.webkit.org/show_bug.cgi?id=68081 which fixes a bug relating to DOMWindow::scrollX() and DOMWindow::scrollY() needing to take the scaling factor into account).
Comment on attachment 108494 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=108494&action=review > LayoutTests/fast/events/script-tests/page-scaled-mouse-click-iframe.js:35 > + debug("This test requires DumpRenderTree. Click on the blue rect with the left mouse button to log the mouse coordinates.") ... or WebKitTestRunner. > Source/WebCore/dom/MouseRelatedEvent.cpp:53 > + return LayoutSize(frameView->scrollX() / scaleFactor, > + frameView->scrollY() / scaleFactor); You should unwrap this line.
Created attachment 109768 [details] Patch
Comment on attachment 109768 [details] Patch Carrying forward Simon's r+
Comment on attachment 109768 [details] Patch Clearing flags on attachment: 109768 Committed r96798: <http://trac.webkit.org/changeset/96798>