Difficult or impossible to exit the edit icons screen

RESOLVED FIXED in Firefox OS v2.0

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: khuey, Assigned: kgrandon)

Tracking

unspecified
2.1 S2 (15aug)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [systemsfe])

Attachments

(2 attachments)

When editing the icons on the homescreen I find it essentially impossible to exit the screen by tapping the done button in the top right.  Gregor and I reproduced this pretty easily on our debug Gecko builds.  He suspects it might be a race condition.

I also see "WARNING: Unhandled state upon single touch start: file ../../../gfx/layers/apz/src/GestureEventListener.cpp, line 136" in logcat.
(Assignee)

Updated

4 years ago
Component: Gaia::Homescreen → Panning and Zooming
Product: Firefox OS → Core
I can look into this tomorrow but if you happen to catch it in a debugger before I get to it, the value of mState at that point would be useful to know.
Assignee: nobody → bugmail.mozilla
OS: Windows 7 → Gonk (Firefox OS)
Hardware: x86_64 → ARM
I'm not able to reproduce the non-working "done" button on latest master (m-c as of this morning). I can reproduce a different issue though:

- Long-press an icon to go into edit mode
- Hit the done button to go back
- Long-press an icon again <-- this will not go into edit mode

With these STR I see the warning from comment 0 get triggered, it's happening because the long-press event is cancelled by content, and so the touch-block (which includes the touch-up event) doesn't get delivered to the GEL. This leaves it in the long-tap state when the next touch-start comes in and so it doesn't process that properly. It handles this case gracefully by resetting the state so the next touch-down should get handled fine.

I'll put up a patch for this issue and hope it fixes the issue described in comment 0 as well.
Created attachment 8465487 [details] [diff] [review]
Patch

When processing a prevent-defaulted block of input events, clear the state in the GEL.
Attachment #8465487 - Flags: review?(botond)

Updated

4 years ago
Attachment #8465487 - Flags: review?(botond) → review+
Oh I can repro the original issue on a Hamachi. I debugged it and it doesn't appear to be APZ-related. I see TabChild::FireSingleTapEvent getting called as expected when I tap on the "done" button. It might be a gaia bug; will check.
Kevin, this looks like a homescreen bug. I definitely see the click event getting dispatched to the content, and the click handler is running, which fires the hashchange event. But window.app.grid._grid.dragdrop.inEditMode is always false for some reason, so window.app.grid._grid.dragdrop.exitEditMode() never gets called. If I call it manually from the console it exits the mode.
Flags: needinfo?(kgrandon)
(Assignee)

Comment 7

4 years ago
Ok, I will dig in. The gaia code here looks extremely simple, so I'm not sure what we could be doing wrong, but I wonder if we could be preventing some touch event from propagating.

We add the edit mode listener here: https://github.com/mozilla-b2g/gaia/blob/bda8c70178259368a13485d8b1ac755370ac9f2a/apps/verticalhome/js/app.js#L20

This should fire a hashchange event and exit edit mode here: https://github.com/mozilla-b2g/gaia/blob/bda8c70178259368a13485d8b1ac755370ac9f2a/apps/verticalhome/js/app.js#L195
Flags: needinfo?(kgrandon)
I stuck a breakpoint inside the enterEditMode() function in grid_dragdrop.js, and the "this" object in there was not the same as the window.app.grid._grid.dragdrop object. So this is definitely a bug in some JS code somewhere. I'll leave it to you :)
(Assignee)

Comment 9

4 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #8)
> I stuck a breakpoint inside the enterEditMode() function in
> grid_dragdrop.js, and the "this" object in there was not the same as the
> window.app.grid._grid.dragdrop object. So this is definitely a bug in some
> JS code somewhere. I'll leave it to you :)

enterEditMode() should really not matter here, the exit code should be independent. I'm looking into this, but I don't really see how that this is possible.

Also having a hard time reproducing on my flame.
I only saw it on hamachi, not on flame. I can look into it more tomorrow if you can't repro.
Keywords: leave-open
Component: Panning and Zooming → Gaia::Homescreen
Product: Core → Firefox OS
On my Hamachi around 25 dragdrop instances are created at [1]. Only one of them will actually process the contextmenu event because the handler does a stopImmediatePropagation at [2]. So only one of the dragdrop instances goes into edit mode, and it's got a 1/25 chance of being the one that the grid_view keeps a handle to.

It seems that the stopImmediatePropagation line was removed very recently in bug 1043293 (I don't have that change locally) but that might fix this issue. However it won't stop all 25 dragdrop instances from being created, which is kind of a waste. I stuck a | if (!dragdrop) | guard *inside* the LazyLoader callback at [1] and that fixed the problem for me locally.

Up to you how you want to fix it.

[1] https://github.com/mozilla-b2g/gaia/blob/dd7e13eb8c6a50a72b447c92eddbe92f6fc655c5/shared/elements/gaia_grid/js/grid_view.js#L403
[2] https://github.com/mozilla-b2g/gaia/blob/dd7e13eb8c6a50a72b447c92eddbe92f6fc655c5/shared/elements/gaia_grid/js/grid_dragdrop.js#L438
Assignee: bugmail.mozilla → kgrandon
(Assignee)

Comment 12

4 years ago
Nice work Kats. Let's see what bug 1043293 does to this.
It might fix it, but there's still a leak here. The 24 extra DragDrop instances don't get GC'd because they're registered as event listeners and never unregister themselves.
No longer blocks: 1009733
(Assignee)

Updated

4 years ago
Blocks: 1047639
(Assignee)

Comment 15

4 years ago
Created attachment 8466562 [details]
Pull request - Safeguard dragdrop intiitalization

Hey Chris - Any chance you could help me review this simple patch? I just want to safeguard and make sure that dragdrop is only instantiated once. I was unable to reproduce this bug, but this should help. Thanks!
Attachment #8466562 - Flags: review?(chrislord.net)
Comment on attachment 8466562 [details]
Pull request - Safeguard dragdrop intiitalization

LGTM.
Attachment #8466562 - Flags: review?(chrislord.net) → review+
(Assignee)

Comment 17

4 years ago
Thanks for the review! Master: https://github.com/mozilla-b2g/gaia/commit/a78866090e213c8321a5d19e01475276727affb6
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

4 years ago
[Blocking Requested - why for this release]: This should block 2.0 both as a potential solution for bug 1047639 and the fact that it could be a pretty bad race condition. I was unable to reproduce this on a flame device, but apparently people could reproduce it on hamachis.
blocking-b2g: --- → 2.0?
Note only the gaia patch needs going into 2.0; the gecko patch won't apply there.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1047948
(Assignee)

Updated

4 years ago
No longer blocks: 1047639
(In reply to Kevin Grandon :kgrandon from comment #18)
> [Blocking Requested - why for this release]: This should block 2.0 both as a
> potential solution for bug 1047639 and the fact that it could be a pretty
> bad race condition. I was unable to reproduce this on a flame device, but
> apparently people could reproduce it on hamachis.

Sounds good - blocking to help resolve bug 1047639.
blocking-b2g: 2.0? → 2.0+
Whiteboard: [systemsfe]
Target Milestone: --- → 2.1 S2 (15aug)
v2.0: https://github.com/mozilla-b2g/gaia/commit/bc629048b01c1cbf71a66386b2b9b2ccbf592ebc
status-b2g-v2.0: --- → fixed
status-b2g-v2.1: --- → fixed
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.