Should not do the sliding-up tour notification transition

VERIFIED FIXED in Firefox 56

Status

()

Firefox
General
P1
normal
VERIFIED FIXED
2 months ago
16 days ago

People

(Reporter: Fischer, Assigned: Fischer)

Tracking

55 Branch
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [photon-onboarding])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 months ago
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

2 months 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

2 months ago
Flags: qe-verify+
QA Contact: jwilliams

Comment 3

2 months 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

2 months 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

2 months 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

2 months 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

Comment 8

2 months ago
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
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)

Updated

2 months ago
Duplicate of this bug: 1379042

Updated

2 months ago
Depends on: 1378685

Updated

2 months ago
Depends on: 1357641
(Assignee)

Comment 11

2 months 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

Updated

2 months ago
See Also: → bug 1377496
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 months 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

2 months 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

a month ago
Keywords: checkin-needed

Comment 15

a month 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

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3dfb4c796b7b
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Depends on: 1380291
Target Milestone: Firefox 57 → Firefox 56
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.