857105 - Keep the tapped item highlighted while the panel loads

Status: RESOLVED FIXED

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

Description

[Etienne Segonzac (:etienne)]

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!

Comment 1

[Etienne Segonzac (:etienne)]

I have a patch, will upload later.

Comment 2

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal

Comment 3

[Anthony Ricaud (:rik)]

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".

Comment 4

[Etienne Segonzac (:etienne)]

Attachment: Patch v2

Something like that?

Comment 5

[Fabien Cazenave [:kaze]]

É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?

Comment 6

[Etienne Segonzac (:etienne)]

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 :/

Comment 7

[Josh Carpenter [:jcarpenter]]

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.

Comment 8

[Etienne Segonzac (:etienne)]

(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.

Comment 9

[Josh Carpenter [:jcarpenter]]

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.

Comment 10

[Fabien Cazenave [:kaze]]

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.

Comment 11

[Etienne Segonzac (:etienne)]

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

Comment 12

[Anthony Ricaud (:rik)]

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).

Comment 13

[Anthony Ricaud (:rik)]

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?

Comment 14

[Fabien Cazenave [:kaze]]

Yup, a custom event sounds good.

Comment 15

[Anthony Ricaud (:rik)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9991

Pointer to Github pull-request

Comment 16

[Fabien Cazenave [:kaze]]

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 17

[Anthony Ricaud (:rik)]

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 18

[Fabien Cazenave [:kaze]]

Comment on attachment 753843 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9991

Thanks for the good work!

Comment 19

[Anthony Ricaud (:rik)]

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

Comment 20

[Anthony Ricaud (:rik)]

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

Comment 21

[José Antonio Olivera Ortega [:jaoo]]

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.

Comment 23

[José Antonio Olivera Ortega [:jaoo]]

: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).

Comment 24

[Anthony Ricaud (:rik)]

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.

Comment 25

[Anthony Ricaud (:rik)]

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.

Comment 26

[John Ford [:jhford] CET/CEST Berlin Time]

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

Comment 27

[Anthony Ricaud (:rik)]

Uplifted with bcf2943f8e10bdccc9697212098f783621550cec.

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

1.1hd: bcf2943f8e10bdccc9697212098f783621550cec

Comment 29

[Lucas Aliaga]

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