Closed Bug 1125601 Opened 9 years ago Closed 9 years ago

[Messages] It's impossible to immediately navigate to Thread or Composer once app is loaded

Categories

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.2+, b2g-v2.0 unaffected, b2g-v2.1 affected, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S9 (3apr)
blocking-b2g 2.2+
Tracking Status
b2g-v2.0 --- unaffected
b2g-v2.1 --- affected
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: azasypkin, Assigned: gduan)

References

()

Details

(Keywords: regression)

Attachments

(1 file)

It's impossible to immediately navigate to Thread or Composer once app is loaded, user has to wait for some time before Thread or Composer links become actionable.

In v2.0 user can tap on Thread/Composer as soon as he sees it and user will navigate there once first page of thread is loaded. It's not the case in v2.1 anymore.

See video: https://www.youtube.com/watch?v=RkOSXx7vXdg&feature=youtu.be
Hey Julien, is what I see in video from comment 0 is the same we've discussed on IRC or you see another issue?

Thanks!
Flags: needinfo?(felash)
Yeah, that's exactly it, thanks !

[Blocking Requested - why for this release]: regression from 2.0, frustrating usability issue (I know because I dogfood 2.1 for 2 weeks), likely small fix.
blocking-b2g: --- → 2.1?
Flags: needinfo?(felash)
triage: regression
blocking-b2g: 2.1? → 2.1+
QA Contact: pcheng
Unable to find a regression window due to this is most likely a performance issue. I've concluded from repro'ing locally as well as from the demonstrated video that when tapping on a thread or composer BEFORE the Rocketbar/status bar gets loaded, the issue will reproduce, otherwise it will NOT repro. So technically the app is NOT fully loaded in this scenario.

As I go back further to older builds the repro rate drops significantly, and it's still dependent on the loading time of rocketbar/status bar when I cold launch the SMS app. In this case I don't believe I can find an accurate window. I've spent two days on this bug and I think I've explored all possibilities on finding the window before calling it quits.
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Thanks a lot Pi Wei, what you suggest implies that our issue is not in the SMS app but in Gaia::System.

I'll try to add more logs in the SMS app to clarify this. And possibly find a better STR if I need you again :)
Flags: needinfo?(felash)
One difference I found between 2.0 and 2.1+ version is the status bar with search. The status bar seems not need to update when entering app in 2.0, but it seems much more action and relayout in status bar after 2.1. All the action seems disabled before status bar ready. Other apps don't have this issue because panel rendering start after status bar ready. I think it's related to DOMContentLoaded we are listening for starting earlier. If we use load event instead me can make sure the all the action is accessible. So there are 2 posibilities:

- new statusbar delay the load time, and it might delay longer if we also doing more things right after DOMContentLoaded.
- system prevent all pointer events before loaded, but Alive said it's implemented in 1.4 or 2.0 ni? Alive for more details.
Flags: needinfo?(alive)
(In reply to Steve Chung [:steveck] from comment #6)
> One difference I found between 2.0 and 2.1+ version is the status bar with
> search. The status bar seems not need to update when entering app in 2.0,
> but it seems much more action and relayout in status bar after 2.1. All the
> action seems disabled before status bar ready. Other apps don't have this
> issue because panel rendering start after status bar ready. I think it's
> related to DOMContentLoaded we are listening for starting earlier. If we use
> load event instead me can make sure the all the action is accessible. So
> there are 2 posibilities:
> 
> - new statusbar delay the load time, and it might delay longer if we also
> doing more things right after DOMContentLoaded.
> - system prevent all pointer events before loaded, but Alive said it's
> implemented in 1.4 or 2.0 ni? Alive for more details.

We delayed the opened event until loaded in bug 950673; and we are preventing pointer events while the opening transition is still there.

Not sure how to fix this gently, what if we just remove 'transition-opening' after opening timeout but not dispatch the appopened event? Do you think this will cause other problem?
Flags: needinfo?(alive) → needinfo?(etienne)
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #7)
> Not sure how to fix this gently, what if we just remove 'transition-opening'
> after opening timeout but not dispatch the appopened event? Do you think
> this will cause other problem?

I'll rephrase just to be sure we're talking about the same thing:
removing the `.transition-opening` class as soon as we get the `animationend` without waiting to reach `handle_opened` makes perfect sense. It might register badly on the app launch time stats but I think we can make the argument we should do it anyway because of the improved perceived performance (like in this bug).

So let's do this, I'm cc'ing Eli just for the heads up and also we might want to do the same for the `transition-closing` for symmetry's sake.
Flags: needinfo?(etienne)
Flags: needinfo?(felash)
Component: Gaia::SMS → Gaia::System::Window Mgmt
Confirmed with the device group that this bug doesn't block any device launch. We are not tagging it with 2.0+ or 2.1+. Given the timing we are in for 2.2, we suggest to mark this bug as blocking-b2g:2.2+. Thanks. 

Now, a problem is, who is the best one to look at this bug?
blocking-b2g: 2.1+ → 2.2+
Alive, do you reach a conclusion? Will you take this bug?
Flags: needinfo?(alive)
George, could you take this and do comment 7? Thanks.
Flags: needinfo?(alive) → needinfo?(gduan)
Assignee: nobody → gduan
Flags: needinfo?(gduan)
Comment on attachment 8582343 [details] [review]
[gaia] cctuan:1125601 > mozilla-b2g:master

Hi Etienne,

Besides removing transition-opening , we also need to remove enlarge class,
see 
https://github.com/cctuan/gaia/blob/1125601/apps/system/js/app_transition_controller.js#L185 
https://github.com/cctuan/gaia/blob/1125601/apps/system/style/window.css#L241

could you check my patch? I'm not sure if there's any side effect, but it works fine for this case.
Thanks.
Attachment #8582343 - Flags: feedback?(etienne)
Comment on attachment 8582343 [details] [review]
[gaia] cctuan:1125601 > mozilla-b2g:master

This is not what Comment 7 was mentioning.
Calling |this.changeTransitionState('complete', evt.type);| here will
* make all the |_waitingForLoad| mechanic basically dead code
* have a real (bad) impact on app launch time

I think we should just stick with the class removal (we can extract it in a `enableUserInteraction` method if there's more than one class to remove, but it should _not_ trigger an appopened event.

PS: of course if you have data that shows that the current patch doesn't impact app launchtime we should just remove all the |_waitingForLoad| mechanic too and clean it up, but sadly I think it's unlikely :/
Attachment #8582343 - Flags: feedback?(etienne) → feedback-
Comment on attachment 8582343 [details] [review]
[gaia] cctuan:1125601 > mozilla-b2g:master

Hi Etienne,
I agree with you. I think 
.appWindow.enlarge .touch-blocker 
would be unnecessary because we already use transition-opening to prevent user touching and reset all the class on the same time, so I remove this line and remove transition-opening class while animationend, Please kindly check again, thanks!
Attachment #8582343 - Flags: feedback- → feedback?
Attachment #8582343 - Flags: feedback? → feedback?(etienne)
Comment on attachment 8582343 [details] [review]
[gaia] cctuan:1125601 > mozilla-b2g:master

Looks good, we should probably add a small comment explaining why we remove the class and some tests (obviously ;)).
Attachment #8582343 - Flags: feedback?(etienne) → feedback+
Comment on attachment 8582343 [details] [review]
[gaia] cctuan:1125601 > mozilla-b2g:master

Hi Etienne,
test and comment are added, could you check it again? Thanks.
Attachment #8582343 - Flags: review?(etienne)
Comment on attachment 8582343 [details] [review]
[gaia] cctuan:1125601 > mozilla-b2g:master

All good, thanks!
Attachment #8582343 - Flags: review?(etienne) → review+
thanks, 
master: https://github.com/mozilla-b2g/gaia/commit/ee1fba22db03606b8c01fdf947e3746a25a2da46
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 8582343 [details] [review]
[gaia] cctuan:1125601 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Not a regression.
[User impact] if declined: User interaction is blocked while launching sms app.
[Testing completed]: Yes, manual check and unit test.
[Risk to taking this patch] (and alternatives if risky): No.
[String changes made]:
Attachment #8582343 - Flags: approval-gaia-v2.2?
Because the bug exists in v2.1, maybe you can request approval on v2.1 as well?

Thanks !
Attachment #8582343 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
This issue is verified fixed on the latest Nightly Flame 3.0 and 2.2 builds.

Actual Results: It is possible to enter a thread or the composer as soon as the buttons can be seen. (20/20 tests between both builds)

Environmental Variables:
Device: Flame 3.0
BuildID: 20150403010203
Gaia: 7969b367a7da62877c3a24a26d3cb5fda89d766c
Gecko: 70a113676b21
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Environmental Variables:
Device: Flame 2.2
BuildID: 20150403002503
Gaia: 022eeb91197ba4a9adfd67bd6db5aa03cc69eb31
Gecko: 77fdc6fbcae9
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
Status: RESOLVED → VERIFIED
Flags: needinfo?(ktucker)
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: