If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Keep the tapped item highlighted while the panel loads

RESOLVED FIXED

Status

Firefox OS
Gaia::Settings
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: etienne, Assigned: rik)

Tracking

(Blocks: 1 bug)

unspecified
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18? verified)

Details

(Whiteboard: u=fx-os-user c=scravag-sprint p=1)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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!
(Reporter)

Comment 1

5 years ago
I have a patch, will upload later.
Assignee: nobody → etienne
(Reporter)

Comment 2

5 years ago
Created attachment 733418 [details] [diff] [review]
Patch proposal
Attachment #733418 - Flags: review?(kaze)
(Assignee)

Comment 3

5 years ago
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".
(Reporter)

Comment 4

5 years ago
Created attachment 733910 [details] [diff] [review]
Patch v2

Something like that?
Attachment #733418 - Attachment is obsolete: true
Attachment #733418 - Flags: review?(kaze)
Attachment #733910 - Flags: review?(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?
(Reporter)

Comment 6

5 years ago
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 :/
Blocks: 856708
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.
(Reporter)

Comment 8

5 years ago
(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.
(Reporter)

Comment 11

5 years ago
I think vingtetun has a poc with a declarative syntax :)
Flags: needinfo?(21)
(Assignee)

Comment 12

5 years ago
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).
Flags: needinfo?(21)
(Assignee)

Updated

5 years ago
Attachment #733910 - Flags: review?(kaze)
(Reporter)

Updated

5 years ago
Assignee: etienne → anthony
tracking-b2g18: --- → ?
Whiteboard: u=fx-os-user c=may-6-17 p=0
(Assignee)

Updated

4 years ago
Attachment #733910 - Attachment is obsolete: true
Whiteboard: u=fx-os-user c=may-6-17 p=0 → u=fx-os-user c=may-6-17 p=1
Blocks: 874441
Whiteboard: u=fx-os-user c=may-6-17 p=1 → u=fx-os-user c=scravag-sprint-may-20-31 p=1
Whiteboard: u=fx-os-user c=scravag-sprint-may-20-31 p=1 → u=fx-os-user c=scravag-sprint p=1
(Assignee)

Comment 13

4 years ago
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.
(Assignee)

Comment 15

4 years ago
Created attachment 753843 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9991

Pointer to Github pull-request
(Assignee)

Updated

4 years ago
Attachment #753843 - Flags: review?(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?
Attachment #753843 - Flags: review?(josea.olivera)
Blocks: 876350
(Assignee)

Comment 17

4 years ago
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.
Attachment #753843 - Flags: review?(josea.olivera) → review+
Summary: Keep the item taped highlighted while the panel loads → Keep the tapped item highlighted while the panel loads
Comment on attachment 753843 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9991

Thanks for the good work!
Attachment #753843 - Flags: review?(kaze) → review+
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 19

4 years ago
https://github.com/mozilla-b2g/gaia/commit/eb6417144696335108a7fb1700ff2cfb57d6bf45 on master
(Assignee)

Comment 20

4 years ago
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
Keywords: qawanted

Updated

4 years ago
Keywords: qawanted → verifyme
Blocks: 879773

Updated

4 years ago
No longer blocks: 879773
Depends on: 879773
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.
Flags: needinfo?(anthony)
Duplicate of this bug: 863537
: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).
(Assignee)

Comment 24

4 years ago
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.
Flags: needinfo?(anthony)
(Assignee)

Comment 25

4 years ago
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.
blocking-b2g: --- → leo?

Updated

4 years ago
blocking-b2g: leo? → leo+
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
Flags: needinfo?(anthony)
(Assignee)

Comment 27

4 years ago
Uplifted with bcf2943f8e10bdccc9697212098f783621550cec.
status-b2g18: --- → fixed
Flags: needinfo?(anthony)
1.1hd: bcf2943f8e10bdccc9697212098f783621550cec
(Assignee)

Updated

4 years ago
Depends on: 889717
(Assignee)

Updated

4 years ago
Depends on: 869338

Comment 29

4 years ago
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
status-b2g18: fixed → verified
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.