Closed
Bug 964421
Opened 11 years ago
Closed 11 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)
Tracking
()
People
(Reporter: astole, Assigned: drs)
References
Details
(Keywords: regression, Whiteboard: dogfood1.3 ETA:2/7)
Attachments
(4 files, 3 obsolete files)
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•11 years ago
|
||
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
Reporter | ||
Comment 2•11 years ago
|
||
Added 20 apps to the top of the collection and had the same behavior occur as in Comment 0.
Comment 3•11 years ago
|
||
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?
Updated•11 years ago
|
Whiteboard: dogfood1.3 → dogfood1.3 [systemsfe]
Updated•11 years ago
|
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(crdlc)
Comment 4•11 years ago
|
||
Ran would be the right person to ask to look into this.
Flags: needinfo?(crdlc) → needinfo?(ran)
Comment 5•11 years ago
|
||
Amir, can you take a look at this? (I can't reproduce on my device)
Flags: needinfo?(ran) → needinfo?(amirn)
Comment 6•11 years ago
|
||
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)
Updated•11 years ago
|
Assignee: nobody → amirn
Updated•11 years ago
|
Keywords: regression
Updated•11 years ago
|
Keywords: regressionwindow-wanted
Comment 7•11 years ago
|
||
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
Updated•11 years ago
|
Blocks: gaia-apzc
Component: Gaia::Everything.me → Panning and Zooming
Keywords: regressionwindow-wanted
Product: Firefox OS → Core
Version: unspecified → 28 Branch
Updated•11 years ago
|
Whiteboard: dogfood1.3 [systemsfe] → dogfood1.3
Comment 8•11 years ago
|
||
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?
Updated•11 years ago
|
Flags: needinfo?(dale)
Comment 9•11 years ago
|
||
Can we get ETA to fix this bug? Thank you.
Updated•11 years ago
|
Assignee: amirn → nobody
Comment 10•11 years ago
|
||
Will take a look at that, cheers kats
Assignee: nobody → dale
Flags: needinfo?(dale)
Updated•11 years ago
|
Whiteboard: dogfood1.3 → dogfood1.3 ETA:2/7
Assignee | ||
Updated•11 years ago
|
Assignee: dale → bugzilla
Assignee | ||
Comment 11•11 years ago
|
||
(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.
Comment 12•11 years ago
|
||
(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.
Assignee | ||
Comment 13•11 years ago
|
||
(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?
Assignee | ||
Comment 14•11 years ago
|
||
What do you think of this?
Attachment #8370830 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 15•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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-
Assignee | ||
Comment 18•11 years ago
|
||
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.
Assignee | ||
Comment 19•11 years ago
|
||
(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.
Assignee | ||
Comment 20•11 years ago
|
||
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)
Assignee | ||
Comment 21•11 years ago
|
||
Basically uses the strategy outlined in comment 17.
Attachment #8370830 -
Attachment is obsolete: true
Attachment #8372327 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 22•11 years ago
|
||
Actual additional tests will be coming soon.
Attachment #8372328 -
Flags: review?(bugmail.mozilla)
Assignee | ||
Comment 23•11 years ago
|
||
Comment 24•11 years ago
|
||
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 25•11 years ago
|
||
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 26•11 years ago
|
||
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+
Assignee | ||
Comment 27•11 years ago
|
||
Addressed review comment, carrying r+ and f?.
Attachment #8372327 -
Attachment is obsolete: true
Attachment #8372372 -
Flags: review+
Attachment #8372372 -
Flags: feedback?(jmathies)
Assignee | ||
Comment 28•11 years ago
|
||
Addressed review comment, carrying r+.
Attachment #8372328 -
Attachment is obsolete: true
Attachment #8372373 -
Flags: review+
Assignee | ||
Comment 29•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #8372372 -
Flags: feedback?(jmathies) → feedback+
Assignee | ||
Comment 30•11 years ago
|
||
Better try run:
https://tbpl.mozilla.org/?tree=Try&rev=2b389246d90b
Assignee | ||
Comment 31•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3712f32bfb
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1bf6ae69dfc7
Whiteboard: dogfood1.3 ETA:2/7 → [dogfood1.3 ETA:2/7 leave-open]
Assignee | ||
Updated•11 years ago
|
Attachment #8372372 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Attachment #8372373 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
Whiteboard: [dogfood1.3 ETA:2/7 leave-open] → dogfood1.3 ETA:2/7 [leave open]
Comment 32•11 years ago
|
||
Comment 33•11 years ago
|
||
Doug, which gecko should we install to test with the gaia changes?
Thanks.
Flags: needinfo?(bugzilla+drs)
Assignee | ||
Comment 34•11 years ago
|
||
(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)
Assignee | ||
Comment 35•11 years ago
|
||
Comment on attachment 8372322 [details]
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)
Comment 36•11 years ago
|
||
Doug, we don't build our own Gecko's :/ Can we flash a device with a specific nightly build?
Assignee | ||
Comment 37•11 years ago
|
||
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•11 years ago
|
||
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.
Assignee | ||
Comment 39•11 years ago
|
||
(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] which is essential for this to work. But you're right, I should file a follow-up for the tests.
Comment 40•11 years ago
|
||
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?
Assignee | ||
Comment 41•11 years ago
|
||
(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
Assignee | ||
Updated•11 years ago
|
Attachment #8372322 -
Flags: review?(ran)
Updated•11 years ago
|
status-b2g-v1.3:
--- → affected
status-b2g-v1.4:
--- → fixed
status-firefox28:
--- → wontfix
status-firefox29:
--- → wontfix
status-firefox30:
--- → fixed
Target Milestone: --- → mozilla30
Updated•11 years ago
|
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 42•11 years ago
|
||
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)
Keywords: branch-patch-needed
Assignee | ||
Comment 43•11 years ago
|
||
(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)
Assignee | ||
Comment 44•11 years ago
|
||
Assignee | ||
Comment 45•11 years ago
|
||
Backed out in https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/90fb02580842 due to gtest failures.
Assignee | ||
Comment 46•11 years ago
|
||
Something is affecting the flow and causing a long tap up event from not getting fired. It isn't a simple rebase.
Assignee | ||
Comment 47•11 years ago
|
||
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.
Assignee | ||
Comment 48•11 years ago
|
||
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
Assignee | ||
Comment 49•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/31f163ebb244
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/ab07e61c2eb0
Keywords: branch-patch-needed → qawanted
Assignee | ||
Comment 50•11 years ago
|
||
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
Updated•11 years ago
|
Comment 51•11 years ago
|
||
(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!
Assignee | ||
Comment 52•11 years ago
|
||
I was able to build and flash 1.3 and I verified that this is fixed there.
Keywords: verifyme
Updated•11 years ago
|
status-b2g-v1.3T:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•