Bug 67592

Summary: Take pageScaleFactor into account for MouseRelatedEvents.
Product: WebKit Reporter: John Knottenbelt <jknotten>
Component: New BugsAssignee: John Knottenbelt <jknotten>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, darin, dglazkov, fsamuel, hyatt, mitz, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 67836    
Bug Blocks: 68075    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

John Knottenbelt
Reported 2011-09-05 03:10:57 PDT
Take pageScaleFactor into account for MouseRelatedEvents.
Attachments
Patch (5.77 KB, patch)
2011-09-05 03:18 PDT, John Knottenbelt 		no flags
DetailsFormatted DiffDiff
Patch (10.02 KB, patch)
2011-09-14 07:00 PDT, John Knottenbelt 		no flags
DetailsFormatted DiffDiff
Patch (10.05 KB, patch)
2011-09-16 08:02 PDT, John Knottenbelt 		no flags
DetailsFormatted DiffDiff
Patch (13.88 KB, patch)
2011-09-23 10:54 PDT, John Knottenbelt 		no flags
DetailsFormatted DiffDiff
Patch (13.85 KB, patch)
2011-10-05 03:14 PDT, John Knottenbelt 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
John Knottenbelt
Comment 1 2011-09-05 03:17:31 PDT
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.
John Knottenbelt
Comment 2 2011-09-05 03:18:56 PDT
Created attachment 106317 [details] Patch
WebKit Review Bot
Comment 3 2011-09-05 09:54:59 PDT
Comment on attachment 106317 [details] Patch Clearing flags on attachment: 106317 Committed r94536: <http://trac.webkit.org/changeset/94536>
WebKit Review Bot
Comment 4 2011-09-05 09:55:03 PDT
All reviewed patches have been landed. Closing bug.
Simon Fraser (smfr)
Comment 5 2011-09-08 10:23:09 PDT
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.
John Knottenbelt
Comment 6 2011-09-09 05:39:46 PDT
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.
Simon Fraser (smfr)
Comment 7 2011-09-09 09:39:11 PDT
(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.
John Knottenbelt
Comment 8 2011-09-14 06:58:49 PDT
Reopening as my original fix had problems.
John Knottenbelt
Comment 9 2011-09-14 07:00:35 PDT
Created attachment 107327 [details] Patch
John Knottenbelt
Comment 10 2011-09-14 07:04:21 PDT
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.
WebKit Review Bot
Comment 11 2011-09-14 07:40:48 PDT
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
Simon Fraser (smfr)
Comment 12 2011-09-14 09:12:36 PDT
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
John Knottenbelt
Comment 13 2011-09-16 08:02:29 PDT
Created attachment 107653 [details] Patch
John Knottenbelt
Comment 14 2011-09-16 08:05:50 PDT
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.
Simon Fraser (smfr)
Comment 15 2011-09-16 08:45:46 PDT
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>
John Knottenbelt
Comment 16 2011-09-16 08:52:29 PDT
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.
Simon Fraser (smfr)
Comment 17 2011-09-16 09:12:48 PDT
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.
Darin Adler
Comment 18 2011-09-16 10:38:43 PDT
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.
John Knottenbelt
Comment 19 2011-09-23 10:54:49 PDT
Created attachment 108494 [details] Patch
John Knottenbelt
Comment 20 2011-09-23 10:57:32 PDT
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.
John Knottenbelt
Comment 21 2011-10-04 03:19:17 PDT
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).
Simon Fraser (smfr)
Comment 22 2011-10-04 11:15:39 PDT
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.
John Knottenbelt
Comment 23 2011-10-05 03:14:29 PDT
Created attachment 109768 [details] Patch
Tony Gentilcore
Comment 24 2011-10-06 02:03:30 PDT
Comment on attachment 109768 [details] Patch Carrying forward Simon's r+
WebKit Review Bot
Comment 25 2011-10-06 03:09:15 PDT
Comment on attachment 109768 [details] Patch Clearing flags on attachment: 109768 Committed r96798: <http://trac.webkit.org/changeset/96798>
WebKit Review Bot
Comment 26 2011-10-06 03:09:21 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.