964421 - [B2G][Everything.me]Moving apps up and down from the top of a collection scrolls the collections page.

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Andrew Stole [:astole]]

Attachment: Video

Attempting to move the apps up or down while viewing the collection causes the entire page to scroll making it difficult to customize the top of the collection.

Repro Steps:
1) Updated Buri to BuildID: 20140127004002
2) Open a collection
3) Press and hold on an app in the top of the collection
4) Move the app down then up.

Actual:
The entire collections page scrolls.

Expected:
The user can move an app around in the top of the collection without the page scrolling.

Environmental Variables:
Device: Buri v1.3 Mozilla RIL
BuildID: 20140127004002
Gaia: 25a45a836a4a21a30f63fa7b544b42e8b781180a
Gecko: c40099a42c1f
Version: 28.0a2
Firmware Version: v1.2-device.cfg

Repro frequency: 3/3, 100%
See attached: Video

Comment 1

[Jason Smith [:jsmith]]

What happens in the case when you try to reproduce this bug with a bunch of apps in top portion of e.me collection (i.e. around 20 apps at the top of the collection)?

Comment 2

[Andrew Stole [:astole]]

Added 20 apps to the top of the collection and had the same behavior occur as in Comment 0.

Comment 3

[Jason Smith [:jsmith]]

Tried this myself - this is poor UX. This bug becomes more prominent when more rows of apps are present at the top of a collection. When you hit it, you'll lose control of where the app is being dragged.

Comment 4

[Jason Smith [:jsmith]]

Ran would be the right person to ask to look into this.

Comment 5

[Ran Ben Aharon [:ranbena] EverythingMe]

Amir, can you take a look at this? (I can't reproduce on my device)

Comment 6

[Amir Nissim (:amirn)]

This is a regression. Cannot reproduce on devices with older Gecko (like Ran's device: 29.0a1 Build identifier 20140119054542).

Need to investigate further.

Comment 7

[Peter Bylenga [:PBylenga]]

I think this is an APZC issue. When APZC is enabled, I can repro on the latest nightly buri v1.3 and reported build ID. Once APZC is disabled, I am no longer able to repro the issue on either build.

v1.3 Environmental Variables:
Device: Buri v1.3 MOZ
BuildID: 20140130004001
Gaia: 8defa5bf0cbce290c649e564b7f3ebe708e19b23
Gecko: 6b12800e0e46
Version: 28.0a2
Firmware Version: v1.2-device.cfg

Comment 8

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

Dale, it looks like we need to cancel the panning operation if mContextMenuHandled becomes true in TabChild.cpp. Maybe calling SendContentReceivedTouch(..., true) having that adjust the state in the AsyncPanZoomController to change the state to NOTHING?

Comment 9

[Kai-Chih Hu [:khu]]

Can we get ETA to fix this bug? Thank you.

Comment 10

[Dale Harvey (:daleharvey)]

Will take a look at that, cheers kats

Comment 11

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> Dale, it looks like we need to cancel the panning operation if
> mContextMenuHandled becomes true in TabChild.cpp. Maybe calling
> SendContentReceivedTouch(..., true) having that adjust the state in the
> AsyncPanZoomController to change the state to NOTHING?

Unfortunately, it's not this simple, since APZC requires you to already be in the WAITING_FOR_LISTENERS state for this flow to kick in. I can't think a good reason that we can't alter APZC to just switch to the NOTHING state if it detects any preventDefault set. This seems to work, but there are edge cases I have to deal with still.

Comment 12

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

(In reply to Doug Sherk (:drs) from comment #11)
> Unfortunately, it's not this simple, since APZC requires you to already be
> in the WAITING_FOR_LISTENERS state for this flow to kick in. I can't think a
> good reason that we can't alter APZC to just switch to the NOTHING state if
> it detects any preventDefault set. This seems to work, but there are edge
> cases I have to deal with still.

The "detect any preventDefault" part is going to be a cross-process message, so we'll need to guard against races. That's why I was thinking we could use the WAITING_FOR_LISTENERS state for it, since that already deals with that sort of thing.

Comment 13

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> The "detect any preventDefault" part is going to be a cross-process message,
> so we'll need to guard against races. That's why I was thinking we could use
> the WAITING_FOR_LISTENERS state for it, since that already deals with that
> sort of thing.

What about locking the touch event queue, emptying it, and then setting the APZC state to NOTHING? Is there any other race that could be caused otherwise?

Comment 14

[Doug Sherk (:drs) (inactive)]

Attachment: Simulate a preventDefault when a context menu handles a long tap, make APZC go to NOTHING state when receiving it

What do you think of this?

Comment 15

[Doug Sherk (:drs) (inactive)]

One thing of note is that I'm not sure whether or not content should still receive a long tap up event when a long tap is preventDefaulted. That would change this.

Comment 16

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

I'll look at this more in detail later but I would also like a test for this code if possible. See gfx/tests/gtest/TestAsyncPanZoomController and https://developer.mozilla.org/en-US/docs/GTest

Comment 17

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

Comment on attachment 8370830 [details] [diff] [review]
Simulate a preventDefault when a context menu handles a long tap, make APZC go to NOTHING state when receiving it

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

Some comments below. Overall I think it would be cleaner and more robust to do the following:
1) Modify the HandleLongTap (and other HandleXXX) functions in GeckoContentController to take a guid.
2) in APZC::OnLongPress, after calling HandleLongTap, go into state WAITING_CONTENT_RESPONSE (and queue the timeout task)
3) In TabChild::RecvHandleLongTap, call SendContentReceivedTouch with the default-prevented notification.

::: dom/ipc/TabChild.cpp
@@ +1882,5 @@
>      return true;
>    }
>  
> +  if (mContextMenuHandled) {
> +    if (aEvent.message == NS_TOUCH_END) {

This check needs to be more robust, I think. It at least needs aEvent.message == NS_TOUCH_CANCEL as well. It might also need checking for number of touch points, in case this is a secondary touch point that went up. Assertions and/or documentation would be useful for that.

@@ +1885,5 @@
> +  if (mContextMenuHandled) {
> +    if (aEvent.message == NS_TOUCH_END) {
> +      mContextMenuHandled = false;
> +    } else {
> +      SendContentReceivedTouch(aGuid, true);

This call should only happen once, right? The way you wrote the patch it will happen for all events past the long tap until the end.

::: gfx/layers/ipc/AsyncPanZoomController.cpp
@@ +1764,5 @@
> +  if (mPreventDefaultSet && mPreventDefault) {
> +    mHandlingTouchQueue = true;
> +    mTouchQueue.Clear();
> +    mHandlingTouchQueue = false;
> +    SetState(NOTHING);

This seems like the same as the if (mState == WAITING_CONTENT_RESPONSE) block below, except worse because it will drop all the touch events instead of the just the ones to the end of the touch block. (In particular this code will run *instead* of the code below for regular touch events because there's no state condition in this version).

Comment 18

[Doug Sherk (:drs) (inactive)]

OK, thanks. In particular I completely forgot that the touch event queue can have more than just one series of events from start->move->end.

Comment 19

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> Some comments below. Overall I think it would be cleaner and more robust to
> do the following:
> 1) Modify the HandleLongTap (and other HandleXXX) functions in
> GeckoContentController to take a guid.
> 2) in APZC::OnLongPress, after calling HandleLongTap, go into state
> WAITING_CONTENT_RESPONSE (and queue the timeout task)
> 3) In TabChild::RecvHandleLongTap, call SendContentReceivedTouch with the
> default-prevented notification.

This strategy works. I'm fixing up the existing long press test and adding more.

Comment 20

[Doug Sherk (:drs) (inactive)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16090

There are some backend changes in Gecko that are needed on top of this. This is needed because touches are still sent to APZC after the long press is handled, and content needs to preventDefault them.

Comment 21

[Doug Sherk (:drs) (inactive)]

Attachment: Add a mechanism to HandleLongTap(Up) to allow content to preventDefault touches and stop panning while long tapping.

Basically uses the strategy outlined in comment 17.

Comment 22

[Doug Sherk (:drs) (inactive)]

Attachment: Fix APZC gtest to simulate some TabChild behavior.

Actual additional tests will be coming soon.

Comment 23

[Doug Sherk (:drs) (inactive)]

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

Comment 24

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

Comment on attachment 8372327 [details] [diff] [review]
Add a mechanism to HandleLongTap(Up) to allow content to preventDefault touches and stop panning while long tapping.

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

LGTM. As a heads-up this will have merge problems with bug 965381 so whoever lands second will need to rebase (shouldn't be too hard). Also please make sure that when you land this, you do it on m-i rather than b2g-i so that we don't end up landing on separate branches and causing headaches for the sheriffs on merge.

f? to jimm to check the style on the metro files and as a heads up that this patch exists. It should have no effect on metro.

::: gfx/layers/ipc/AsyncPanZoomController.h
@@ +508,5 @@
>  
>    /**
> +   * 
> +   */
> +  void SetContentResponseTimer();

Fill in the comment

Comment 25

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

Comment on attachment 8372328 [details] [diff] [review]
Fix APZC gtest to simulate some TabChild behavior.

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

::: gfx/tests/gtest/TestAsyncPanZoomController.cpp
@@ +655,5 @@
>  
> +  // To get a LongTapUp event, we must kick APZC to flush its event queue. This
> +  // would normally happen if we had a (Tab|RenderFrame)(Parent|Child)
> +  // mechanism.
> +  apzc->ContentReceivedTouch(false);

If this is the thing that triggers the LongTapUp, can we move the EXPECT_CALL(... HandleLongTapUp ...) to just above this? That way we can ensure that it doesn't get run prematurely.

Comment 26

[Jim Mathies [:jimm]]

Comment on attachment 8372327 [details] [diff] [review]
Add a mechanism to HandleLongTap(Up) to allow content to preventDefault touches and stop panning while long tapping.

Thanks for the heads, lgtm.

Comment 27

[Doug Sherk (:drs) (inactive)]

Attachment: Add a mechanism to HandleLongTap(Up) to allow content to preventDefault touches and stop panning while long tapping.

Addressed review comment, carrying r+ and f?.

Comment 28

[Doug Sherk (:drs) (inactive)]

Attachment: Fix APZC gtest to simulate some TabChild behavior.

Addressed review comment, carrying r+.

Comment 29

[Doug Sherk (:drs) (inactive)]

Another try run since the last one failed on Android:
https://tbpl.mozilla.org/?tree=Try&rev=ac174c706391

I'm going to wait until the tests pass before pushing to inbound, so you can go ahead and push yours first, if you get r+ before then.

Comment 30

[Doug Sherk (:drs) (inactive)]

Better try run:
https://tbpl.mozilla.org/?tree=Try&rev=2b389246d90b

Comment 31

[Doug Sherk (:drs) (inactive)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3712f32bfb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1bf6ae69dfc7

Comment 32

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5c3712f32bfb
https://hg.mozilla.org/mozilla-central/rev/1bf6ae69dfc7

Comment 33

[Amir Nissim (:amirn)]

Doug, which gecko should we install to test with the gaia changes?

Thanks.

Comment 34

[Doug Sherk (:drs) (inactive)]

(In reply to Amir Nissim (Everything.me) from comment #33)
> Doug, which gecko should we install to test with the gaia changes?
> 
> Thanks.

You have to build with mozilla-central/trunk. This hasn't been uplifted to b2g 1.3 yet.

Comment 35

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8372322 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/16090

Moving reviewer to Ran after discussion on GitHub: https://github.com/mozilla-b2g/gaia/pull/16090#issuecomment-34574014

Comment 36

[Ran Ben Aharon [:ranbena] EverythingMe]

Doug, we don't build our own Gecko's :/ Can we flash a device with a specific nightly build?

Comment 37

[Doug Sherk (:drs) (inactive)]

Unfortunately, the nightly build from last night doesn't have it. Maybe you could try the TBPL builds at https://tbpl.mozilla.org/? There's a "B2G Device Image Opt" section for each push, which might work for you. If not, it's probably better to just wait a day for the next nightly build instead of building Gecko yourself.

Comment 38

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

Doug, maybe close this bug since the patches are in, and file a follow-up for adding tests? It would be good to mark this closed since it's the one with the 1.3+ flag sitting on it.

Comment 39

[Doug Sherk (:drs) (inactive)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #38)
> Doug, maybe close this bug since the patches are in, and file a follow-up
> for adding tests? It would be good to mark this closed since it's the one
> with the 1.3+ flag sitting on it.

We're still waiting on review and pushing of attachment 8372322 [details] [review] which is essential for this to work. But you're right, I should file a follow-up for the tests.

Comment 40

[Ran Ben Aharon [:ranbena] EverythingMe]

Doug, I flashed with today's nightly build and... the bug does not reproduce. (I didn't apply your patch.)
The build I'm using isn't from Mozilla nightly's (it's from one of our partners) Gecko d2ef169.

Could it be that the patch isn't needed?

Comment 41

[Doug Sherk (:drs) (inactive)]

(In reply to Ran Ben Aharon (Everything.me) from comment #40)
> Doug, I flashed with today's nightly build and... the bug does not
> reproduce. (I didn't apply your patch.)
> The build I'm using isn't from Mozilla nightly's (it's from one of our
> partners) Gecko d2ef169.
> 
> Could it be that the patch isn't needed?

Yes, I'm unable to repro it too. I'm going to resolve this as fixed if you're ok with that.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

The Gecko patches need some serious rebasing for b2g28. Might as well check the Gaia patch while you're at it and put up a new PR for that if it doesn't cherry-pick nicely.

Comment 43

[Doug Sherk (:drs) (inactive)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #42)
> The Gecko patches need some serious rebasing for b2g28. Might as well check
> the Gaia patch while you're at it and put up a new PR for that if it doesn't
> cherry-pick nicely.

The Gaia patch is not needed. I'll rebase for 1.3 and uplift it myself.

Comment 44

[Doug Sherk (:drs) (inactive)]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/7ad08f5284e9
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/8f9821730b2c

Comment 45

[Doug Sherk (:drs) (inactive)]

Backed out in https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/90fb02580842 due to gtest failures.

Comment 46

[Doug Sherk (:drs) (inactive)]

Something is affecting the flow and causing a long tap up event from not getting fired. It isn't a simple rebase.

Comment 47

[Doug Sherk (:drs) (inactive)]

This requires the changes in bug 795567 to work. I'm going to try rebasing without it, but I think this is going to require an additional patch and review for the patch specific to 1.3.

Comment 48

[Doug Sherk (:drs) (inactive)]

OK, I don't think the changes that I needed to make are huge enough that I need to request review again. I'll point out what I changed after I land the uplift, and if :kats thinks it's bad enough, we can do a follow-up fix.

Unfortunately, I was unable to test whether or not the patches actually work on 1.3. The tests pass now, and they work on master. I tried flashing my Hamachi device with 1.3+these patches, but the screen just goes blank on boot up. Hopefully QA can verify whether it's working or not.

Comment 49

[Doug Sherk (:drs) (inactive)]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/31f163ebb244
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/ab07e61c2eb0

Comment 50

[Doug Sherk (:drs) (inactive)]

Without bug 795567 uplifted, I had to do the following workaround to get the event queue to flush:
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/31f163ebb244#l6.131

I also had to re-add back the SendAsyncScrollDOMEvent expected call in the test fix patch:
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/ab07e61c2eb0#l1.13

Comment 51

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

(In reply to Doug Sherk (:drs) from comment #50)
> Without bug 795567 uplifted, I had to do the following workaround to get the
> event queue to flush:
> https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/31f163ebb244#l6.131
> 
> I also had to re-add back the SendAsyncScrollDOMEvent expected call in the
> test fix patch:
> https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/ab07e61c2eb0#l1.13

These changes look ok, as far as I can tell. Thanks for taking care of this tricky rebase!

Comment 52

[Doug Sherk (:drs) (inactive)]

I was able to build and flash 1.3 and I verified that this is fixed there.