1687430 - Introduce a new INPUT_RESULT to represent nsEventStatus_eConsumeNoDefault

Status: RESOLVED FIXED

(GeckoView :: IME, enhancement)

Description

[Hiroyuki Ikezoe (:hiro)]

In my understandings, nsEventStatus_eConsumeNoDefault is representing that the event was consumed by either the root APZC or a non-root APZC but browsers should do nothing in response to the event.

Botond suggested in bug 1676814 comment 4 to use INPUT_RESULT_UNHANDLED but I know AC uses INPUT_RESULT_UNHANDLED to restore the toolbar state, so we should introduce a new flag which we haven't used anywhere.

The Botond's comment from bug 1676814 comment 4 for references;
(In reply to Botond Ballo [:botond] from comment #4)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
The reason that combining mHandledResult == Nothing() with mStatus == eConsumeNoDefault doesn't make sense, is that eConsumeNoDefault means the event will not be dispatched to Gecko, so there will never be a delayed result.

So, in a case like this, we should fill in an mHandledResult value corresponding to what behaviour we want. I think in this case we may want Unhandled, as we probably don't want to combine overscroll with dynamic toolbar movement.

Without introducing the new result, bug 1678505 will be quite messy because currently we return either INPUT_RESULT_HANDLED or INPUT_RESULED_HANDLED_CONTENT depending on the initial target APZC, but I've found there is a case where we are not sure about the target APZC, here and I have no idea how we should treat the case anyway.

Though, I have no great idea about the name right now, INPUT_RESULT_SHOULD_BE_IGNORED? If it's long INPUT_RESULT_CONSUMED or some such?

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1687430 - Introduce INPUT_RESULT_IGNORED to represent nsEventStatus_eConsumeNoDefault. r?botond!,#geckoview-reviewers!

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cca8be923dd7
Introduce INPUT_RESULT_IGNORED to represent nsEventStatus_eConsumeNoDefault. r=botond,geckoview-reviewers,agi

Comment 3

[Alexandru Michis [:malexandru]]

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

Comment 4

[Petru-Mugurel Lingurar [:petru]]

I've created https://github.com/mozilla-mobile/android-components/issues/9480 to handle the new return value on AC/Fenix.
As a quick fix I've just mapped this new value to what we used as INPUT_RESULT_HANDLED which in manual testing seems to result in Fenix keeping the old behavior.
I'm wondering if we should change any behavior related to the dynamic toolbar / pull to refresh now in Fenix or wait for the bigger refactoring following #1678505.
Hiro, can you please advise?

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I was expecting AC and Fenix wouldn't need to be changed for this change.

From the github issue;

If the new event would be mapped to INPUT_RESULT_UNHANDLED then

a fling up gesture would show the toolbar everytime while
a swipe up gesture would keep the bottom toolbar hidden

This is wrong. If the crash happens unless INPUT_RESULD_IGNORED is not mapped to something, then mapping it to a new flag would be the right solution I believe. That said, it's not clear to me why the crash happens, I'd expect INPUT_RESULT_IGNORED will never be appeared in AC (or Fenix) code base.

Comment 6

[Petru-Mugurel Lingurar [:petru]]

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

I was expecting AC and Fenix wouldn't need to be changed for this change.
This is wrong. If the crash happens unless INPUT_RESULD_IGNORED is not mapped to something, then mapping it to a new flag would be the right solution I believe. That said, it's not clear to me why the crash happens, I'd expect INPUT_RESULT_IGNORED will never be appeared in AC (or Fenix) code base.

AC would crash on because of this row [1] where we would try to map the integer GV returns to an enum instance. Because there is no enum right now for the new integer value the mapping fails.
This is by design, exactly to catch these kind of situations and have the chance to discuss the changes rather than having the app ignore input returns it does not know about and so offer different behavior without our knowledge.
Mapping the INPUT_RESULD_IGNORED value to an existing AC - InputResult is just a temporary solution, a minimal change to hopefully keep the current behavior.

But for the future I wanted to ask you what should we do with this new value (that we receive here [2] and store it internally for the dynamic toolbar, pull to refresh to check against, to know if they should react or not)?:

• if you don't expect us to receive it should we just drop it (not overwrite whatever inputResult we currently have)?
• if we map it to a new AC flag, how should the app treat this?
  We know for example the dynamic toolbar should be animated only for INPUT_RESULT_HANDLED.
  But seeing this new INPUT_RESULD_IGNORED gets returned for fling gestures based on which GV scrolls the webpage I feel like in this case also the dynamic toolbar should be animated but the pull to refresh should be cancelled & should not appear for down flings)

---

[1] https://github.com/mozilla-mobile/android-components/blob/GV-Nightly-85.0.20201214091023/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineView.kt#L175
[2] https://github.com/mozilla-mobile/android-components/blob/master/components/browser/engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/NestedGeckoView.kt#L122-L128

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to Petru-Mugurel Lingurar [:petru] from comment #6)

But for the future I wanted to ask you what should we do with this new value (that we receive here [2] and store it internally for the dynamic toolbar, pull to refresh to check against, to know if they should react or not)?:

• if you don't expect us to receive it should we just drop it (not overwrite whatever inputResult we currently have)?

Yep, I think AC or Fenix can drop INPUT_RESULT_IGNORED.

• if we map it to a new AC flag, how should the app treat this?

If AC wants to map a new value, the app should do nothing in response to the new value.

We know for example the dynamic toolbar should be animated only for INPUT_RESULT_HANDLED.
But seeing this new INPUT_RESULD_IGNORED gets returned for fling gestures based on which GV scrolls the webpage I feel like in this case also the dynamic toolbar should be animated but the pull to refresh should be cancelled & should not appear for down flings)

It sounds weird a pull-to-refresh has already been triggered during fast flings. So for example, INPUT_RESULT_IGNORED is returned when user starts touching the screen (i.e. touchstart event in terms of DOM) during the previous scrolling is still on-going, to me there is no chance to trigger pull-to-refresh in the case because the previous scroll is on-going means there's room to scroll, thus pull-to-refresh should not be triggered at that time.

A possible case I can think of that we might need to handle INPUT_RESULT_IGNORE is, if user tried to scroll down then the user tries to scroll up during the first scrolling down is on-going, we probably need to change the dynamic toolbar height. To fix this case, it's bit unfortunate because we need to know which direction user tried to scroll, which means we need to call onTouchEventForResult in cases of ACTION_MOVEs too, that's we want to avoid.

Comment 8

[Petru-Mugurel Lingurar [:petru]]

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

Yep, I think AC or Fenix can drop INPUT_RESULT_IGNORED.
If AC wants to map a new value, the app should do nothing in response to the new value.

Thank you!
Then to not complicate things I'll just drop this value.

It sounds weird a pull-to-refresh has already been triggered during fast flings. So for example, INPUT_RESULT_IGNORED is returned when user starts touching the screen (i.e. touchstart event in terms of DOM) during the previous scrolling is still on-going, to me there is no chance to trigger pull-to-refresh in the case because the previous scroll is on-going means there's room to scroll, thus pull-to-refresh should not be triggered at that time.

Indeed. Don't think the INPUT_RESULT_IGNORED would be an issue for pull to refresh.
Pull to refresh has a nasty bug - https://github.com/mozilla-mobile/fenix/issues/16577 because it currently does not check where in the webpage the user started the touch event. So with the page not scrolled to the top, if the user does a continuous scroll down the page will be scrolled to the top and then if the gesture continues the throbber will appear.
Don't think INPUT_RESULT_IGNORED will change anything related to this though but hoped bug #1678505 would help with.

A possible case I can think of that we might need to handle INPUT_RESULT_IGNORE is, if user tried to scroll down then the user tries to scroll up during the first scrolling down is on-going, we probably need to change the dynamic toolbar height. To fix this case, it's bit unfortunate because we need to know which direction user tried to scroll, which means we need to call onTouchEventForResult in cases of ACTION_MOVEs too, that's we want to avoid.

In this case wouldn't we loose just one opportunity to animate the toolbar? The scroll up would eventually mean a different inputResult based on which the toolbar will be animated as expected and should be something transparent to the user.

Also, thank you for your support on this!

Comment 9

[Hiroyuki Ikezoe (:hiro)]

(In reply to Petru-Mugurel Lingurar [:petru] from comment #8)

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

Yep, I think AC or Fenix can drop INPUT_RESULT_IGNORED.
A possible case I can think of that we might need to handle INPUT_RESULT_IGNORE is, if user tried to scroll down then the user tries to scroll up during the first scrolling down is on-going, we probably need to change the dynamic toolbar height. To fix this case, it's bit unfortunate because we need to know which direction user tried to scroll, which means we need to call onTouchEventForResult in cases of ACTION_MOVEs too, that's we want to avoid.

In this case wouldn't we loose just one opportunity to animate the toolbar? The scroll up would eventually mean a different inputResult based on which the toolbar will be animated as expected and should be something transparent to the user.

After more thought on this case, I am pretty sure dropping INPUT_RESULT_IGNORED is the right thing to do, which means keeping the previous result.

Comment 10

[Chris Peterson [:cpeterson]]

Moving some input bugs to the new GeckoView::IME component.