1132391 - Edit mode can freeze

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect)

Description

[Kevin Grandon :kgrandon]

We see the following error: [JavaScript Error: "too much recursion"]

I think this might have something to do with trying to drag/drop a place holder, or single app within a group. Working on narrowing this down.

Comment 1

[Kevin Grandon :kgrandon]

Still trying to narrow down this STR, but I think this works:

1 - Have a group at the top of the screen with only 1 icon.
2 - Long-press on the group header and drop it in the same position.
3 - Expand the group.
4 - Long press and let go on an empty place after the first icon (a placeholder).
5 - Try to long-press on any icon, I think we are inside an infinite loop at this point.

Comment 2

[Kevin Grandon :kgrandon]

Probably want to block as this makes the home screen freeze.

Comment 3

[Kevin Grandon :kgrandon]

Found an STR:

1 - Have a group at the top of the screen with only 1 icon.
2 - Long press the icon to enter edit mode and release.
3 - Long press and let go on an empty place after the first icon (a placeholder).
4 - Try to long-press on any icon, I think we are inside an infinite loop at this point.

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1132391_placeholder_pointer_events > mozilla-b2g:master

Comment 5

[Kevin Grandon :kgrandon]

Comment on attachment 8563318 [details] [review]
[gaia] KevinGrandon:bug_1132391_placeholder_pointer_events > mozilla-b2g:master

Chris - you're right, after finding the STR it was a pretty simple fix.

I spent quite a long time trying to write a test for this, but ultimately failed. Following the STR in a test is quite easy, but I don't think we have good methods to easily follow the same path as our touch events do. If you have any ideas let me know, but I think we might just want to land this without a test. I don't think it would easily regress.

Comment 6

[Kevin Grandon :kgrandon]

This also allows users to long-press on a placeholder to move the entire group which I think is a nice benefit.

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 8563318 [details] [review]
[gaia] KevinGrandon:bug_1132391_placeholder_pointer_events > mozilla-b2g:master

This is a conditional r+, because:

I think this rule will add weird behaviour if you long-press on a placeholder in a large group - the group would then collapse, but be very far from the user's finger...

So this is an r+ on the condition that you disable placeholders triggering group moves (I think a CSS rule that disabled pointer events on not:(.collapsed) group backgrounds/shadows would probably do the trick - likely there's already a rule that can be modified).

Comment 8

[Kevin Grandon :kgrandon]

Thanks. I've added a new selector to handle the existing pointer events rule for a lot of the grouping stuff. Starting the drag from the bottom of a large group did indeed screw up the experience.

In master: https://github.com/mozilla-b2g/gaia/commit/9fe2b6583206cea52656b0d00941684ecfc58e42

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8563318 [details] [review]
[gaia] KevinGrandon:bug_1132391_placeholder_pointer_events > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Unsure, this likely group with some grouping work.
[User impact] if declined: The app can freeze, causing it to restart.
[Testing completed]: Manual testing - currently working on adding an integration test for this, but it's extremely difficult.
[Risk to taking this patch] (and alternatives if risky): Low risk as it's a simple CSS-only patch.
[String changes made]: No

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Reverted because it was my best guess for what caused the Gu near-permafail starting on the below Bumper Bot push:
https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=bd459caf1f9a

Master: https://github.com/mozilla-b2g/gaia/commit/3c6d38bb0b016eb8d1083a8f3c3d837303a71dda

https://treeherder.mozilla.org/logviewer.html#?job_id=1344176&repo=b2g-inbound

Comment 11

[Kevin Grandon :kgrandon]

Hmm, this seems to be intermittent on b-i somehow? Will investigate tomorrow.

Comment 12

[GitHub Autolander Bot]

Attachment: [gaia](reland) KevinGrandon:bug_1132391_reland > mozilla-b2g:master

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8563691 [details] [review]
[gaia](reland) KevinGrandon:bug_1132391_reland > mozilla-b2g:master

This patch is to reland when/if things turn green.

Comment 14

[bhavana bajaj [:bajaj]]

waiting for re-landing here before taking it on the branch.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

*sigh* wasn't yours, so I'm back to the drawing board

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/237525eddabc234e886cea22b51ddcd1504207dc

Comment 17

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/ea64caf6d4ab03fc4472eca9f41f20d651d55fa9

Comment 18

[Hermes Cheng (inactive after July 27, 2015)]

cannot find the error logs [JavaScript Error: "too much recursion"]

*2.2
Build ID               20150326164141
Gaia Revision          6d0174e28576f2f93e696a43d1ac3b03340117f6
Gaia Date              2015-03-26 21:47:33
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/820c2f2e817a
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150326.201153
Firmware Date          Thu Mar 26 20:12:05 EDT 2015
Bootloader             L1TC000118D0

*master
Build ID               20150326160206
Gaia Revision          525c341254e08f07f90da57a4d1cd5971a3cc668
Gaia Date              2015-03-26 16:34:16
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/59554288b4eb
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150326.193247
Firmware Date          Thu Mar 26 19:32:58 EDT 2015
Bootloader             L1TC100118D0