Closed Bug 1090562 Opened 10 years ago Closed 10 years ago

[Rocketbar] Transitions are missing

Categories

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

x86
macOS
defect
Not set
normal

Tracking

(blocking-b2g:2.1+, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S8 (7Nov)
blocking-b2g 2.1+
Tracking Status
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: kgrandon, Assigned: kgrandon)

References

Details

(Keywords: regression, Whiteboard: [systemsfe])

Attachments

(3 files)

It appears that at some point we lost the rocketbar transitions that we're supposed to see entering the search app. This can be quite confusing for a user as you can see through the backdrop and see two search bars. It also looks quite bad for a new feature.

STR:
From an application with a minimized rocketbar, tap to enter search.

Expected result:
The rocketbar should expand into the full size.

Actual result:
The transition is missing and instead the search app immediately comes into focus.
I did a quick bisection, and here is the window I found. I'm not 100% sure it's accurate due to the overlay changes we made half-way through the release, but we should investigate.
git bisect start
git bisect bad fede7e11f3786f8944ec5e6b74d8c30f5790c326
git bisect good f33f82d7d97c36a1ff392ac41132319c2a0bf666

git bisect bad
Bisecting: 908 revisions left to test after this (roughly 10 steps)
[3ef7886e15f2be9ea6240e09335ab3a746858c41] Merge pull request #23542 from asutherland/bug1060922-email-sig-upgrade-null

git bisect bad
Bisecting: 453 revisions left to test after this (roughly 9 steps)
[a25ae14dbd2fe3e25144a7064efc0cc4e31042b8] Merge pull request #22075 from empoalp/bug_976678

git bisect good
Bisecting: 226 revisions left to test after this (roughly 8 steps)
[85cd1af363fc0dc3d38adff6ef3e1c6425dc6583] Merge pull request #23320 from wilsonpage/1015292

git bisect bad
Bisecting: 113 revisions left to test after this (roughly 7 steps)
[c4284d24db251582731424617d268394b9045310] Merge pull request #23332 from sfoster/close-apps-task-manager-bug-1047143

git bisect good
Bisecting: 57 revisions left to test after this (roughly 6 steps)
[5dbbd4ee09de929d6c388d19f128c902ac7d108f] Merge pull request #23246 from karanluthra/1057798-clean-up-mozl10n-api-in-email

git bisect bad
Bisecting: 28 revisions left to test after this (roughly 5 steps)
[acc1f9f1d7e37a3eb4dc818fc71af0e244a66bbc] Merge pull request #23347 from mnjul/revert_1024298

git bisect good
Bisecting: 14 revisions left to test after this (roughly 4 steps)
[68873f26ac29ef24b5ea3104c1b4574424277212] Merge pull request #23368 from yurenju/fix-space-for-profile-rule

git bisect good
Bisecting: 7 revisions left to test after this (roughly 3 steps)
[7f2100f5aa4fc2fd2dc4682ac722f29337f0d54e] Merge pull request #23378 from KevinGrandon/bug_1052503_progress_indicator_steps

git bisect bad
Bisecting: 2 revisions left to test after this (roughly 2 steps)
[d281d90c3679d513627e17c0f4dcf9e895c9b47d] Merge pull request #23351 from mozfreddyb/bug-1056882

git bisect bad
Bisecting: 1 revision left to test after this (roughly 1 step)
[3ffb2e095dd36a700fe7119f317c814be822f0a6] Merge pull request #23338 from KevinGrandon/bug_1054521_retain_search_input_apps

git bisect bad
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[1b33233dc9e579af45fe42252be51af3ca2d2fa7] Bug 1054521 - [Rocketbar] Persist search term for non-browser launches r=benfrancis

git bisect bad
1b33233dc9e579af45fe42252be51af3ca2d2fa7 is the first bad commit
commit 1b33233dc9e579af45fe42252be51af3ca2d2fa7
Author: Kevin Grandon <kevingrandon@yahoo.com>
Date:   Tue Aug 26 17:53:06 2014 -0700

    Bug 1054521 - [Rocketbar] Persist search term for non-browser launches r=benfrancis
Blocks: 1054521
Whiteboard: [systemsfe]
Attached image Screenshot of issue
Assignee: nobody → kgrandon
[Blocking Requested - why for this release]: Seems this is a pretty ugly, especially for a 2.1 showcase feature. I think we need to fix this.
Status: NEW → ASSIGNED
blocking-b2g: --- → 2.1?
Here's a WIP - unfortunately this has some other strange effects with the statusbar icons. For some reason they aren't shown again when exiting rocketbar - I need to investigate why.
Comment on attachment 8513174 [details] [review]
Pull request - add rocketbar animations again

Etienne - seems that I've broken the rocketbar transition before. I noticed that this might conflict with bug 1071114, so that patch is included here.

In order for the statusbar icons to properly update, we need to fire the chromecollapsed/expanded events in more places, so this moves that logic. We now always listen for transition events, so the statusbar is updated with the proper width.

If we do go with something like this, then I'll go ahead and remove the scroll safety timeout tests that I see you added as well.
Attachment #8513174 - Attachment description: WIP - pull request, missing tests → Pull request - add rocketbar animations again
Attachment #8513174 - Flags: review?(etienne)
Francis - can we get some UX opinion on whether or not this is a blocker? Being that BrowserOS is one of the primary 2.1 features, I don't think we should consider shipping with something this bad.
Flags: needinfo?(fdjabri)
I totally agree that this should be a blocker. As Kevin says, this is a showcase feature and it currently looks broken. The whole idea behind the rocket bar is that it should be a continuous object between different states, so that the user understands that it's the same thing everywhere they see it. Showing two rocket bars at the same time completely messes with this and could be very confusing. Something like this will badly effect the overall perceptions of quality of the product.
Flags: needinfo?(fdjabri)
Agree this is a blocker. Kevin, do you think you'll have a fix in time for 2.1?
(In reply to Stephany Wilkes from comment #9)
> Agree this is a blocker. Kevin, do you think you'll have a fix in time for
> 2.1?

Yes, we should be able to get a fix in this week.
Broken feature.
blocking-b2g: 2.1? → 2.1+
Target Milestone: --- → 2.1 S8 (7Nov)
Depends on: 1071114
Comment on attachment 8513174 [details] [review]
Pull request - add rocketbar animations again

Oh, I saw it long ago and I thought it was a UX decision... should have double-checked :)

Anyway, glad we're putting it back in!
Love the app_chrome.js change, some comments on the rocketbar.js change.

Also, when you open the rocketbar from a fullscreen app we have the same issue with missing border-radius during the transition. So we need the same fix than [1] for fullscreen apps  (but with a black background).

And I can see the reload button when opening the rocketbar from the calendar (I think they have location changes at launch).

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/style/chrome/chrome.css#L127-129


> If we do go with something like this, then I'll go ahead and remove the
> scroll safety timeout tests that I see you added as well.

As long as we keep the one about not publishing anything when a transitionend for the controls bubbles.
Attachment #8513174 - Flags: review?(etienne)
Comment on attachment 8513174 [details] [review]
Pull request - add rocketbar animations again

Hey Etienne - could you take a look?

This does unfortunately not fix fullscreen apps 100% yet, but since we're running out of time, I suggest landing this without fullscreen apps being 100% supported, and we can fix those later. Let me know what you think. Thanks!
Attachment #8513174 - Flags: review?(etienne)
Comment on attachment 8513174 [details] [review]
Pull request - add rocketbar animations again

Comments on github, tests and a small logic error.
Enough to warrant a last review round but should be quick :)
Attachment #8513174 - Flags: review?(etienne)
Comment on attachment 8513174 [details] [review]
Pull request - add rocketbar animations again

Hey Etienne - thanks for the great review. I've fixed the logic, added an app chrome test, and restored the callOrder tests. Let me know what you think.
Attachment #8513174 - Flags: review?(etienne)
Comment on attachment 8513174 [details] [review]
Pull request - add rocketbar animations again

Awesome, thanks!
Attachment #8513174 - Flags: review?(etienne) → review+
In master: https://github.com/mozilla-b2g/gaia/commit/311d7dee72b0dcadc1e0911985bf774edfbfeea4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment on attachment 8513174 [details] [review]
Pull request - add rocketbar animations again

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature implementation.
[User impact] if declined: Users will have a poor experience using the rocketbar, a new 2.1 feature.
[Testing completed]: Manual and unit tests.
[Risk to taking this patch] (and alternatives if risky): Fairly low risk as it's already broken.
[String changes made]: None.
Attachment #8513174 - Flags: approval-gaia-v2.1?(bbajaj)
Attachment #8513174 - Flags: approval-gaia-v2.1?(bbajaj) → approval-gaia-v2.1+
Seems this needs a branch patch. I'm working on one now.
This issue is verified fixed on Flame 2.1 and 2.2.

Result: The rocket bar expands with the animation properly when the user taps the minimized rocket bar. Tested with Dialer, Messages, Contacts, Settings, Email, and Calendar app.

Device: Flame 2.1 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141105001204
Gaia: 154da5e17029a51002d5d9b7df39563d509edde6
Gecko: 3b0c3580a58d
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1) 
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2 Master (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141105040206
Gaia: 7c9e7cabbde941b976e0e40a3a1d94e21aa9c5e9
Gecko: 62990ec7ad78
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 36.0a1 (2.2) 
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: