Closed
Bug 1377433
Opened 7 years ago
Closed 7 years ago
Should not do the sliding-up tour notification transition
Categories
(Firefox :: General, enhancement, P1)
Tracking
()
VERIFIED
FIXED
Firefox 56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: Fischer, Assigned: Fischer)
References
Details
(Whiteboard: [photon-onboarding])
Attachments
(1 file)
After discussion with UX team, we don't want the sliding-up tour notification transition any more. But the sliding-down transition should still be kept.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [photon-onboarding]
Target Milestone: --- → Firefox 57
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Flags: qe-verify+
QA Contact: jwilliams
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8882544 [details]
Bug 1377433 - Should not do the sliding-up tour notification transition,
https://reviewboard.mozilla.org/r/153660/#review158816
::: browser/extensions/onboarding/content/onboarding.css:349
(Diff revision 2)
> width: 100%;
> height: 122px;
> min-width: 640px;
> background: rgba(255, 255, 255, 0.97);
> border-top: 2px solid #e9e9e9;
> transition: transform 0.8s;
Why is this not removed?
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #3)
> Comment on attachment 8882544 [details]
> Bug 1377433 - Should not do the sliding-up tour notification transition,
>
> https://reviewboard.mozilla.org/r/153660/#review158816
>
> ::: browser/extensions/onboarding/content/onboarding.css:349
> (Diff revision 2)
> > width: 100%;
> > height: 122px;
> > min-width: 640px;
> > background: rgba(255, 255, 255, 0.97);
> > border-top: 2px solid #e9e9e9;
> > transition: transform 0.8s;
>
> Why is this not removed?
Because we still want the sliding-down transition so
- with .onboarding-opened, it would be `transition: none` and no siding-up transition
- without .onboarding-opened, it would be `transition: transform 0.8s` and there would be a siding-down transition
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8882544 [details]
Bug 1377433 - Should not do the sliding-up tour notification transition,
https://reviewboard.mozilla.org/r/153660/#review158824
Attachment #8882544 -
Flags: review?(dtownsend) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Fischer [:Fischer] from comment #6)
> Comment on attachment 8882544 [details]
> Bug 1377433 - Should not do the sliding-up tour notification transition,
>
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/153660/diff/2-3/
Just rebased on the latest central
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ee5f3f39677f
Should not do the sliding-up tour notification transition, r=mossop
Keywords: checkin-needed
Comment 9•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/d6ec0becb624 for Windows and Mac e10s permaorange in browser_aboutHome.js and browser_bug735471.js, https://treeherder.mozilla.org/logviewer.html#?job_id=111362014&repo=autoland
Given the screenshot showing the onboarding panel covering the buttons on the bottom of the page and what browser_aboutHome.js is doing, clicking the Sync button, it's hard to imagine why it would be e10s-only, why it wouldn't also be permaorange on Linux, and why it wasn't already permaorange, though. Was it racing to click the button before your transition could cover it up?
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Phil Ringnalda (:philor) from comment #9)
> Backed out in https://hg.mozilla.org/integration/autoland/rev/d6ec0becb624
> for Windows and Mac e10s permaorange in browser_aboutHome.js and
> browser_bug735471.js,
> https://treeherder.mozilla.org/logviewer.html#?job_id=111362014&repo=autoland
>
> Given the screenshot showing the onboarding panel covering the buttons on
> the bottom of the page and what browser_aboutHome.js is doing, clicking the
> Sync button, it's hard to imagine why it would be e10s-only, why it wouldn't
> also be permaorange on Linux, and why it wasn't already permaorange, though.
> Was it racing to click the button before your transition could cover it up?
Thanks, yeah it seems that because the onboarding notification bar in the bottom would cover the buttons in the bottom of about:home. As a result, failed to trigger those buttons. A simple solution of disabling the onboarding notification bar is added (since the onboarding notification is not the test target). Running try to see the solution: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6efea4461b484760705bb5ff7909aed67af73145&selectedJob=112452646
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
Comment on attachment 8882544 [details]
Bug 1377433 - Should not do the sliding-up tour notification transition,
Hi Mossop,
Would you please look at the patch again, thanks.
Run try[3] on Mac and Window for 10x times and the results are good.
The only updates are [1] and [2] to fix the browser_aboutHome.js failure. The failure reason is as the comment 11.
And for the browser_bug735471.js failure, after looking into the test, the test shouldn't relate to our onboarding tours because it simply loads an about:newtab then calling the `openPreferences` utility api to open about:preferences. From the try results, didn't see any browser_bug735471.js failure either so didn't the test for now.
[1] browser/base/content/test/general/browser_aboutHome.js
[2] browser/base/content/test/general/head.js
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=344ae8584f3360c6434ffc3d12a95f278b3901d6&selectedJob=112769606
Attachment #8882544 -
Flags: review+ → review?(dtownsend)
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8882544 [details]
Bug 1377433 - Should not do the sliding-up tour notification transition,
https://reviewboard.mozilla.org/r/153660/#review160896
Attachment #8882544 -
Flags: review?(dtownsend) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 15•7 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3dfb4c796b7b
Should not do the sliding-up tour notification transition, r=mossop
Keywords: checkin-needed
Comment 16•7 years ago
|
||
bugherder |
Updated•7 years ago
|
Target Milestone: Firefox 57 → Firefox 56
Comment 17•7 years ago
|
||
I have verified that this bug is fixed with today's nightly.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•