Closed Bug 964421 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Panning and Zooming, defect)

28 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
blocking-b2g 1.3+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: astole, Assigned: drs)

References

Details

(Keywords: regression, Whiteboard: dogfood1.3 ETA:2/7)

Attachments

(4 files, 3 obsolete files)

Attached video 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
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)?
Keywords: qawanted
Added 20 apps to the top of the collection and had the same behavior occur as in Comment 0.
Keywords: qawanted
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.
blocking-b2g: --- → 1.3?
Whiteboard: dogfood1.3 → dogfood1.3 [systemsfe]
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(crdlc)
Ran would be the right person to ask to look into this.
Flags: needinfo?(crdlc) → needinfo?(ran)
Amir, can you take a look at this? (I can't reproduce on my device)
Flags: needinfo?(ran) → needinfo?(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.
Flags: needinfo?(amirn)
Assignee: nobody → amirn
Keywords: regression
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
Blocks: gaia-apzc
Component: Gaia::Everything.me → Panning and Zooming
Product: Firefox OS → Core
Version: unspecified → 28 Branch
Whiteboard: dogfood1.3 [systemsfe] → dogfood1.3
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?
Flags: needinfo?(dale)
Can we get ETA to fix this bug? Thank you.
Assignee: amirn → nobody
Will take a look at that, cheers kats
Assignee: nobody → dale
Flags: needinfo?(dale)
Whiteboard: dogfood1.3 → dogfood1.3 ETA:2/7
Assignee: dale → bugzilla
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
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.
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 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).
Attachment #8370830 - Flags: review?(bugmail.mozilla) → review-
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
Attachment #8372322 - Flags: review?(amirn)
Basically uses the strategy outlined in comment 17.
Attachment #8370830 - Attachment is obsolete: true
Attachment #8372327 - Flags: review?(bugmail.mozilla)
Actual additional tests will be coming soon.
Attachment #8372328 - Flags: review?(bugmail.mozilla)
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
Attachment #8372327 - Flags: review?(bugmail.mozilla)
Attachment #8372327 - Flags: review+
Attachment #8372327 - Flags: feedback?(jmathies)
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.
Attachment #8372328 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8372327 - Flags: feedback?(jmathies) → feedback+
Addressed review comment, carrying r+ and f?.
Attachment #8372327 - Attachment is obsolete: true
Attachment #8372372 - Flags: review+
Attachment #8372372 - Flags: feedback?(jmathies)
Addressed review comment, carrying r+.
Attachment #8372328 - Attachment is obsolete: true
Attachment #8372373 - Flags: review+
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.
Attachment #8372372 - Flags: feedback?(jmathies) → feedback+
Attachment #8372372 - Flags: checkin+
Attachment #8372373 - Flags: checkin+
Whiteboard: [dogfood1.3 ETA:2/7 leave-open] → dogfood1.3 ETA:2/7 [leave open]
Doug, which gecko should we install to test with the gaia changes?

Thanks.
Flags: needinfo?(bugzilla+drs)
(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.
Flags: needinfo?(bugzilla+drs)
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
Attachment #8372322 - Flags: review?(amirn) → review?(ran)
Doug, we don't build our own Gecko's :/ Can we flash a device with a specific nightly build?
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.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Blocks: 970578
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?
(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.
Whiteboard: dogfood1.3 ETA:2/7 [leave open] → dogfood1.3 ETA:2/7
Attachment #8372322 - Flags: review?(ran)
Target Milestone: --- → mozilla30
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(drs+bugzilla)
(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.
Flags: needinfo?(drs+bugzilla)
Something is affecting the flow and causing a long tap up event from not getting fired. It isn't a simple rebase.
Depends on: 795567
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.
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.
No longer depends on: 795567
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
Keywords: qawantedverifyme
(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!
I was able to build and flash 1.3 and I verified that this is fixed there.
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.