Closed Bug 1009530 Opened 6 years ago Closed 6 years ago
[User Story] Visual to indicate when in edit mode
As a user, I want to be able to clearly tell when I am in edit mode on the homescreen so that I know to leave the edit state when finished. Acceptance Criteria: 1. When in edit mode, there is a visual indication to denote this, per UX spec. 2. After exiting edit mode as described by UX spec, visual indication of edit mode is no longer displayed.
1.06 MB, image/jpeg
2.49 MB, video/quicktime
46 bytes, text/x-github-pull-request
|Details | Review|
670.27 KB, image/jpeg
729.98 KB, image/jpeg
46 bytes, text/x-github-pull-request
|Details | Review|
No description provided.
We need to do a performance investigation here to see if there is some way to have the icons pulse. Either by scaling them up and down, or an opacity pulse. We should check memory and CPU usage, and ensure this does not cause additional jank during a scroll or dragdrop operation.
No longer blocks: 1015335
Hey Chris - are you currently looking at this one?
Assignee: kgrandon → nobody
I am, yes - current progress is I have the icons pulsing in edit mode with poor performance and I'm working on improving performance (with promising initial results). I should have something to show for it on Monday, after which perhaps we can make the decision on whether we want to go with a static design or not.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Here's my current work. This patch currently breaks the zoomed out view and has serious performance issues when dragging icons (a fix for this is blocked by a layout bug with web components, but I'll see if I can find other fixes). I've not actually seen the UX spec, so this is only based on what I've been told. This implements a pulsing opacity effect to indicate icons can be edited. For performance reasons, this stops during scrolling and during dragging. Alongside the problems mentioned in the first paragraph, edit mode transitions also cause greater checkerboarding at the start of scrolls - this may be mitigated by optimisation or better display-port heuristics. Worth mentioning, I've disabled progressive rendering and low-precision rendering locally as they seem to cause a number of visible (but transient) glitches.
Un-break dragging at the 3-across zoom level. Performance still terrible.
Attachment #8432626 - Attachment is obsolete: true
Ok, this fixes the performance issues with editing and layout issues with the zoomed out view. This will perform better when https://bugzilla.mozilla.org/show_bug.cgi?id=1010742#c4 is fixed (and even better when the layout bug that stops me changing the overflow of a shadow-dom element is fixed) This seems to have regressed high-dpi icon drawing in some cases though, it seems.
Attachment #8432642 - Attachment is obsolete: true
When testing this patch, please add this to your user-prefs via the edit-prefs script: user_pref("layers.progressive-paint", false); user_pref("layers.low-precision-buffer", false); This most recent patch I think is a reasonable indicator of what we can expect for performance - fixing the comment mentioned will markedly improve some of the pauses experienced when rearranging icons. I'd say scrolling performance is decent, but I wish it was better. Going with a design that isn't animated will help with initial scroll performance, but the issues with edit mode will remain. I think it's (just about) realistic to include this effect, but not including it may save us some maintenance hassle. I'm inclined to say that we should have it and it'll help make sure we don't lose track of some of the performance issues we have in the platform that cause this to be sub-optimal. There are still things we can do Gaia-side to improve the performance, but the biggest issues lie in layout and gfx. In particular, the rendering load caused by a large displayport is particularly a problem here, and there are many rendering errors when progressive tile painting is enabled.
I've pushed my most recent changes to github: https://github.com/Cwiiis/gaia/commit/1ba5f898d56acf4da193ab648f02886f565e5194 I spoke too soon in my e-mail, this still shows rendering glitches with progressive paint, but that's really nothing to do with the visual feedback (even if that was disabled, you would still see those glitches - they almost certainly happen on the current vertical homescreen after moving icons too). This shows good performance for me, when moving icons, when animating the icons before moving, etc. There's still plenty of opportunity to optimise too.
The diff against the branch point, for convenience.
Attachment #8432722 - Attachment is obsolete: true
Hi Chris/Kevin, Please see the two attachments for 'Plan A' and 'Plan B'. We can do Plan B, which simply omits the animation part, if we can't work out the jank with the opacity pulse on Flame.
Chris, here is an animation showing the pulse speed at 0.8 sec duration for a cycle: 65% > 100% > 65%.
Sounds good. I took a quick look at the current opacity pulse today. It's very slick, nice work Chris. I think it does uncover a few underlying platform graphics problems, or maybe my prefs are setup are wrong (I noticed large chunks of the screen disappearing at times). And at other times icons appear to slightly jump when the animation starts/stops. The fallback does seem pretty trivial to implement. I suppose at this point I would be ok with shooting for the pulsing, and if we run into perf problems, we can do a quick fallback implementation.
(In reply to Chris Lord [:cwiiis] from comment #10) > I spoke too soon in my e-mail, this still shows rendering glitches with > progressive paint So I don't lose track of this, could you file a bug with STR and make it block bug 993473?
Comment on attachment 8435068 [details] [review] Visual edit-mode feedback Hey Chris - Thanks for the patch, this is awesome! I will give the code a good lookover on github, but I am currently unfortunately unable to give a R+ , mainly due to the scroll issues. It appears that this patch somehow breaks the scrolling of the search bar. Basically the search bar and icons should scroll under the statusbar as one continuous layer. Please take a look and let me know what you think.
Duplicate of this bug: 1021450
Tweaked so that the app grid is in-line with the search bar (matching prior behaviour). That was much harder than I thought it would be :p
New Edit Mode spec using edit mode header. Please note that the status bar will fade to black in this design upon invoking edit mode, and it will fade back to 15% opacity after exiting edit mode. Also note that the dark grey overlay that fades in on top of the wallpaper has the colour #333333, while the normal 5% wallpaper overlay is black in colour (#000000). So the edit mode background can either be a new layer or the same one but it shifts colour. Please see next attachment for transition spec.
Attachment #8433794 - Attachment is obsolete: true
Transition spec for entering/exiting edit mode.
Comment on attachment 8435880 [details] [review] Visual edit-mode feedback v2 Thank you so much for the patch. Unfortunately due to the platform limitations we won't be able to use this in 2.0. Let's definitely keep an eye on the platform and try to implement this down the road. There are a variety of issues filed against meta bug 1017954, and once solved we should be able to implement this.
Going to take to implement edit mode. Thanks for the hard work on this Chris.
Assignee: chrislord.net → kgrandon
(In reply to Kevin Grandon :kgrandon from comment #29) > Comment on attachment 8435880 [details] [review] > Visual edit-mode feedback v2 > > Thank you so much for the patch. Unfortunately due to the platform > limitations we won't be able to use this in 2.0. Let's definitely keep an > eye on the platform and try to implement this down the road. > > There are a variety of issues filed against meta bug 1017954, and once > solved we should be able to implement this. Fair enough - even if you disable the animation, there are some performance improvements there that I think we will want, especially wrt dragging icons around. At the very least, the fixing of the from/to parameters in GridView.render gives a nice boost to responsiveness.
Yeah, I would definitely love to get some of your drag drop improvements in. Would you be willing to split them out and into a patch? This patch isn't going to touch dragdrop too much, so I wanted to avoid doing that here.
Hey, would either of you guys be able to review this? Thanks!
erm, I don't really know how Gaia reviews work, but I've added review comments to the pull request :)
Comment on attachment 8436358 [details] [review] Pull request - Simple edit mode (In reply to Chris Lord [:cwiiis] from comment #34) > erm, I don't really know how Gaia reviews work, but I've added review > comments to the pull request :) Thanks Chris! :) Comments on github are usually fine. If there's any blocking issues, then I'll generally just note so in the bug and clear or R- the attachment. If they're just nits then I'll generally leave an R+ and trust that the patch author will update as necessary. I'll go ahead and update the nits on github, mark this as R+. Thanks!
Landed for now: https://github.com/mozilla-b2g/gaia/commit/9fa521957796c1b8c95b1a86aa0e9a400778851a Going to close this one, but let's open a new bug to get your dragdrop improvements in.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.
You need to log in before you can comment on or make changes to this bug.