1392474 - [Onboarding] replace the check box with a “Skip Tour” button

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[[e-mail]]

based on https://docs.google.com/document/d/1XVowXtnAzzzyLcwdFiP6cx3l9_CNBadP9_7v17XJ40s/edit

replace the “Mark all as complete, and hide the tour” check box with a “Skip Tour” button 

* The overlay should close immediately
* address a11y change
* should still mark all tours completed
* update test cases

Comment 1

[[e-mail]]

For this issue we should not set `browser.onboarding.hidden` anymore.

Related to bug 1392472, we should reset onboarding-overlay-button:hover::after style to hide the speech bubble and the bluedot, that part could be done after bug 1392475

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Verdi,

Can you confirm the text "Skip The Tour" from [1] is capitalized instead of "Skip the tour"?

[1] https://docs.google.com/document/d/1XVowXtnAzzzyLcwdFiP6cx3l9_CNBadP9_7v17XJ40s/edit

Comment 3

[Ricky Chien [:rickychien] (inactive)]

BTW, can you provide the updated visual spec for this button? The only picture I see is at [1] but it didn't tell me the details.

Can you add the button details in invision spec [2], I'd like to know such as the width of button (fit l10n), hover, active state and margin/padding around it. Thanks


[1] https://docs.google.com/document/d/1XVowXtnAzzzyLcwdFiP6cx3l9_CNBadP9_7v17XJ40s/edit
[2] https://mozilla.invisionapp.com/share/XYD3ERUKE#/248971014_New_Tab_-_Onboarding

Comment 4

[Ricky Chien [:rickychien] (inactive)]

This bug aims at introducing (https://docs.google.com/document/d/1XVowXtnAzzzyLcwdFiP6cx3l9_CNBadP9_7v17XJ40s/edit):

* All tour items can be checked off
* All tour notifications are stopped (and snippets can begin on the next new tab)
* The overlay should close immediately (you should no longer also have to click the overlay close button).
* The overlay icon should change to the watermark version.

A follow-up bug will be filed soon for addressing watermark icon

* The overlay icon should change to the watermark version

Comment 5

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 57, this bug (among others) will track the "Onboarding spec change" work planned for early Beta57

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1392474 - [Onboarding] replace the check box with a “Skip Tour” button

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

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8904140 [details]
Bug 1392474 - [Onboarding] replace the check box with a “Skip Tour” button

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175924/diff/1-2/

Comment 8

[[e-mail]]

Verdi, do we use 'Skip this Tour' as the final string?

Comment 9

[Fischer [:Fischer]]

Comment on attachment 8904140 [details]
Bug 1392474 - [Onboarding] replace the check box with a “Skip Tour” button

https://reviewboard.mozilla.org/r/175924/#review180934

Thanks for helping. Besides the below updates we need to update the browser_onboarding_hide_all.js test (probably need to rename the test as browser_onboarding_skip_all.js)

::: browser/extensions/onboarding/content/onboarding.css:148
(Diff revision 2)
> -  grid-row: footer-start;
>    grid-column: dialog-start / tour-end;
>    font-size: 13px;
>  }
>  
> -#onboarding-tour-hidden-checkbox {
> +#onboarding-tour-hidden-button {

Could we change the id `#onboarding-skip-tour-button`?
This is for:
1. make it clear this is about "skip the tour" different from the old "hidden" checkbox
2. we no longer have "hidden" state so removing the related wording could reduce the confusion.

::: browser/extensions/onboarding/content/onboarding.css:153
(Diff revision 2)
> -#onboarding-tour-hidden-checkbox {
> +#onboarding-tour-hidden-button {
>    margin-inline-start: 27px;
>    margin-inline-end: 10px;
> +  margin-bottom: 15px;
> +  border-radius: 2px;
> +  border: none;

Please use `border: 1px solid transparent` for the high-contrast mode see [1].

But in fact since these all are buttons under #onboarding-overlay, we probably could go like
```
#onboarding-overlay button {
  border: 1px solid transparent;
}
```

[1] http://searchfox.org/mozilla-central/rev/999385a5e8c2d360cc37286882508075fc2078bd/browser/extensions/onboarding/content/onboarding.css#316

::: browser/extensions/onboarding/content/onboarding.css:339
(Diff revision 2)
>    margin-inline-end: 26px;
>    margin-top: -32px;
>  }
>  
>  /* Remove default dotted outline around buttons' text */
>  .onboarding-tour-action-button::-moz-focus-inner,

We probably need `#onboarding-overlay button` here so could cover the new Skip-tour button

::: browser/extensions/onboarding/content/onboarding.css:345
(Diff revision 2)
>  #onboarding-overlay-button::-moz-focus-inner {
>    border: 0;
>  }
>  
>  /* Keyboard focus specific outline */
>  .onboarding-tour-action-button:-moz-focusring,

We probably need `#onboarding-overlay button` here so could cover the new Skip-tour button

::: browser/extensions/onboarding/content/onboarding.js:496
(Diff revision 2)
>          this.gotoPage(this._firstUncompleteTour.id);
>          break;
> +      case "onboarding-tour-hidden-button":
> +        this.hideNotification();
> +        this.skipTour();
> +        this.hideOverlay();

This is really a bit edge and nit but it would be slightly better we do `hideOverlay` before `skipTour`. This is because inside `skipTour` we would send msg to chrom process if sth woring during sending, the code following wouldn't run.

::: browser/extensions/onboarding/content/onboarding.js
(Diff revision 2)
>  
> -  hide() {
> +  skipTour() {
>      this.setToursCompleted(this._tours.map(tour => tour.id));
>      sendMessageToChrome("set-prefs", [
>        {
> -        name: "browser.onboarding.hidden",

Need to remove "browser.onboarding.hidden" in the bootstrap.js, OnboardingTourType.jsm and the below addEventListner below too. Please search "browser.onboarding.hidden" on the searchfox in case more out there missed.

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:6
(Diff revision 2)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  # LOCALIZATION NOTE(onboarding.overlay-title2): This string will be used in the overlay title.
>  onboarding.overlay-title2=Let’s get started
> -onboarding.hidden-checkbox-label-text=Mark all as complete, and hide the tour
> +onboarding.hidden-button-label=Skip This Tour

The same as id case, could we use `onboarding.skip-tour-button-label`?

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8904140 [details]
Bug 1392474 - [Onboarding] replace the check box with a “Skip Tour” button

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175924/diff/2-3/

Comment 12

[Francesco Lodolo [:flod]]

(In reply to Fred Lin [:gasolin] from comment #8)
> Verdi, do we use 'Skip this Tour' as the final string?

Isn't the doc saying to use "Skip Tour" (as the commit message)? Also, the current text has "This" capitalized.

Comment 13

[Ricky Chien [:rickychien] (inactive)]

(In reply to Francesco Lodolo [:flod] from comment #12)
> (In reply to Fred Lin [:gasolin] from comment #8)
> > Verdi, do we use 'Skip this Tour' as the final string?
> 
> Isn't the doc saying to use "Skip Tour" (as the commit message)? Also, the
> current text has "This" capitalized.

From doc's picture, it's saying "Skip This Tour" as the string. Anyway, I think it's just a concept but not a real spec, let's wait for Verdi's spec.

Comment 14

[Fischer [:Fischer]]

Comment on attachment 8904140 [details]
Bug 1392474 - [Onboarding] replace the check box with a “Skip Tour” button

https://reviewboard.mozilla.org/r/175924/#review181190

Thanks for updating the patch. Improved a lot.

::: browser/extensions/onboarding/content/onboarding.css:157
(Diff revision 3)
> +  border-radius: 2px;
> +  border: 1px solid transparent;
> +  padding: 10px 20px;
> +  font-size: 14px;
> +  color: #202340;
> +}

Please add background color such as `backgorund: #dedede;` because the current button backgound color(`rgb(240, 240, 240)`) is a bit close to the ovrelay background color(`rgb(245, 245, 247)`) so hard to tell these 2 elements. Maybe the UX hasn't given the button styles at least we could set a color making a better view on our own first, thanks.

::: browser/extensions/onboarding/content/onboarding.css:160
(Diff revision 3)
> +  font-size: 14px;
> +  color: #202340;
> +}
> +
> +#onboarding-skip-tour-button:hover {
> +  background-color: #ebebeb;

Please go for a #d3d3d3 background. This is compared to the #dedede set above. You could come up with your own color

::: browser/extensions/onboarding/content/onboarding.css
(Diff revision 3)
>    margin-top: -32px;
>  }
>  
>  /* Remove default dotted outline around buttons' text */
> -.onboarding-tour-action-button::-moz-focus-inner,
> +#onboarding-overlay button::-moz-focus-inner {
> -#onboarding-overlay-button::-moz-focus-inner {

Don't remove #onboarding-overlay-button. This is the fox head icon button.

::: browser/extensions/onboarding/content/onboarding.css
(Diff revision 3)
>    border: 0;
>  }
>  
>  /* Keyboard focus specific outline */
> -.onboarding-tour-action-button:-moz-focusring,
> +#onboarding-overlay button:-moz-focusring,
> -#onboarding-notification-action-btn:-moz-focusring,

Don't remove #onboarding-notification-action-btn. This is a button on the notification bar not on the overlay

::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_all.js:6
(Diff revision 3)
> -      ok(!removal, `Should remove ${selector} onboarding element`);
> -    }
> -  });
> -}
> -
>  add_task(async function test_hide_onboarding_tours() {

Since we are not testing "hidden" any more. Please change the filename to browser_onboarding_skip_tour.js and change this test function name to test_skip_onboarding_tours

::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_all.js:16
(Diff revision 3)
>      promisePrefUpdated("browser.onboarding.notification.finished", true)
>    ];
>    tourIds.forEach((id, idx) => expectedPrefUpdates.push(promisePrefUpdated(`browser.onboarding.tour.${id}.completed`, true)));
>  
>    let tabs = [];
>    for (let url of URLs) {

We don't have to loop URLs to open 2 tabs. The original purpose for this is to test we set the "hidden" state in one tab then another tab would also enter the "hidden" state. Since no more "hidden" state, we don't have to test this cross-tabs behavior. We coudl just open one "about:newtab".

::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_all.js:24
(Diff revision 3)
>      await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-overlay-button", {}, tab.linkedBrowser);
>      await promiseOnboardingOverlayOpened(tab.linkedBrowser);
>      tabs.push(tab);
>    }
>  
> -  let doc = content.document;
> +  await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-skip-tour-button", {}, gBrowser.selectedBrowser);

We are expecting the overlay closed after clicking #onboarding-skip-tour-button so please add one line before here: `let overlayClosedPromise = promiseOnboardingOverlayClosed(gBrowser.selectedBrowser);` and await this promise below.

::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_all.js:24
(Diff revision 3)
>      await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-overlay-button", {}, tab.linkedBrowser);
>      await promiseOnboardingOverlayOpened(tab.linkedBrowser);
>      tabs.push(tab);
>    }
>  
> -  let doc = content.document;
> +  await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-skip-tour-button", {}, gBrowser.selectedBrowser);

We are expecting the overlay closed after clicking #onboarding-skip-tour-button so please add one line before here: `let overlayClosedPromise = promiseOnboardingOverlayClosed(gBrowser.selectedBrowser);` and await this promise below

::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_all.js:25
(Diff revision 3)
>      tabs.push(tab);
>    }
>  
> -  let doc = content.document;
> +  await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-skip-tour-button", {}, gBrowser.selectedBrowser);
> -  await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-tour-hidden-checkbox", {}, gBrowser.selectedBrowser);
>    await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-overlay-close-btn", {}, gBrowser.selectedBrowser);

Please replace `await synthesizeMouseAtCenter("#onboarding-overlay-close-btn", ...)` with  `await overlayClosedPromise;`

::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_all.js:28
(Diff revision 3)
> -  await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-tour-hidden-checkbox", {}, gBrowser.selectedBrowser);
>    await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-overlay-close-btn", {}, gBrowser.selectedBrowser);
>    await Promise.all(expectedPrefUpdates);
> -  ok(!doc.getElementById("onboarding-overlay-button"));
>  
>    for (let i = tabs.length - 1; i >= 0; --i) {

No longer open 2 tabs above so we don't need to loop here either.

::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_all.js:34
(Diff revision 3)
> -    await assertOnboardingDestroyed(tab.linkedBrowser);
>      await BrowserTestUtils.removeTab(tab);
>    }
>  });
>  
>  add_task(async function test_refresh_onboarding_tours_after_hide() {

We can remove this test case. The original purpose for this test case is to test after reloading, the Onboarding should remain in the "hidden" state. But there is no longer "hidden" state. The things right now we care about are:
  1. "browser.onboarding.notification.finished" pref is true
  2. all tours' completed prefs are true
  3. closed the overlay after cliking the skip-tour button.
These are all covered by the above test case.

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8904140 [details]
Bug 1392474 - [Onboarding] replace the check box with a “Skip Tour” button

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175924/diff/3-4/

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8904140 [details]
Bug 1392474 - [Onboarding] replace the check box with a “Skip Tour” button

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175924/diff/4-5/

Comment 17

[Verdi [:verdi] Best to slack me]

Attachment: skiptour.png

This button should use the same style as the "Learn More" buttons on the notifications.

The copy should be, "Skip Tour"
System font, 14px, #202340

Button:
background: FBFBFB
border: C1C1C1
border-radius: 2px

On hover:
background: EBEBEB

Active:
background: DADADA

Comment 18

[Fischer [:Fischer]]

Comment on attachment 8904140 [details]
Bug 1392474 - [Onboarding] replace the check box with a “Skip Tour” button

https://reviewboard.mozilla.org/r/175924/#review181644

r+ with issues and UX's comments addressed. Another thing need your help, that is, please run xpcshell test as well before checked-in. The onboarding got xpcshell test as well in case that we missed the xpcshell test in the prvious modifications.

::: browser/extensions/onboarding/test/browser/browser_onboarding_skip_tour.js:20
(Diff revision 5)
> -    await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-overlay-button", {}, tab.linkedBrowser);
> +  await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-overlay-button", {}, tab.linkedBrowser);
> -    await promiseOnboardingOverlayOpened(tab.linkedBrowser);
> +  await promiseOnboardingOverlayOpened(tab.linkedBrowser);
> -    tabs.push(tab);
> -  }
>  
> -  let doc = content.document;
> +  let overlayClosedPromise = promiseOnboardingOverlayClosed(gBrowser.selectedBrowser);

nit: please go for `tab.linkedBrowser` just for the consistency

::: browser/extensions/onboarding/test/browser/browser_onboarding_skip_tour.js:21
(Diff revision 5)
> -    await promiseOnboardingOverlayOpened(tab.linkedBrowser);
> +  await promiseOnboardingOverlayOpened(tab.linkedBrowser);
> -    tabs.push(tab);
> -  }
>  
> -  let doc = content.document;
> -  await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-tour-hidden-checkbox", {}, gBrowser.selectedBrowser);
> +  let overlayClosedPromise = promiseOnboardingOverlayClosed(gBrowser.selectedBrowser);
> +  await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-skip-tour-button", {}, gBrowser.selectedBrowser);

nit: please go for `tab.linkedBrowser` just for the consistency

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8904140 [details]
Bug 1392474 - [Onboarding] replace the check box with a “Skip Tour” button

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175924/diff/5-6/

Comment 20

[Ricky Chien [:rickychien] (inactive)]

Thanks for the review. I've checked xpcshell test and it looks great. Let's wait for try result.

Comment 21

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8904140 [details]
Bug 1392474 - [Onboarding] replace the check box with a “Skip Tour” button

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/175924/diff/6-7/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe4b20934031
[Onboarding] replace the check box with a “Skip Tour” button r=Fischer

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fe4b20934031

Comment 24

[Justin [:JW_SoftvisionQA]]

I have confirmed that this is fixed on Today's nightly.

Comment 25

[Emil Ghitta, QA [:emilghitta]]

This issue is verified using Firefox 57.0b6 (BuildId:20171005195903) on Windows 10 64bit, macOS 10.11.6 and Ubuntu 16.04 64bit.