1377433 - Should not do the sliding-up tour notification transition

Status: VERIFIED FIXED

(Firefox :: General, enhancement, P1)

Description

[Fischer [:Fischer]]

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.

Comment 1

[Fischer [:Fischer]]

Attachment: Bug 1377433 - Should not do the sliding-up tour notification transition,

Review commit: https://reviewboard.mozilla.org/r/153660/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153660/

Comment 2

[Fischer [:Fischer]]

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/1-2/

Comment 3

[Dave Townsend [:mossop]]

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?

Comment 4

[Fischer [:Fischer]]

(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

[Dave Townsend [:mossop]]

Comment on attachment 8882544 [details]
Bug 1377433 - Should not do the sliding-up tour notification transition,

https://reviewboard.mozilla.org/r/153660/#review158824

Comment 6

[Fischer [:Fischer]]

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/

Comment 7

[Fischer [:Fischer]]

(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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee5f3f39677f
Should not do the sliding-up tour notification transition, r=mossop

Comment 9

[Phil Ringnalda (:philor)]

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?

Comment 11

[Fischer [:Fischer]]

(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 12

[Fischer [:Fischer]]

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/3-4/

Comment 13

[Fischer [:Fischer]]

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

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8882544 [details]
Bug 1377433 - Should not do the sliding-up tour notification transition,

https://reviewboard.mozilla.org/r/153660/#review160896

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3dfb4c796b7b
Should not do the sliding-up tour notification transition, r=mossop

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3dfb4c796b7b

Comment 17

[Justin [:JW_SoftvisionQA]]

I have verified that this bug is fixed with today's nightly.