Bug 67592 - Take pageScaleFactor into account for MouseRelatedEvents.
Summary: Take pageScaleFactor into account for MouseRelatedEvents.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Knottenbelt
URL:
Keywords:
Depends on: 67836
Blocks: 68075
  Show dependency treegraph
 
Reported: 2011-09-05 03:10 PDT by John Knottenbelt
Modified: 2011-10-06 03:09 PDT (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description John Knottenbelt 2011-09-05 03:10:57 PDT
Take pageScaleFactor into account for MouseRelatedEvents.
Comment 1 John Knottenbelt 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.
Comment 2 John Knottenbelt 2011-09-05 03:18:56 PDT
Created attachment 106317 [details]
Patch
Comment 3 WebKit Review Bot 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>
Comment 4 WebKit Review Bot 2011-09-05 09:55:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Simon Fraser (smfr) 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.
Comment 6 John Knottenbelt 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.
Comment 7 Simon Fraser (smfr) 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.
Comment 8 John Knottenbelt 2011-09-14 06:58:49 PDT
Reopening as my original fix had problems.
Comment 9 John Knottenbelt 2011-09-14 07:00:35 PDT
Created attachment 107327 [details]
Patch
Comment 10 John Knottenbelt 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.
Comment 11 WebKit Review Bot 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
Comment 12 Simon Fraser (smfr) 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
Comment 13 John Knottenbelt 2011-09-16 08:02:29 PDT
Created attachment 107653 [details]
Patch
Comment 14 John Knottenbelt 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.
Comment 15 Simon Fraser (smfr) 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>
Comment 16 John Knottenbelt 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.
Comment 17 Simon Fraser (smfr) 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.
Comment 18 Darin Adler 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.
Comment 19 John Knottenbelt 2011-09-23 10:54:49 PDT
Created attachment 108494 [details]
Patch
Comment 20 John Knottenbelt 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.
Comment 21 John Knottenbelt 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).
Comment 22 Simon Fraser (smfr) 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.
Comment 23 John Knottenbelt 2011-10-05 03:14:29 PDT
Created attachment 109768 [details]
Patch
Comment 24 Tony Gentilcore 2011-10-06 02:03:30 PDT
Comment on attachment 109768 [details]
Patch

Carrying forward Simon's r+
Comment 25 WebKit Review Bot 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>
Comment 26 WebKit Review Bot 2011-10-06 03:09:21 PDT
All reviewed patches have been landed.  Closing bug.