Closed Bug 1056770 Opened 11 years ago Closed 10 years ago

Implement transitions for back & forward

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::System::Browser Chrome
Platform:
x86_64
Linux
Type:
defect

Tracking

(tracking-b2g:backlog)

Status:
RESOLVED FIXED
Milestone:
2.2 S11 (1may)
Project Flags:
tracking-b2g backlog

People

(Reporter: benfrancis, Assigned: apastor)

References

Details

(Keywords: polish, Whiteboard: [systemsfe][p=2])

Attachments

(2 files, 1 obsolete file)

Screenshot - Bad UX /w patch
49.51 KB, image/png
Details
[gaia] albertopq:1056770-back-forward-animation2 > mozilla-b2g:master
46 bytes, text/x-github-pull-request
kgrandon
: review+
pcheng
: feedback+
Details | Review
The Rocketbar currently jumps from one size to anther when back and forward buttons appear and disappear. The spec asks for a transition from one size to another. See video mockup here: https://mozilla.app.box.com/s/sn2m5espkyupeyldpq91
Changing blocking bug since this doesn't block core back/forward functionality.
Blocks: browser-chrome-mvp
No longer blocks: 941174
blocking-b2g: --- → backlog
Keywords: polish
If we get to this, let's definitely request uplift. This one animation could have very serious risks to rocketbar performance for the collapse/expand animation, which is much more important than the back/forward animation. Since it's backlogged, I'm moving to rocketbar-next so we can stay focused on the more important tasks. I do hope to get to this in 2.1 though, assuming we get the less risky things done first.
Blocks: rocketbar-next
No longer blocks: browser-chrome-mvp
Priority: -- → P1
blocking-b2g: backlog → ---
tracking-b2g: --- → backlog
Assignee: nobody → apastor
Comment on attachment 8590337 [details] [review] [gaia] albertopq:1056770-back-forward-animation > mozilla-b2g:master Eric, could you please review the animations? Thanks!
Attachment #8590337 - Flags: ui-review?(echang)
Comment on attachment 8590337 [details] [review] [gaia] albertopq:1056770-back-forward-animation > mozilla-b2g:master Sorry, typo.
Attachment #8590337 - Flags: ui-review?(echang) → ui-review?(epang)
Attachment #8590337 - Flags: review?(kgrandon)
(In reply to Alberto Pastor [:albertopq] from comment #5) > Comment on attachment 8590337 [details] [review] > [gaia] albertopq:1056770-back-forward-animation > mozilla-b2g:master > > Sorry, typo. Hey Alberto, Looks much better already but I'm guessing it's not possible to only expand the left edge of the rocket bar? Cause right now it's a little strange how it jumps right and left a little. Just going to hold off reviewing till I understand the reasoning.
Flags: needinfo?(apastor)
Comment on attachment 8590337 [details] [review] [gaia] albertopq:1056770-back-forward-animation > mozilla-b2g:master Much better then what we currently have though not the exact transition that has been designed. I've talked to Alberto and understand the reasoning is because of performance. R+ since this is already a huge improvement!
Attachment #8590337 - Flags: ui-review?(epang) → ui-review+
Thanks Eric¡
Flags: needinfo?(apastor)
Comment on attachment 8590337 [details] [review] [gaia] albertopq:1056770-back-forward-animation > mozilla-b2g:master I will take a quick pass at the code now, but there are at least two failing tests (gij 14 and 15). Please address those tests and re-flag me for review. Thanks!
Flags: needinfo?(apastor)
Attachment #8590337 - Flags: review?(kgrandon)
Flags: needinfo?(apastor)
Attachment #8590337 - Flags: review?(kgrandon)
Comment on attachment 8590337 [details] [review] [gaia] albertopq:1056770-back-forward-animation > mozilla-b2g:master Hi Alberto - The code looks good, but I believe there's a slight visual regression with the rocketbar positioning. It's most easily seen on the private browser page, due the the darkness of the chrome. Expected result: Search input and app chrome input should be aligned as much as possible. Actual result: The inputs appear to have shifted with the patch applied, and you can see the app chrome input behind the overlay. The positioning looks off.
Attachment #8590337 - Flags: review?(kgrandon) → review-
Attachment #8590337 - Flags: review- → review?(kgrandon)
Comment on attachment 8590337 [details] [review] [gaia] albertopq:1056770-back-forward-animation > mozilla-b2g:master This looks good to me, thank you!
Attachment #8590337 - Flags: review?(kgrandon) → review+
Keywords: checkin-needed
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Having caused two bugs, I think it makes sense to back this out, and try to get it re-landed with those bugs fixed. Master backout: https://github.com/mozilla-b2g/gaia/commit/e66a1660bf82e95c9b5e3808def26204d4ed9274
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: --- → 2.2 S11 (1may)
Comment on attachment 8598512 [details] [review] [gaia] albertopq:1056770-back-forward-animation2 > mozilla-b2g:master Same patch, with bug 1156899 fixed. I cannot repro bug 1156911 (that would explain it was reopened again after this bug was backed out). :piwei, could you please confirm that 1156911 and 1156899 are fixed with this patch? Thanks!
Attachment #8598512 - Flags: feedback?(pcheng)
Comment on attachment 8598512 [details] [review] [gaia] albertopq:1056770-back-forward-animation2 > mozilla-b2g:master After the patch, I cannot reproduce bug 1156899, but I do reproduce bug 1156911. See screenshot: http://i.imgur.com/VP6KoZc.jpg
Attachment #8598512 - Flags: feedback?(pcheng) → feedback-
:piwei, which device are you using? What gecko build? Can you confirm that without the patch, bug 1156911 doesn't happen? Thanks!
Flags: needinfo?(pcheng)
I believe I was on 4/28 nightly Flame build, which has the following environmental variables: Device: Flame BuildID: 20150428010206 Gaia: 0636405f0844bf32451a375b2d61a2b16fe33348 Gecko: caf25344f73e Version: 40.0a1 (3.0 Master) I recall NOT reproducing bug 1156899 nor bug 1156911 BEFORE the patch. And after patch bug 1156911 reproduced.
Flags: needinfo?(pcheng)
Comment on attachment 8598512 [details] [review] [gaia] albertopq:1056770-back-forward-animation2 > mozilla-b2g:master I finally reproduced it. Would you mind to test again with latest patch? Thanks!
Attachment #8598512 - Flags: feedback- → feedback?(pcheng)
Comment on attachment 8598512 [details] [review] [gaia] albertopq:1056770-back-forward-animation2 > mozilla-b2g:master Before patch: No repro of bug 1156899 nor bug 1156911. After patch: No repro of bug 1156899 nor bug 1156911. I didn't notice anything else breaking either. This is tested on today's Flame nightly Device: Flame BuildID: 20150506010204 Gaia: 3e6fd1e0a478af2c95d09ce95c2c6de2de2fec14 Gecko: ba44099cbd07 Version: 40.0a1 (3.0 Master)
Attachment #8598512 - Flags: feedback?(pcheng) → feedback+
Attachment #8590337 - Attachment is obsolete: true
Comment on attachment 8598512 [details] [review] [gaia] albertopq:1056770-back-forward-animation2 > mozilla-b2g:master It was already r+, but can you take a second look now that the 2 regressions have been fixed? Thanks!
Attachment #8598512 - Flags: review?(kgrandon)
Comment on attachment 8598512 [details] [review] [gaia] albertopq:1056770-back-forward-animation2 > mozilla-b2g:master I tested and it seems to be working great for me. Thank you.
Attachment #8598512 - Flags: review?(kgrandon) → review+
Keywords: checkin-needed
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/pull/29765 Autolander could not land the pull request due to not having collaborator rights. This is possibly due to a tree closure. Please check the tree status and request checkin again once the tree is open.
Let's try again: checkin-needed
Keywords: checkin-needed
Keywords: checkin-needed
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: