1171073 - Scroll remains work after touch-action set to "none"

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Dmitriy Barkalov]

Attachment: touch-action-none-with-scroll.html

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.81 Safari/537.36

Steps to reproduce:

1. Open attached html in Nightly
2. Quick scroll textarea down the way that scroll will proceed after finger is up.
3. Give new push to scroll again in the same direction (while scrolling is not stopped)


Actual results:

On the 3rd step textarea have changed the background color (means that pointermove event rised) and speed up textarea scrolling.


Expected results:

I assume there could be two options:
1. If multiple touch scrolling is one action than on the 3rd step there should be pointercancel event and no pointermove event
2. Otherwise 3rd step should not speed up scrolling (like in touch-action: auto mode).

Comment 1

[Maksim Lebedev]

Some investigation:
> InputQueue::ReceiveTouchInput(...) {
> ...
>   if (aEvent.mType == MultiTouchInput::MULTITOUCH_START) {
> ...
>     haveBehaviors = CurrentTouchBlock()->GetAllowedTouchBehaviors(currentBehaviors);
> ...
>     block = StartNewTouchBlock(aTarget, aTargetConfirmed, false);
> ...
>     block->SetDuringFastFling();
> ...
>     block->SetAllowedTouchBehaviors(currentBehaviors);
> ...
>   }
> ...
>   if (block->IsDuringFastFling()) {
>     result = nsEventStatus_eConsumeNoDefault;
>   }
> ...
> }
In case second touchdown event (if previous event involved scrolling) we put state "FastFling" and get currentBehaviors from previous block (without request to real content). After that we return ConsumeNoDefault and scrolling continues work after such manipulations. (It looks like we move content as first touch event sequence).

Comment 2

[Maksim Lebedev]

Suggestion:
In such case we can get allowed behaviors from content (via sync request to TabChild) and after that check it and set "FastFling" mode (and continue scrolling) or stop "scrolling" (and provide obvious behavior) according with allowed behaviors.

Comment 3

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

It's not clear to me what the bug here is. What prefs do you have enabled - APZ and touch events? Or APZ and pointer events? Or all three?

(In reply to Dmitriy Barkalov from comment #0)
> Expected results:
> 
> I assume there could be two options:
> 1. If multiple touch scrolling is one action than on the 3rd step there
> should be pointercancel event and no pointermove event

This is not quite correct. If we're hitting the "FastFling" mode, then there should be no pointer event at all sent to content. The APZ should be consuming the events (and based on what Maksim said in comment 1 about it returning ConsumeNoDefault, that sounds like what is happening). Content will not get any pointer events for that second fling action, not even a pointercancel, because the fast-fling gesture is considered to be a single gesture in the APZ code.

I can kick off a windows build to try this out but it sounds to me like there isn't really a bug here.

Comment 4

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

Also, in the future please make sure to file any APZ bugs in the Core::Panning and Zooming component, rather than Firefox::Untriaged. That way the right people will see them faster. You can use the component picker at https://bugzilla.mozilla.org/enter_bug.cgi to do this.

Comment 5

[Maksim Lebedev]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> It's not clear to me what the bug here is. What prefs do you have enabled -
> APZ and touch events? Or APZ and pointer events? Or all three?
E10S(true)+APZ(true)+touchEvents(true)+PointerEvents(true)+touchAction(true)
> I can kick off a windows build to try this out but it sounds to me like
> there isn't really a bug here.
I suppose that it happens on all platforms, not only at windows build.
> Content will not get any pointer events for that second fling action, not even a
> pointercancel, because the fast-fling gesture is considered to be a single
> gesture in the APZ code.
In second case we have that touchAction has "none" value,
but after touchdown action we don't get any pointer events.
(We should get them according pointer events specification).

Comment 6

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

(In reply to Maksim Lebedev from comment #5)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> > It's not clear to me what the bug here is. What prefs do you have enabled -
> > APZ and touch events? Or APZ and pointer events? Or all three?
> E10S(true)+APZ(true)+touchEvents(true)+PointerEvents(true)+touchAction(true)

Thanks.

> > I can kick off a windows build to try this out but it sounds to me like
> > there isn't really a bug here.
> I suppose that it happens on all platforms, not only at windows build.

Yeah but windows is the only one with touch input support.

> > Content will not get any pointer events for that second fling action, not even a
> > pointercancel, because the fast-fling gesture is considered to be a single
> > gesture in the APZ code.
> In second case we have that touchAction has "none" value,
> but after touchdown action we don't get any pointer events.
> (We should get them according pointer events specification).

As far as I can tell the behaviour I'm getting on the test case is what I expect. When the frame is in fast-fling mode and you put your finger down to scroll it again, content doesn't get informed of that, because that block of input events just continues the gesture they already started. No touch events OR pointer events are dispatched to content for that block of input. AFAIK this behaviour is still spec-compatible, because the spec doesn't say that every user input must dispatch a pointer event. The stream of pointer events seen by content is still consistent, and that's all that matters. Closing as INVALID for now but if you disagree please feel free to argue your case.

Comment 7

[Dmitriy Barkalov]

(In reply to Maksim Lebedev from comment #5)
> E10S(true)+APZ(true)+touchEvents(true)+PointerEvents(true)+touchAction(true)
Thanks Maksim for covering me. The setup is right. Just mention that pan-zoom also need to be enabled.

> When the frame is in fast-fling mode and you put your finger down to
> scroll it again, content doesn't get informed of that, because that block of
> input events just continues the gesture they already started. No touch
> events OR pointer events are dispatched to content for that block of input.
I agree with this behaviour. But browser doesn't do this way. Please note that `pointermove` event fires (background becomes gray) during second move. But no events should be fired during second move and pointermove also.

So regarding the explanation of expected behaviour the expected result should be:
Background shouldn't change color on 3rd step. I.e. pointermove action shouldn't be recieved

Comment 8

[Dmitriy Barkalov]

Please provide you responce on my last comment and if you agree reopen the bug.

Comment 9

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

(In reply to Dmitriy Barkalov from comment #7)
> So regarding the explanation of expected behaviour the expected result
> should be:
> Background shouldn't change color on 3rd step. I.e. pointermove action
> shouldn't be recieved

I agree and when I followed the STR in comment 0 the background color did not change, as expected. Can you attach a video of what you are doing?

Comment 10

[Dmitriy Barkalov]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> Can you attach a video of what you are doing?
Yes but on Monday. It's too late in my timezone now.

I've tried my test in another device and at first I've got same result as you. But when I:
1. Removed my Mozilla profile (remove Mozilla folder from Users\<user>\AppData\Roaming). 
2. Set only APZ(true)+touchEvents(1) (all other setting a set by default in latest m-c version that I am using)

I was able to reproduce this bug.

To see the difference visually: scroll will become less sharper. On the old profile when I try to scroll and up my finger it stops immediately. On new I make a push and it is scrolling far.

Comment 11

[Dmitriy Barkalov]

Attachment: IMG_1840.MOV

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)
> Can you attach a video of what you are doing?

I've asked my colleague to reproduce the bug and record this video.

As you can see second touch (performed before scroll ends) paints scrollable area in gray (i.e. fires pointermove event).

Tip: Remove your Nightly profile before reproducing.

Comment 12

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

So interestingly I can reproduce what you are describing on nightly but not on a local build, both using clean profiles with APZ and touch events enabled. There must be some configuration difference between a nightly build and a local build that is responsible. It also looks like when the issue happens (at least for me) mouse events are getting generated from the touch input because i see text getting selected as i pan around. That should not be happening.

Comment 13

[Maksim Lebedev]

Some very interesting investigation:
1. This bug reproduced only at 64-bit builds! On 32-bit build current bug is not reproduced!
   Builds were got from https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6f96d09ae78
2. At first touch we can see that some of strings are become marked on 64-bit builds
   And after that we can see that such content is scrolled (with marked strings).
   On 32-bit builds we cannot mark any strings and scroll content without marked strings.
3. After moving and releasing touch, APZ goes to FLING mode (it can be seen at debug build).
   And after second touch, we can see that another(!) strings are become marked on 64-bit builds,
   and content become gray, and we still can scroll it (with ability to mark some strings).
   On 32-bit builds we can scroll only black content without ability to mark any strings.
My assumption: On 64-bit builds we see real content and scrolling real content (and thats why during scroll we still can mark some of string). And on 32-bit build we see APZ layer with picture, which is scrolled and on that picture we cannot mark any strings). Maybe on 64-bit builds APZ layer is situated under real content, and on 32-bit builds APZ layer is situated above real content.

Any suggestions? Any assumptions? Any comments?

Comment 14

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

Can you reproduce it with a local 64-bit build? If so you should just do that and stick a breakpoint in the mousemove handler and see why we are generating mousemove events for touch input.

Comment 15

[Maksim Lebedev]

I have downloaded build from try-server and I have tried to debug it. (It was made with debug symbols).
Unfortunately, a lot of variables is unavailable to see it, but I can see callstack and I can put breakpoints. After second touchdown and scrolling I can see that SelectionManager works after calling TabChild::RecvRealMouseButtonEvent(). It can be called from nsWindow::DispatchMouseEvent().
Looks like issue is situated at statement
> if (mTouchWindow && WinUtils::GetIsMouseFromTouch(aEventType)) {...}
At least during testing breakpoint at code
> MOZ_ASSERT(mAPZC);
have never caught working process.
So I can assume that WinUtils::GetIsMouseFromTouch() works incorrect at 64-bit platform.

Comment 16

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

Cool, thanks for investigating. It looks like a bug in GetIsMouseFromTouch on 64-bit then. I forget the rules for C++ promotion but it's possible that the MOZ* int variables in that function are getting promoted to 64-bit values and so the top 32bits will be set. Can you masking out those bits and seeing if it fixes the problem?

Comment 17

[Maksim Lebedev]

If I correctly understand assembler code, I can see that comparison has values 0xFFFFFFFFFFFFFF80 and 0xFFFFFFFFFF515780 but GetMessageExtraInfo() returns 0x00000000ff515788. That's why comparison returns FALSE as result.

Comment 18

[Maksim Lebedev]

Attachment: x64_scrolling_issue_ver1.diff

+ Changed  int  ->  uint32_t

Suggestions and comments and objections are very welcome.

Comment 19

[Maksim Lebedev]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1283417283e7

Comment 20

[Maksim Lebedev]

Attachment: x64_scrolling_issue_ver2.diff

+ Only changed comment for patch

Comment 21

[Maksim Lebedev]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dde5f511688

Comment 22

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

Comment on attachment 8626546 [details] [diff] [review]
x64_scrolling_issue_ver2.diff

Review of attachment 8626546 [details] [diff] [review]:
-----------------------------------------------------------------

You don't need to re-request review for that.

Comment 23

[Dmitriy Barkalov]

(In reply to Maksim Lebedev from comment #21)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dde5f511688

I've tested on x32 and x64 versions. Everything works fine. Thanks.

Comment 24

[Maksim Lebedev]

If there are no objections, I put checkin-needed flag.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3986f931721

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b3986f931721