888365 - Use CSSIntPoint for client/page coordinates in events

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 1

[Kartikaya Gupta (email:[e-mail])]

This one makes me happy and sad. Happy because you did this, sad because I spent the last two days doing something very similar. I will upload my WIP so far.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: My current WIP

I started at the opposite end in the widget code and propagated from there. I'm not sure if all of this is correct. I'm more than happy to let Ms2ger's patch land first and rebase on top of that as it will make my patch much simpler.

I would still like feedback on my changes in nsIDOMTouchEvent.idl because the way nsIDOMTouch::mRefPoint was previously being used made very little sense to me.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 769024 [details] [diff] [review]
Patch v1


>   // If there is some scrolling, add scroll info to client point.
>   if (aPresContext && aPresContext->GetPresShell()) {
>     nsIPresShell* shell = aPresContext->GetPresShell();
>     nsIScrollableFrame* scrollframe = shell->GetRootScrollFrameAsScrollable();
>     if (scrollframe) {
>-      nsPoint pt = scrollframe->GetScrollPosition();
>-      pagePoint += nsIntPoint(nsPresContext::AppUnitsToIntCSSPixels(pt.x),
>-                              nsPresContext::AppUnitsToIntCSSPixels(pt.y));
>+      pagePoint += CSSIntPoint::FromAppUnitsRounded(scrollframe->GetScrollPosition());
scrollframe->GetScrollPositionCSSPixels() ?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 769024 [details] [diff] [review]
> Patch v1
> 
> 
> >   // If there is some scrolling, add scroll info to client point.
> >   if (aPresContext && aPresContext->GetPresShell()) {
> >     nsIPresShell* shell = aPresContext->GetPresShell();
> >     nsIScrollableFrame* scrollframe = shell->GetRootScrollFrameAsScrollable();
> >     if (scrollframe) {
> >-      nsPoint pt = scrollframe->GetScrollPosition();
> >-      pagePoint += nsIntPoint(nsPresContext::AppUnitsToIntCSSPixels(pt.x),
> >-                              nsPresContext::AppUnitsToIntCSSPixels(pt.y));
> >+      pagePoint += CSSIntPoint::FromAppUnitsRounded(scrollframe->GetScrollPosition());
> scrollframe->GetScrollPositionCSSPixels() ?

Makes sense, but GetScrollPositionCSSPixels currently returns nsIntPoint, so I'll do that in another bug.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Ms2ger, are you landing this?

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 769087 [details] [diff] [review]
My current WIP

I'm gonna obsolete this WIP. I have figured out some things since then and from talking to smaug on IRC and have a better idea of what needs to be done.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/2a2adc41af9b

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Are you working on the refpoint stuff?

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Not at the moment. If you want it you're welcome to have it.