Fix mouse pointer event clientX/Y

Fix mouse pointer event clientX/Y

Before this change the clientX/Y was set from
root frame coordinates. This CL transform such
coordinates to frame coordinates for setting
clientX/Y.

BUG=605676
Committed: https://crrev.com/85ece81cc3561eb24ecc96f25d7ae9ecd40c219f
Cr-Commit-Position: refs/heads/master@{#393247}

[Navid Zolghadr, 2016-05-09 17:09:12]

nzolghadr@chromium.org changed reviewers:
+ mustaq@chromium.org

[Navid Zolghadr, 2016-05-09 17:09:12]

Mustaq, is that what you had in mind?

One alternate approach I had in mind, was changing the other constructor path in
MouseRelatedEvent but it seems that there are a lot of other places using that
same path.

Also in the original MouseRelatedEvent constructor

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

there was a check for position of the event which we don't seem to have it here
in pointer events. Do you that is something we are missing or what?

[mustaq, 2016-05-09 18:22:51]

Thanks, this is what I meant. We need to test different scroll positions---I
commented that in crrev.com/1964523002 because this is true for touch as well.

https://codereview.chromium.org/1960233002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1960233002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:108: if (frame) {
The original code in MouseRelatedEvent() seems to have a null check here. May
be:
  if(view && (frame = view->frame()))?

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[Navid Zolghadr, 2016-05-10 21:56:07]

nzolghadr@chromium.org changed reviewers:
+ bokan@chromium.org, dtapuska@chromium.org

[Navid Zolghadr, 2016-05-10 21:56:08]

bokan@, in this change I kind of did the same thing regarding zooming and scroll
offset calculation for mouse pointer events. Not sure if this is the ideal
scenario that you had in mind. Can you have a look and see whether you have a
better place of code in mind to put this kind of logic in?

[bokan, 2016-05-10 22:24:09]

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:109: if
(FrameView* frameView = frame->view()) {
Fold this check into the above one.

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:111:
adjustedPageLocation = frameView->rootFrameToContents(mouseEvent.position());
s/adjustedPageLocation/locationInContents

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:113: if
(scaleFactor != 1.0f) {
No need for the check here, just allow scaling by 1 (which should no-op).

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:119: LayoutPoint
clientPoint = adjustedPageLocation - toLayoutSize(scrollPosition);
I would do this like so:

locationInContents = frameView->rootFrameToContents(mouseEvent.position());
locationInFrameZoomed = frameView->contentsToFrame(locationInContents);
float scaleFactor = 1 / frame->pageZoomFactor();
locationInFrameZoomed.scale(scaleFactor, scaleFactor)

[blink-reviews, 2016-05-10 22:40:23]

Oh, sorry, didn't realize I still hadn't stamped it. I will take a look now.

PS. If you're waiting on a review for more than 24hrs, feel free to
message/email a reviewer directly.

On Tue, May 10, 2016 at 5:56 PM, <nzolghadr@chromium.org> wrote:

> bokan@, in this change I kind of did the same thing regarding zooming and
> scroll
> offset calculation for mouse pointer events. Not sure if this is the ideal
> scenario that you had in mind. Can you have a look and see whether you
> have a
> better place of code in mind to put this kind of logic in?
>
>
> https://codereview.chromium.org/1960233002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-05-10 22:40:24]

Oh, sorry, didn't realize I still hadn't stamped it. I will take a look now.

PS. If you're waiting on a review for more than 24hrs, feel free to
message/email a reviewer directly.

On Tue, May 10, 2016 at 5:56 PM, <nzolghadr@chromium.org> wrote:

> bokan@, in this change I kind of did the same thing regarding zooming and
> scroll
> offset calculation for mouse pointer events. Not sure if this is the ideal
> scenario that you had in mind. Can you have a look and see whether you
> have a
> better place of code in mind to put this kind of logic in?
>
>
> https://codereview.chromium.org/1960233002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blink-reviews, 2016-05-10 22:58:36]

Whoops ^^^ Ignore that, replied to the wrong email.


On Tue, May 10, 2016 at 6:40 PM, David Bokan <bokan@google.com> wrote:

> Oh, sorry, didn't realize I still hadn't stamped it. I will take a look
> now.
>
> PS. If you're waiting on a review for more than 24hrs, feel free to
> message/email a reviewer directly.
>
> On Tue, May 10, 2016 at 5:56 PM, <nzolghadr@chromium.org> wrote:
>
>> bokan@, in this change I kind of did the same thing regarding zooming
>> and scroll
>> offset calculation for mouse pointer events. Not sure if this is the ideal
>> scenario that you had in mind. Can you have a look and see whether you
>> have a
>> better place of code in mind to put this kind of logic in?
>>
>>
>> https://codereview.chromium.org/1960233002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-05-10 22:58:36]

Whoops ^^^ Ignore that, replied to the wrong email.


On Tue, May 10, 2016 at 6:40 PM, David Bokan <bokan@google.com> wrote:

> Oh, sorry, didn't realize I still hadn't stamped it. I will take a look
> now.
>
> PS. If you're waiting on a review for more than 24hrs, feel free to
> message/email a reviewer directly.
>
> On Tue, May 10, 2016 at 5:56 PM, <nzolghadr@chromium.org> wrote:
>
>> bokan@, in this change I kind of did the same thing regarding zooming
>> and scroll
>> offset calculation for mouse pointer events. Not sure if this is the ideal
>> scenario that you had in mind. Can you have a look and see whether you
>> have a
>> better place of code in mind to put this kind of logic in?
>>
>>
>> https://codereview.chromium.org/1960233002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mustaq, 2016-05-11 14:02:17]

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html
(right):

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html:35:
'clientY'
Looks like view property is never checked anywhere. Could you please add it
here, and also in {mouse,touch}-pointer-event-properties.html to cover the main
frame case?

[Navid Zolghadr, 2016-05-11 18:09:44]

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html
(right):

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/pointerevent/fast/events/pointerevents/pointer-event-properties-in-iframe.html:35:
'clientY'
On 2016/05/11 14:02:17, mustaq wrote:
> Looks like view property is never checked anywhere. Could you please add it
> here, and also in {mouse,touch}-pointer-event-properties.html to cover the
main
> frame case?

As we discussed offline, I'll do it in another change as I need to change some
tests that are not quite related to this change anyway.

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/events/PointerEventFactory.cpp (right):

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:109: if
(FrameView* frameView = frame->view()) {
On 2016/05/10 22:24:09, bokan wrote:
> Fold this check into the above one.

Done.

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:111:
adjustedPageLocation = frameView->rootFrameToContents(mouseEvent.position());
On 2016/05/10 22:24:09, bokan wrote:
> s/adjustedPageLocation/locationInContents

Done.

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:113: if
(scaleFactor != 1.0f) {
On 2016/05/10 22:24:09, bokan wrote:
> No need for the check here, just allow scaling by 1 (which should no-op).

Done.

https://codereview.chromium.org/1960233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/events/PointerEventFactory.cpp:119: LayoutPoint
clientPoint = adjustedPageLocation - toLayoutSize(scrollPosition);
On 2016/05/10 22:24:09, bokan wrote:
> I would do this like so:
> 
> locationInContents = frameView->rootFrameToContents(mouseEvent.position());
> locationInFrameZoomed = frameView->contentsToFrame(locationInContents);
> float scaleFactor = 1 / frame->pageZoomFactor();
> locationInFrameZoomed.scale(scaleFactor, scaleFactor)

Done.

[Navid Zolghadr, 2016-05-11 18:09:48]

ptal.

[bokan, 2016-05-11 18:22:25]

lgtm

[mustaq, 2016-05-12 14:06:03]

lgtm

[Navid Zolghadr, 2016-05-12 14:40:44]

The CQ bit was checked by nzolghadr@chromium.org

[commit-bot: I haz the power, 2016-05-12 14:41:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1960233002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1960233002/100001

[commit-bot: I haz the power, 2016-05-12 14:48:03]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-05-12 14:50:16]

Description was changed from

==========
Fix mouse pointer event clientX/Y

Before this change the clientX/Y was set from
root frame coordinates. This CL transform such
coordinates to frame coordinates for setting
clientX/Y.

BUG=605676
==========

to

==========
Fix mouse pointer event clientX/Y

Before this change the clientX/Y was set from
root frame coordinates. This CL transform such
coordinates to frame coordinates for setting
clientX/Y.

BUG=605676

Committed: https://crrev.com/85ece81cc3561eb24ecc96f25d7ae9ecd40c219f
Cr-Commit-Position: refs/heads/master@{#393247}
==========

[commit-bot: I haz the power, 2016-05-12 14:50:17]

Patchset 6 (id:??) landed as
https://crrev.com/85ece81cc3561eb24ecc96f25d7ae9ecd40c219f
Cr-Commit-Position: refs/heads/master@{#393247}