1897345 - Figure out a way to handle INPUT_RESULT_IGNORED harmlessly

Status: NEW

(Core :: Panning and Zooming, defect, P2)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: t.html

While I was diagnosing bug 1895742, I found a test case causing "MOZ_ASSERT_UNREACHABLE: nsEventStatus_eConsumeNoDefault should involve a valid APZHandledResult" (bug 1852854) reliably. Attaching file is the test case, STR is just trying to scroll down not quickly.

What's going on there is that

1. there's an active touch-start event listener so that we wait for the result of the event listner
2. when the first touch-move event is received
   2-1) the APZEventResult for the touch-move event is initialized with mHandledResult = Nothing() since the target APZ is the root and the event needs to be dispatched to the content
   2-2) we fall into this if (block->UpdateSlopState(aEvent, consumable) block, thus the event result becomes nsEventStatus_eConsumeNoDefault

Thus the assertion happens.

So now I am pretty sure the assertion is invalid, it is a possible scenario.

A question here is that what the best way to handle the scenario is?

An easy way I can think of is to send the INPUT_RESULT_IGNORED state as it is (this is what we are currently doing) then ignore it in the android-component.

An alternative way is to let APZ not send the INPUT_RESULT_IGNORED state wait for more touch-move events until APZ escapes from in slop state, and once after APZ escaped from the state, send a proper InputResultDetail to the android-component.

I've confirmed locally the former way fixes bug 1895742, but bug 1895742 case is slightly different from the attached test case, in that bug case there's touch-action: none style so that INPUT_RESULT_IGNORED should not happen.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP: Bug 1897345 - Handle INPUT_RESULT_IGNORED in android-components.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Posted the former way.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Botond, what do you think about above two approaches? Do you have some other ideas?

Comment 4

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)

An alternative way is to let APZ not send the INPUT_RESULT_IGNORED state wait for more touch-move events until APZ escapes from in slop state, and once after APZ escaped from the state, send a proper InputResultDetail to the android-component.

I believe this is what we already do, in cases where the touch event listener runs within the content_response_timeout limit.

If the event status is eConsumeNoDefault, the event is not dispatched to Gecko. So, if we are waiting for a delayed InputResult, we should continue waiting until we get the "content received input block" callback, which should only happen after web content receives the first touchmove event (which requires that APZ does dispatch the touchmove event to Gecko).

I guess, in this testcase, since the event listener is waiting 1000ms (longer than the content_response_timeout limit), we time out the input block. The behaviour in that case should be the same as if we received a "content received input block" callback with preventDefault = false, so we should be able to produce a valid InputResult (which is not IGNORED).

Comment 5

[Botond Ballo [:botond]]

In the attached test case, we are not waiting for a delayed result. The touch-action: none allows us to send a HANDLED_CONTENT result eagerly, which is what I'm seeing.

Comment 6

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #5)

In the attached test case, we are not waiting for a delayed result. The touch-action: none allows us to send a HANDLED_CONTENT result eagerly, which is what I'm seeing.

I do intermittently see a subsequent result (for a touch-move event) of IGNORED.

I proposed in bug 1897567 that we stop asking for InputResults for touch-move events, so maybe that is sufficient to address this issue?

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: a more reduced case

No "touch-action:none" no 1000ms junk in the touchstart event listener.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to Botond Ballo [:botond] from comment #6)

(In reply to Botond Ballo [:botond] from comment #5)

In the attached test case, we are not waiting for a delayed result. The touch-action: none allows us to send a HANDLED_CONTENT result eagerly, which is what I'm seeing.

I do intermittently see a subsequent result (for a touch-move event) of IGNORED.

I proposed in bug 1897567 that we stop asking for InputResults for touch-move events, so maybe that is sufficient to address this issue?

The bug you meant is that we will have an additional assertion that the aInput needs to be a ACTION_DOWN in this branch? If so, it definitely fixes this assertion.

Comment 9

[Botond Ballo [:botond]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)

The bug you meant is that we will have an additional assertion that the aInput needs to be a ACTION_DOWN in this branch?

Ideally, in the entire function, if aReturnResult is not null (i.e. the caller asked of a result), the action should be ACTION_DOWN.