Should not do the sliding-up tour notification transition

VERIFIED FIXED in Firefox 56

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years 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])

Attachments

(1 attachment)

(Assignee)

Description

2 years 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 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)
Flags: qe-verify+
QA Contact: jwilliams

Comment 3

2 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

2 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

2 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

2 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

Comment 8

2 years 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 years ago
Duplicate of this bug: 1379042

Updated

2 years ago
Depends on: 1378685

Updated

2 years ago
Depends on: 1357641
(Assignee)

Comment 11

2 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

Updated

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

Comment 13

2 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

2 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

2 years ago
Keywords: checkin-needed

Comment 15

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3dfb4c796b7b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
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.