67592 – Take pageScaleFactor into account for MouseRelatedEvents.

RESOLVED FIXED 67592

Take pageScaleFactor into account for MouseRelatedEvents.

https://bugs.webkit.org/show_bug.cgi?id=67592

Summary Take pageScaleFactor into account for MouseRelatedEvents.

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	Details Formatted Diff Diff
Patch (10.02 KB, patch) 2011-09-14 07:00 PDT, John Knottenbelt	no flags	Details Formatted Diff Diff
Patch (10.05 KB, patch) 2011-09-16 08:02 PDT, John Knottenbelt	no flags	Details Formatted Diff Diff
Patch (13.88 KB, patch) 2011-09-23 10:54 PDT, John Knottenbelt	no flags	Details Formatted Diff Diff
Patch (13.85 KB, patch) 2011-10-05 03:14 PDT, John Knottenbelt	no flags	Details Formatted Diff Diff
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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component New Bugs

Assignee

John Knottenbelt

Reported

2011-09-05 03:10 PDT

Modified

2011-10-06 03:09 PDT History

CC List

8 users Show

URL

Keywords

Depends on

67836

Blocks

68075

Dependencies

tree graph