1037066 - [Dialer] Keypad background color changes are delayed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect, P3)

Description

[Doug Sherk (:drs) (inactive)]

When tapping on the keypad, the background color doesn't change immediately. I suspect that this is a problem with APZC's 300ms delay, but I need to investigate more.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: WIP

This fixes the issue, but it doesn't quite make sense in my head. My memory of APZC's touch event handling is a bit cloudy. I'd like to come back to this when I can fully explain it.

Comment 2

[Anthony Ricaud (:rik)]

In the perfect situation, the keypad should be a non-scrollable area. So when touching a non-scrollable/non-zoomable area, Gecko should apply the hover styles directly. Maybe we broke that by making the keypad scrollable or maybe Gecko isn't doing a shortcut for this case?

Comment 3

[Doug Sherk (:drs) (inactive)]

This delay is being caused by the touch event delay which is induced when content sets touch event listeners. I don't think this has anything to do with the content being scrollable/unscrollable as it's a result of interactions between the main process <-> content process. The WIP works by not setting any touch event listeners, and instead only setting mouse event listeners, which don't trigger the delay code path. What's puzzling to me is, as you said, why it's affecting the hover styles. I'm going to look more into this before I ask for input from the other APZ guys.

Comment 4

[Doug Sherk (:drs) (inactive)]

Jon, we use the ASSIGNED status differently within dialer. I haven't begun work on it yet, so I haven't set it to ASSIGNED. But I was going to start soon, so it's ok.

Comment 5

[Doug Sherk (:drs) (inactive)]

Some more info on this:

* As shown in attachment 8453929 [details] [diff] [review], switching the touch listeners to mouse listeners fixes this issue.
* I can also resolve this by switching .keypad-button:active to .keypad-button.active and adding/removing the .active class in the touchstart/touchend handler.
* Using the :hover selector instead seems to make it respond faster, but incorrectly (since the invisible cursor stays over that key).
* If I print out |event.target.parentNode.querySelector(':active')| in the touchstart and touchend handlers, it's unset during touchstart and touchend. Though this may be expected.

Maybe APZ is waiting for the touch listeners to respond since :active shouldn't be applied to the element if it gets preventDefault()'d?

Comment 6

[Doug Sherk (:drs) (inactive)]

I put together a reduced test case here:
http://people.mozilla.org/~dsherk/touch-listeners-active.html

If you open this in the B2G browser and tap on the red box, it instantly switches to blue. If you enable touch listeners and then tap on it again, it takes a moment for the background to change.

Kats, any suggestions? This kind of makes sense if you need to wait for content to preventDefault() or not preventDefault() the event before setting the :active status, but is there a clean way around this (maybe one of my tests in comment 5)? Is there a use case where we actually need to wait for content?

Comment 7

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

The active state is set from [1] if we know the thing is not scrollable (that's what the aArg indicates). The thing is, that APZStateChange notification only comes after we process the touch-start event in the APZ [2], which waits for the prevent-default notification or timeout.

The first question I have to check if this is even feasible is: if the content DOES do a prevent-default on the touch-start, do we still want the element to be :active-ated? If we don't, then we try moving the call at [1] to somewhere earlier in the flow. However in that case we might have imperfect information about whether the element is scrollable.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp?rev=ac8248c5b891#1905
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?rev=93ac908b09ab#998

Comment 8

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

Also CC'ing botond since he wrote the ActiveElementManager code and might have some thoughts on this.

Comment 9

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> The first question I have to check if this is even feasible is: if the
> content DOES do a prevent-default on the touch-start, do we still want the
> element to be :active-ated? If we don't, then we try moving the call at [1]
> to somewhere earlier in the flow. However in that case we might have
> imperfect information about whether the element is scrollable.

I just checked how this works on Fennec and Chrome for Android.

Fennec:
* preventDefault - it gets stuck blue (yikes).
* No preventDefault - seems to be the same speed as no touch listeners.

Chrome for Android:
* preventDefault - stays red, never turns blue.
* No preventDefault - seems to be the same speed as no touch listeners.

I think the reason we wouldn't :active-ate the element when preventDefaulted is because querySelector(':active') would false-positive on this element. So it seems to me like it's better to fix this in Gaia. Unfortunately, I think this is the best way to do it:

(In reply to Doug Sherk (:drs) from comment #5)
> * I can also resolve this by switching .keypad-button:active to
> .keypad-button.active and adding/removing the .active class in the
> touchstart/touchend handler.

I'm open to suggestions, though.

Comment 10

[Doug Sherk (:drs) (inactive)]

Botond, before I get started on a Gaia patch, do you have any thoughts on this?

Comment 11

[Doug Sherk (:drs) (inactive)]

Ok, wait. Why does there have to be any delay? The main process shouldn't care at all about the :active state of elements in content processes. Can this not be handled entirely within the content process?

Comment 12

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

(In reply to Doug Sherk (:drs) from comment #11)
> Ok, wait. Why does there have to be any delay? The main process shouldn't
> care at all about the :active state of elements in content processes. Can
> this not be handled entirely within the content process?

I'm not sure exactly what you mean here. The content process doesn't know if the APZ can scroll the element or not, and that information is one of the inputs to the decision as to whether or not to make it :active right away.

Comment 13

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> I'm not sure exactly what you mean here. The content process doesn't know if
> the APZ can scroll the element or not, and that information is one of the
> inputs to the decision as to whether or not to make it :active right away.

But in the case of the keypad, neither the key element or anything else currently visible is scrollable.

Comment 14

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

Yes, but how is the content process supposed to know that?

Comment 15

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> Yes, but how is the content process supposed to know that?

The main process could sync this with the content process and notify it of any changes after NotifyLayersUpdated. You would only be able to know whether or not anything currently visible is scrollable, but it would be better than nothing.

Comment 16

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

I'd rather wait for Botond to weigh in. It feels like we're going down a rabbithole that I don't think is even a solution the problem we're looking at.

Comment 17

[Botond Ballo [:botond]]

What is the expected behaviour of the :active state when content prevent-defaults a touch event? Is it reasonable to make the target element :active as soon as the finger goes down, and then remove the :active if a prevent-default occurs?

If so, perhaps we can move the APZStateChange::StartTouch notification earlier, i.e. do it in APZC::ReceiveInputEvent() rather than APZC::HandleInputEvent(). If content then prevent-defaults the touch, we can have TabChild pretend it got an EndTouch notification.

Comment 18

[Doug Sherk (:drs) (inactive)]

(In reply to Botond Ballo [:botond] from comment #17)
> What is the expected behaviour of the :active state when content
> prevent-defaults a touch event? Is it reasonable to make the target element
> :active as soon as the finger goes down, and then remove the :active if a
> prevent-default occurs?
> 
> If so, perhaps we can move the APZStateChange::StartTouch notification
> earlier, i.e. do it in APZC::ReceiveInputEvent() rather than
> APZC::HandleInputEvent(). If content then prevent-defaults the touch, we can
> have TabChild pretend it got an EndTouch notification.

That seems reasonable to me, but you could still get a false-positive on querySelector(':active') this way. I'm inclined to just fix this in Gaia or by my proposal in comment 15, so I'm going to work on a Gaia patch for now.

Comment 19

[Doug Sherk (:drs) (inactive)]

Attachment: Set .active class on keypad keys in touch listeners instead of using :active.

PR: https://github.com/mozilla-b2g/gaia/pull/22427

If we decide to do anything within APZ, we can file another bug for that. I'll leave that to Kats and Botond to decide.

Comment 20

[Botond Ballo [:botond]]

(In reply to Doug Sherk (:drs) from comment #19)
> If we decide to do anything within APZ, we can file another bug for that.
> I'll leave that to Kats and Botond to decide.

I investigated this a bit and discussed it with Kats and Doug. Summarizing:

The time required for content to send a prevent-default=no notification to APZ, and APZ to send back the APZStateChange::StartTouch message that triggers the :active state, is not the problem. This typically takes just a few milliseconds.

The problematic case is when you hold your finger down but do not move it, so that a touch-start is generated but not a touch-move. Since content is allowed to prevent-default the touch on the first touch-move as well as on the touch-start, it does not send the prevent-default=no message until the first touch-move is received and that has not been prevent-defaulted. Thus, in the case where there is no touch-move, a prevent-default=no message is never sent, and APZ waits for the full 300 ms before processing the touch block and sending the StartTouch message.

Doug's suggestion in comment 15 would not solve this problem - content would have to wait for the touch-move that never arrives just as much as APZ does, or else risk getting a false-positive for querySelector(':active') as mentioned in comment 18.

Instead, Kats suggested getting widget code, such as nsAppShell, to generate a touch-move event immediately after a touch-start, with the same coordinates as the touch-start. The gesture detection code can handle this gracefully (i.e. it won't mistakenly treat it as a pan gesture, before there is a tolerance threshold for that), but it would ensure that any touch-move listener on the content side that wants to prevent-default the touch block does so right away.

Comment 21

[Doug Sherk (:drs) (inactive)]

(In reply to Botond Ballo [:botond] from comment #20)
> Instead, Kats suggested getting widget code, such as nsAppShell, to generate
> a touch-move event immediately after a touch-start, with the same
> coordinates as the touch-start. The gesture detection code can handle this
> gracefully (i.e. it won't mistakenly treat it as a pan gesture, before there
> is a tolerance threshold for that), but it would ensure that any touch-move
> listener on the content side that wants to prevent-default the touch block
> does so right away.

Thanks for investigating this more. I like this idea. Do you have any plans to do this? I would rather fix the issue properly this way. If you don't have the cycles, I can probably do it instead. However, I suspect that there may be some fallout from this change, and I'm not sure I'll be able to handle it all.

Comment 22

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

I don't have any plans to do this in the immediate future. Feel free to try it and back it out if the fallout is too much. At least we'll know what kind of problems crop up. For the record I don't imagine there would be that much fallout from it.

Comment 23

[Anthony Ricaud (:rik)]

Comment on attachment 8466381 [details] [diff] [review]
Set .active class on keypad keys in touch listeners instead of using :active.

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

Quick review in case we want to take the Gaia path to fix this.

Let's aim to fix it in 2.1 (that is before FL). If we have a good Gecko patch that sticks, we'll take it but we have this Gaia patch as a fallback.

::: shared/js/dialer/keypad.js
@@ +15,5 @@
>    '*': [941, 1209], '0': [941, 1336], '#': [941, 1477]
>  };
>  
> +// Artificially limit the maximum amount of digits entered to 50.
> +var gMaxDigits = 50;

Instead of a global, this should be a property on KeypadManager.

@@ +520,5 @@
>      event.stopPropagation();
>  
>      switch (event.type) {
>        case 'touchstart':
> +        if (this._phoneNumber.length < gMaxDigits - 1) {

With that check, I think keys will never get highlighted after 50 digits while being on a call.

Comment 24

[Doug Sherk (:drs) (inactive)]

Attachment: Simulate a touchmove in Gonk immediately after a touchstart in the same location.

This fixes the problem and doesn't seem to have caused any new problems. Should we get mwu to review this as well? I'll post a try push when the tree reopens.

Comment 25

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

Comment on attachment 8472029 [details] [diff] [review]
Simulate a touchmove in Gonk immediately after a touchstart in the same location.

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

mwu will also have to review the patch. Flagging him for feedback now (even though the patch is wrong) in case he has objections that invalidate this approach entirely.

::: widget/gonk/nsAppShell.cpp
@@ +771,5 @@
>              if (captured) {
>                  return;
>              }
> +
> +            uint32_t touchStartBits = AMOTION_EVENT_ACTION_DOWN|AMOTION_EVENT_ACTION_POINTER_DOWN;

These aren't bitflags, you can't | them like this. POINTER_DOWN has a value of 5 which will match both ACTION_UP and ACTION_OUTSIDE. Also, this entire block should move up to before the if (captured) condition above.

Comment 26

[Doug Sherk (:drs) (inactive)]

Attachment: Simulate a touchmove in Gonk immediately after a touchstart in the same location.

Thanks, here it is with those fixes.

This also raises another question: should we simulate a mousemove for the simulated touchmove? The spec isn't specific about this:
http://www.w3.org/TR/touch-events/#mouse-events

I would say no, since this is meant to solve a problem with touch events only.

Comment 27

[Doug Sherk (:drs) (inactive)]

Attachment: Simulate a touchmove in Gonk immediately after a touchstart in the same location.

Thanks, here it is with those fixes.

This also raises another question: should we simulate a mousemove for the simulated touchmove? The spec isn't specific about this:
http://www.w3.org/TR/touch-events/#mouse-events

I would say no, since this is meant to solve a problem with touch events only.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=79d6bea9fbcb

Comment 28

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

Comment on attachment 8472324 [details] [diff] [review]
Simulate a touchmove in Gonk immediately after a touchstart in the same location.

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

I'm not sure about the mouse event behaviour - might want to check with smaug on that. Also that made me realize this patch will cause events to be dispatched in the order "touchstart,touchmove,mousedown,..." whereas before the mousedown would immediately follow the touchstart. I'm not sure if that matters or not - again smaug would know.

::: widget/gonk/nsAppShell.cpp
@@ +771,5 @@
> +
> +            if (action == AMOTION_EVENT_ACTION_DOWN ||
> +                action == AMOTION_EVENT_ACTION_POINTER_DOWN) {
> +                // Bug 1037066: Simulate a touchmove event at the same spot as
> +                // the touchstart immediately after it. This allows content to

s/after/before/. Or s/touchstart immediately/touchstart, but immediately/

Comment 29

[Doug Sherk (:drs) (inactive)]

Olli, please see comment 27 and comment 28. Are there any problems you can foresee with this behavior?

Should we re-arrange this such that the ordering is "touchstart, mousedown, simulated touchmove, (simulated mousemove)?, ..."?

Comment 30

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

Comment on attachment 8472324 [details] [diff] [review]
Simulate a touchmove in Gonk immediately after a touchstart in the same location.

I'd prefer keeping the old ordering unless there is some strong
reason to change it.
So couldn't we handle this like there was a
touch move (asynchronously) right after touch down?
That would mean touchstart related events would happen in the order they
happen now, and after those we had the simulated move.

Comment 31

[Doug Sherk (:drs) (inactive)]

(In reply to Olli Pettay [:smaug] from comment #30)
> I'd prefer keeping the old ordering unless there is some strong
> reason to change it.
> So couldn't we handle this like there was a
> touch move (asynchronously) right after touch down?
> That would mean touchstart related events would happen in the order they
> happen now, and after those we had the simulated move.

What about whether or not we should include a simulated mousemove?

Comment 32

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

You'd do whatever happens if there actually was a touch move happening right after touch down.

Comment 33

[Michael Wu [:mwu]]

Comment on attachment 8472324 [details] [diff] [review]
Simulate a touchmove in Gonk immediately after a touchstart in the same location.

Seems ok. Not a fan of copying UserInputData just to change the action, but this code is all getting moved to GeckoTouchDispatcher Real Soon Now so maybe we can get something prettier then.

Comment 34

[Doug Sherk (:drs) (inactive)]

Attachment: Simulate a touchmove in Gonk immediately after a touchstart in the same location.

Sorry to request review again, but this has changed significantly as a result of Olli's comments.

Comment 35

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

Comment on attachment 8472436 [details] [diff] [review]
Simulate a touchmove in Gonk immediately after a touchstart in the same location.

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

::: widget/gonk/nsAppShell.cpp
@@ +608,5 @@
>      virtual void monitor() {}
>  
>      // Called on the main thread
>      virtual void dispatchOnce();
> +    virtual void dispatchMouseEvent(UserInputData& data, nsEventStatus status);

I would prefer something like dispatchMouseEventFromTouch but that seems kinda long. I don't feel too strongly about this.

@@ +762,5 @@
>              return;
>          }
>  
>          nsEventStatus status = nsEventStatus_eIgnore;
> +        bool captured;

Since you don't really need the last |if (captured) { return }| block, you can move this back into the HOVER_MOVE block, and add a separate local bool captured inside the ACTION_DOWN block. I think the reduced scoping would make things a bit clearer, even though it means you have two local variables instead of one.

@@ +792,4 @@
>          }
> +
> +        if (captured) {
> +            return;

This is not needed; the break will exit the switch and return anyway.

Comment 36

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

Also CC'ing :mchang since we keep bitrotting his patches. Sorry!

Comment 37

[Doug Sherk (:drs) (inactive)]

Attachment: Simulate a touchmove in Gonk immediately after a touchstart in the same location.

Addressed code review comments from Kats. Will carry r+ in a sec.

Comment 38

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8472457 [details] [diff] [review]
Simulate a touchmove in Gonk immediately after a touchstart in the same location.

Carrying r+ from Kats.

Comment 39

[Michael Wu [:mwu]]

Comment on attachment 8472457 [details] [diff] [review]
Simulate a touchmove in Gonk immediately after a touchstart in the same location.

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

Random thought - could we peak into the event queue and see if there's an upcoming move? Then there's no need to fake one when we don't need to. No idea how likely this is to help real devices, but it might be worth playing with later.

::: widget/gonk/nsAppShell.cpp
@@ +814,5 @@
>      }
>  }
>  
>  void
> +GeckoInputDispatcher::dispatchMouseEventFromInputData(UserInputData& data, nsEventStatus status)

This thing is entirely self contained, so you could just make it a normal static function. Also, there's no other function that calls sendMouseEvent, so this could be the new sendMouseEvent function by inlining sendMouseEvent into this.

sendMouseEvent as a name also fits in better than dispatchMouseEventFromInputData.

Comment 40

[Doug Sherk (:drs) (inactive)]

Attachment: Simulate a touchmove in Gonk immediately after a touchstart in the same location.

(In reply to Michael Wu [:mwu] from comment #39)
> Random thought - could we peak into the event queue and see if there's an
> upcoming move? Then there's no need to fake one when we don't need to. No
> idea how likely this is to help real devices, but it might be worth playing
> with later.

I'm not sure what the value here is. Is there a reason we wouldn't want a touchmove to be dispatched in this case? Also, I could see there being problems in the case where only a touchstart is batched up, but a touchmove will come after the first batch of touch events in this series is handled.

> This thing is entirely self contained, so you could just make it a normal
> static function. Also, there's no other function that calls sendMouseEvent,
> so this could be the new sendMouseEvent function by inlining sendMouseEvent
> into this.
> 
> sendMouseEvent as a name also fits in better than
> dispatchMouseEventFromInputData.

Thanks, that's a great catch.

Carrying r+.

Comment 41

[Doug Sherk (:drs) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a06f8cd41a9

I think I should have landed this on b2g-inbound, actually, but oh well.

Comment 42

[Michael Wu [:mwu]]

(In reply to Doug Sherk (:drs) from comment #40)
> (In reply to Michael Wu [:mwu] from comment #39)
> > Random thought - could we peak into the event queue and see if there's an
> > upcoming move? Then there's no need to fake one when we don't need to. No
> > idea how likely this is to help real devices, but it might be worth playing
> > with later.
> 
> I'm not sure what the value here is. Is there a reason we wouldn't want a
> touchmove to be dispatched in this case? Also, I could see there being
> problems in the case where only a touchstart is batched up, but a touchmove
> will come after the first batch of touch events in this series is handled.
> 

On second thought, we should be able to get the right behavior and not synthesize a touch move if we don't need to when we start interpolating in GeckoTouchDispatcher, so this really doesn't matter.

Comment 43

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8a06f8cd41a9

Comment 44

[Doug Sherk (:drs) (inactive)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4d3473b96641

for regressions: bug 1055214, bug 1049250, bug 1055203

Comment 48

[Doug Sherk (:drs) (inactive)]

We're going to try to take this again next sprint.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/4d3473b96641

Comment 50

[Doug Sherk (:drs) (inactive)]

Attachment: Set .active class on keypad keys in touch listeners instead of using :active.

PR: https://github.com/mozilla-b2g/gaia/pull/22427

Anthony and I decided to go with the Gaia patch for now until we can fix this properly in Gecko. I'll get back to the Gecko patch after 2.1 FL.

Comment 51

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/35ca4477c6ef13dfa0ad13416b76fb5fde1e9cb6

We'll fix this on the platform side in bug 1058255, then revert this patch.