Closed Bug 1139303 Opened 9 years ago Closed 9 years ago

[Homescreen] Edit mode can be very janky

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Homescreen
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(b2g-v2.2 fixed, b2g-master fixed)

Status:
RESOLVED FIXED
Milestone:
2.2 S9 (3apr)
Tracking Flags:
Tracking Status
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: cwiiis, Assigned: cwiiis)

References

Details

(Keywords: perf, polish, Whiteboard: [systemsfe])

Attachments

(1 file, 1 obsolete file)

[gaia] Cwiiis:bug1139303-optimise-edit-mode > mozilla-b2g:master
46 bytes, text/x-github-pull-request
cwiiis
: review+
bajaj
: approval-gaia-v2.2+
Details | Review 
will-change is overused on the homescreen due to performance bugs that don't exist (as much) anymore. Meanwhile, the behaviour of will-change has changed so that if you use it too much, it doesn't work at all.

The consequence is that edit mode can behave very jankily when you have more than around a page-full of visible icons.

We should rework the CSS a little so we don't overuse will-change, so that the times we do use it will work and help improve performance.


I think this should block, as it should have a very noticeable effect and the change will be mostly (possibly entirely) CSS and thus very low risk.
hmm, removing blocker nom because I thought this was easy and turns out it isn't... If I find a quick/easy fix, will re-nom.
blocking-b2g: 2.2? → ---
Got some promising initial work on this, ends up we're doing way more work during edit mode than necessary and platform isn't doing a good job at catching redundant style changes...
Well, ends up there are multiple issues, not just will-change, and solving the will-change problem temporarily makes things worse, so retitling this bug to be about the larger issue. Got a few commits that I think will solve the large part of this, ends up all the issues are quite small (and probably compounded over time).
Summary: will-change often has no effect on home-screen due to overuse → [Homescreen] Edit mode can be very janky
Comment on attachment 8572596 [details] [review]
[gaia] Cwiiis:bug1139303-optimise-edit-mode > mozilla-b2g:master

These fixes significantly improve edit mode for me, but I'd appreciate a second opinion as well as the review.

I'd also really like to keep these commits separate as they all do distinct things (but I think they're small enough and related enough to be kept in the same bug).

Scrolling when holding an item at the top/bottom is still pretty janky, but I think as it's pretty much unrelated to these fixes, it should be in a different bug.
Attachment #8572596 - Flags: review?(kgrandon)
Oh, another polish/perf issue that this doesn't fix that would be nice is the colour-change of the background element. Animating background-color, unfortunately isn't an async animation (though it could be... Thought I'd filed a bug for this already, but can't find it - will file one), so it might be worth rejigging things a bit so we can do the effect with an opacity animation instead.
Comment on attachment 8572596 [details] [review]
[gaia] Cwiiis:bug1139303-optimise-edit-mode > mozilla-b2g:master

I like the direction of this, but after playing with it a bit on a flame it seems to not be an improvement in every way. Example: dragging an icon stutters a lot more with the patch, and I'm assuming this is because of POSITION_CHANGE_PROCESS_FREQ? For me personally, this results in a worse perceived performance. Is it meant to feel as smooth as it does on master?

I'm also seeing some problem where the group background will flash as if it's active after releasing an icon on a group.
Flags: needinfo?(chrislord.net)
Attachment #8572596 - Flags: review?(kgrandon)
On my Flame I also see a stutter when dragging the icon. It does seem like the performance may be slightly worse with the patch (or least that's how it seems).
Doh! Ok, back to the drawing board... In my defence, I did most of my testing on a branch that didn't have bug 1118006 merged, and that bug goes a long way in papering over some of the badness in our code. Might have to give up on the idea of not setting will-change on the icons...
Flags: needinfo?(chrislord.net)
Ok, oddly having rebased this on master, performance seems much better...? Still looking.
Comment on attachment 8572596 [details] [review]
[gaia] Cwiiis:bug1139303-optimise-edit-mode > mozilla-b2g:master

Hey both, what do you think of this? Most of the jank seems ironed out to me now, and I fixed up the weirdness while auto-scrolling in edit mode. The biggest improvement I think is in response time when long-pressing icons to initiate dragging, otherwise the interactive performance wasn't all that bad before.

One big bit of jank left (that I think is mostly the same without these patches) is the fading of the background between blue and grey... I think I'll try to fix that up too, but want to know if this feels at all better/worse to either of you.
Attachment #8572596 - Flags: feedback?(tshakespeare)
Attachment #8572596 - Flags: feedback?(kgrandon)
Comment on attachment 8572596 [details] [review]
[gaia] Cwiiis:bug1139303-optimise-edit-mode > mozilla-b2g:master

I think this looks pretty good. It does introduce some edge case where I stopped being able to move icons though - I haven't yet tracked that down, and nothing in logcat. I'll look at the code, and see if I spot anything.
Attachment #8572596 - Flags: feedback?(kgrandon) → feedback+
Comment on attachment 8572596 [details] [review]
[gaia] Cwiiis:bug1139303-optimise-edit-mode > mozilla-b2g:master

The jankiness does seem a lot better. I did run into one issue when moving groups around. It became stuck and I couldn't move the group. Hadn't experienced that before so IDK if it's the patch or not.

https://www.youtube.com/watch?v=eOcVJkj35HI&feature=youtu.be
Attachment #8572596 - Flags: feedback?(tshakespeare) → feedback+
(In reply to Tiffanie Shakespeare from comment #13)
> Comment on attachment 8572596 [details] [review]
> [gaia] Cwiiis:bug1139303-optimise-edit-mode > mozilla-b2g:master
> 
> The jankiness does seem a lot better. I did run into one issue when moving
> groups around. It became stuck and I couldn't move the group. Hadn't
> experienced that before so IDK if it's the patch or not.
> 
> https://www.youtube.com/watch?v=eOcVJkj35HI&feature=youtu.be

This is odd, thanks for the video - Going on what Kevin was saying, I'm assuming this patch breaks it - I'll try to reproduce and fix.
Trying to theorise how comment #13 could happen, as I've not been able to reproduce it yet.

If the icon/group is highlighted and lifted like that, but just not moving when you move your finger, positionIcon can't be being called in response to touch-move. The only time that should happen, I think, is if either inDragAction is false (which shouldn't happen if begin has been called, which is what sets the raised style and so on), or if this.isScrolling is true but actually the scrolling callback isn't happening - but given how that function is structured, I don't see that being possible either...

I do see a problem that I think it's possible for positionAndScrollIfNeeded to be called after finish() has been called which is easily fixed, but I think this problem is pre-existing. Maybe it's possible that that's affecting state badly, or causing an exception somewhere bad (although Kevin said nothing in logcat, so I guess not?)
Comment on attachment 8572596 [details] [review]
[gaia] Cwiiis:bug1139303-optimise-edit-mode > mozilla-b2g:master

For the life of me I can't reproduce the issue that you and Tiffanie mentioned, but I've also been testing with a rebase and the fix that I mentioned in the last comment, so it may well be gone now.

I think this is ready for review, but I'd also appreciate testing too, to see if you can reproduce the bad state and if so, if you can figure out some STR.

The bad background fading I'll open a new bug for, I think that's separate enough, pre-existing and this is already kinda big.
Flags: needinfo?(tshakespeare)
Attachment #8572596 - Flags: review?(kgrandon)
Comment on attachment 8572596 [details] [review]
[gaia] Cwiiis:bug1139303-optimise-edit-mode > mozilla-b2g:master

Sorry for the delay in review. The changes make sense to me conceptually, though it was hard for me to notice when playing with the code. I did leave a comment on github that I think it would be worthwhile to see if we could clean up, though it wouldn't block the review. Feel free to flag me again if needed/desired.

I do think that you should probably squash this to a single commit before landing as it was reviewed in one go, and it's a bit tricky to determine the state after any individual commit.
Attachment #8572596 - Flags: review?(kgrandon) → review+
Sorry for the delay! Looks good to me - I don't see the issue I was seeing before. Thanks Chris!
Flags: needinfo?(tshakespeare)
Well, let's see how autolander deals with this. I think tests are green, but I'm not sure.
Keywords: checkin-needed
Oh, now tests trigger! Never mind...
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
I'm still in two minds as to whether this is worth uplifting or not. I think it's a significant improvement, but it's not without risk and perhaps what's there at the moment is good enough (disclosure; I don't think either state is actually good enough, but speaking in relative terms).

Tiffanie, what do you think?
Flags: needinfo?(tshakespeare)
Reverted for possibly causing Gij failures on b-i. This is strange since it was passing on TC, but failed on b-i =(

Revert: https://github.com/mozilla-b2g/gaia/commit/aebfbd998041e960cea0468533c0b5041b504850

Failure: http://ftp.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/b2g-inbound-linux64_gecko/1427211628/b2g-inbound_ubuntu64_vm-b2gdt_test-gaia-js-integration-10-bm54-tests1-linux64-build65.txt.gz

I want to make sure this fixes things (if not will re-land). Adding a needinfo on myself to take a look later.
Status: RESOLVED → REOPENED
Flags: needinfo?(kgrandon)
Resolution: FIXED → ---
Just to follow-up here, we are also looking at switching out gaia jobs on TH to use the new TC jobs. Since this test was not failing on TC somehow, we could potentially just re-land it.
Chris - it appears that this was the cause of breakage on Gij10 in comment 23. This is unfortunate as try requests are currently running on TC, but the b-i jobs are still on buildbot. We are trying to get jobs ported to TC, and this should be done in a few days in which you could simply land this.

Adding a needinfo on you in case you want to take a look at the test in the meantime. If this is a real failure that is not caught on TC then we should figure out why. My hunch is that this might be related to a resolution difference between environments? Trying to reproduce the failure locally would be the first step I imagine.
Flags: needinfo?(kgrandon) → needinfo?(chrislord.net)
(In reply to Kevin Grandon :kgrandon from comment #25)
> Chris - it appears that this was the cause of breakage on Gij10 in comment
> 23. This is unfortunate as try requests are currently running on TC, but the
> b-i jobs are still on buildbot. We are trying to get jobs ported to TC, and
> this should be done in a few days in which you could simply land this.
> 
> Adding a needinfo on you in case you want to take a look at the test in the
> meantime. If this is a real failure that is not caught on TC then we should
> figure out why. My hunch is that this might be related to a resolution
> difference between environments? Trying to reproduce the failure locally
> would be the first step I imagine.

I can reproduce the failure locally, so taking a look. It might be a bit racey, so that could explain it not failing on TC I guess?
Flags: needinfo?(chrislord.net)
huh, so it's a real bug actually, I can replicate this testing manually. I don't know why TC isn't catching this.
And the error is trivial - I renamed a function and forgot to mirror the rename in another file... My bad for not testing well enough before the final push (which is also when I did the rename :p)
Comment on attachment 8583070 [details] [review]
[gaia] Cwiiis:bug1139303-optimise-edit-mode > mozilla-b2g:master

Fixed error caused by incomplete function rename, carrying r=kgrandon.
Attachment #8583070 - Flags: review+
Attachment #8572596 - Attachment is obsolete: true
Keywords: checkin-needed
http://docs.taskcluster.net/tools/task-graph-inspector/#PGg84Rk5QryFO7hEZXyrpg

The pull request failed to pass integration tests. It could not be landed, please try again.
(In reply to Autolander from comment #31)
> http://docs.taskcluster.net/tools/task-graph-inspector/
> #PGg84Rk5QryFO7hEZXyrpg
> 
> The pull request failed to pass integration tests. It could not be landed,
> please try again.

Hmm, this is possibly related to infra issues? Investigating...
Saw that we're getting a weird clone error on TC. Let me try this once more and will monitor it.
Keywords: checkin-needed
http://docs.taskcluster.net/tools/task-graph-inspector/#4SAlWZ3IT-2SlsQda6Wctg

The pull request failed to pass integration tests. It could not be landed, please try again.
I am very confused here and will investigate, but let's land this manually for now: https://github.com/mozilla-b2g/gaia/commit/fdc700c05183edd93173a9cab57234b888e974c7

I'm wondering if some rebase is getting autolander into a weird/confused state.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Comment on attachment 8583070 [details] [review]
[gaia] Cwiiis:bug1139303-optimise-edit-mode > mozilla-b2g:master

I know this is a late request for a patch of this size, so I won't be upset if this is rejected. On the other hand, I think it's a worthwhile patch (edit mode is considerably more responsive with it) and I think it's had a good amount of bake time now.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: Initiating edit mode can be janky/confusing, response during edit mode can also be janky/inconsistent. Scrolling while dragging is also much improved by this patch.
[Testing completed]: Homescreen is covered reasonably well with automated testing, also tested manually and been baking on master for a while now with no regressions reported.
[Risk to taking this patch] (and alternatives if risky): After this amount of bake time, I'd say low, but it's not the smallest of patches.
[String changes made]: None
Attachment #8583070 - Flags: approval-gaia-v2.2?
Attachment #8583070 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
removing Tif's NI (resolved bug)
Flags: needinfo?(tshakespeare)
You need to log in before you can comment on or make changes to this bug.