We have some panels in the Settings app that requires a few hundred milliseconds to load. And we can't start the panel transition before the next panel is loaded (because of how the lazy load works). So to the user it's: - I tap an item in the list - see it highlighted (by the :active) for a very short time if I'm lucky - wait 200ms with no idea if my tap was registered properly - then see the transition Vivien astutely observed that iOS mitigate this issue by keeping the list item highlighted so the user at least knows that the tap was registered. We should do this!

I have a patch, will upload later.

Comment on attachment 733418 [details] [diff] [review] Patch proposal Review of attachment 733418 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/settings/js/settings.js @@ +814,5 @@ > + panel.addEventListener('transitionend', function trWait() { > + trCount++; > + > + // We get 2 transitionend because of the .peek hack > + if (trCount >= 2) { Instead of counting the number of transitionend, I'd prefer if we listened to an event saying "we're done transitioning panels".

Something like that?

Étienne, your code makes sense but I’m so sad to use JS for such a bug that I’m trying to find a declarative way atm. (In reply to Etienne Segonzac (:etienne) from comment #0) > - I tap an item in the list > - see it highlighted (by the :active) for a very short time if I'm lucky When an item is tapped, there’s a significant delay before it gets highlighted. Same thing for the “back” buttons in sub-panels. I have a feeling that it used to be much better… any reason why it could have regressed?

Just to write it down somewhere: from our experiments the time between the click event and the hashchange event is already 120ms, so no panel can load faster than that... Concerning the bug, a declarative solution would make me happier too :) But if we can't come up with an easy upliftable declarative fix an imperative one beats no fix at all because the status quo is pretty bad :/

Nice! Funny, we must have a Canadian-French psychic connection. I just wrote the following in bug #856708 (meta bug for Settings performance improvement): > 5. Task completion (opening a subsection) as soon as possible after > touch end. > > ...Or to add a bit more nuance: Task completion asap after touch > end, ALBEIT after a N ms wait for the button :active state to clear. > We want to establish a rhythm to the FFOS interface. Usually it > means making a response happen _faster_. But in some cases we want > to throttle the response slightly so we can make sure a button :active > state can play out it's cycle (to pick one example). iOS does this > nicely, defining a disciplined timing consistency across the OS, even > when it seems likely their HW could load content a few ms faster > than they actually are. UX has not yet defined these timing > thresholds within FFOS (our focus has been on responsiveness) but > we'll add it to the docket. So keeping the button hit state alive for a bit longer in situations like these makes sense, and should be added to a comprehensive system-wide "animation timing" doc that UX produces.

(In reply to Josh Carpenter [:jcarpenter] from comment #7) > Nice! Funny, we must have a Canadian-French psychic connection. I can already smell the fernet-cola.

This bug implements a persistent highlight, but what are we doing to address the elated issue: the fact that there's such a massive delay in displaying button :active states? That's really the core issue.

I can’t agree more, though we obviously need to address these three issues: 1/ the :active (or :focus) state should be immediate; 2/ the highlight should be persistent until the sub-panel is displayed; 3/ the sub-panel should come quickly. Regarding 1/, I wonder if it isn’t a regression — maybe our HTML structure or CSS selectors changed in the last few weeks? Note that this delay can also be seen with the “back” buttons in all sub-panels (= those tiny little arrows in the headers). Regarding 2/ (= current bug), I’d like to see if we can’t have a declarative way to do that — i.e. using :focus selectors and `tabindex=0' attributes. Regarding 3/, we realized that there’s ~120ms between the `onclick' and `onhashchange' events. Something should be done on the Gecko side about that, but it probably means we should not use plain links (<a href="…"/>) and rely on JS routing instead. This bug is high on my list but it’s not a TEF+ one… (hint, hint) (In reply to Etienne Segonzac (:etienne) from comment #8) > (In reply to Josh Carpenter [:jcarpenter] from comment #7) > > Nice! Funny, we must have a Canadian-French psychic connection. > I can already smell the fernet-cola. I recently ate the inner of a wizard, turns out they’re actually made of Fernet.

I think vingtetun has a poc with a declarative syntax :)

We looked at this with Vivien and the :focus (+ tabindex if it's not a link) works in the general case. So that's a very good news, we can use this across the OS whenever we need quick feedback. On settings, we see :focus being applied after :active but only for a very small amount of time. That's because when we click on an item, we navigate to the panel (with a hash) and that panel gets the focus. So I think we'll have to change the whole navigation system to not use hashes (because it is slow per comment 6 and because it steals the focus).

Here is my WIP patch https://github.com/Rik/gaia/compare/keep-item-focused-857105 The last thing I need to deal with is for the "Call settings" panel. It's using hashchange to update the UI when you enter that panel. I think emitting an event every time we enter a panel would be the right way to fix this. Any different idea?

Yup, a custom event sounds good.

Pointer to Github pull-request

Comment on attachment 753843 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9991 r=me for the Settings part with nit addressed. Jaoo, can you please have a look at the `call.js' part?

Comment on attachment 753843 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9991 Carrying r+ over IRC from Jaoo. Kaze, I addressed your comments and fixed an issue with some panels. See the new commit in the PR.

Comment on attachment 753843 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9991 Thanks for the good work!

https://github.com/mozilla-b2g/gaia/commit/eb6417144696335108a7fb1700ff2cfb57d6bf45 on master

For QA: This patch has rewritten a lot of the logic to navigate through the panels. Given how many panels this app contains, I might have missed an edge-case. Things to check: - All panels can be opened and close - Transitions between panels should not have changed (we sometimes can't see the transition but that's not because of this patch) - Panels should still be functional

Guys, is there any plan for uplifting this bug to v1-train branch? Bug 876543 was fixed on top of the changes introduced here and now the uplift for bug 876543 has conflicts.

:rik, we need to uplift also bug 882057. As I commented in comment #21 before uplifting bug 876543 we need to know whether there is any plan for uplifting this bug. Bug 876543 somehow depends on this bug and we are having conflicts when uplifting it. The conflict could be resolved or uplift this bug (that way there wouldn't be such conflicts).

I was waiting on some feedback to see if that broke some things or not. A week later, bug 879773 was filed and resolved. I've also checked with Josh Carpenter if it's a good improvement and he agrees. So let's move this to v1-train.

Leo approval rationale: - This improves the feeling of responsiveness of the Settings app. - I expect a lot of commits in the Settings app to depend on this (we've already seen two). - It has been on master for two weeks so it feels safe to uplift now.

I was not able to uplift this bug to v1-train. If this bug has dependencies which are not marked in this bug, please comment on this bug. If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval. Otherwise, if this is just a merge conflict, you might be able to resolve it with: git checkout v1-train git cherry-pick -x eb6417144696335108a7fb1700ff2cfb57d6bf45 <RESOLVE MERGE CONFLICTS> git commit

Uplifted with bcf2943f8e10bdccc9697212098f783621550cec.

1.1hd: bcf2943f8e10bdccc9697212098f783621550cec

Verified fixed in latest Leo 1.1 on Leo device. The panels remain highlighted until the scene transitions in settings. - Verified Fixed Leo 1.1 - Gaia 680f3b86b1e4ff1411ece6ba397b8b0e56b4b31c Gecko 4bfd6a51cd05 BuildID 20131015041203 Version 18.0