Closed Bug 1381765 Opened 7 years ago Closed 7 years ago

Should show the first uncomplete tour by default

Categories

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

Product:
Firefox
Component:
New Tab Page
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: gasolin, Assigned: gasolin)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(1 file)

Bug 1381765 - Should show the first uncomplete tour by default;
59 bytes, text/x-review-board-request
mossop
: review+
Fischer
: review+
Details 
currently when we load tours, we show the first tour by default
 
this.gotoPage(tours[0].id);

But after we can set tour as complete, the correct behavior would be show the first uncomplete tour by default
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: -- → P1
Whiteboard: [photon-onboarding]
Hi Verdi, I found a corner case that need UX definition: What tour should the user expect to see when the user reopen overlay in the same page?

1. user open the overlay and see the first uncomplete tour
2. user tap the call-to-action button in a tour (private browsing), and the tour is set as complete, 
3. the user close the overlay then reopen the overlay

Now, what tour should the user expect to see?

1. the same tour(private browsing) before the user close the overlay (what we current have), or
2. the next umcomplete tour(Add-ons)
Flags: needinfo?(mverdi)
This is a great idea. Yes, let's show the next incomplete tour. If all the tours are complete, then just start at the beginning.
Flags: needinfo?(mverdi)
QA Contact: jwilliams
I moved the complete state check out of _loadTours, so we could fullfill the request in comment 2

also added tests
Comment on attachment 8888170 [details]
Bug 1381765 - Should show the first uncomplete tour by default;

https://reviewboard.mozilla.org/r/159090/#review164496

::: browser/extensions/onboarding/content/onboarding.js:376
(Diff revision 2)
>  
>        return;
>      }
>  
>      switch (evt.target.id) {
>        case "onboarding-overlay-icon":

Probably need some adjustment here to go to the 1st uncompleted page.

::: browser/extensions/onboarding/content/onboarding.js:429
(Diff revision 2)
>        this._loadTours(this._tours);
>      }
>  
> +    let selectedTourId;
> +    for (let tour of this._tours) {
> +      if (!this.markTourCompletionState(tour.id) && !selectedTourId) {

Please use `isTourCompleted` because our purpose is to check the complete state.

::: browser/extensions/onboarding/content/onboarding.js:433
(Diff revision 2)
> +    for (let tour of this._tours) {
> +      if (!this.markTourCompletionState(tour.id) && !selectedTourId) {
> +        selectedTourId = tour.id;
> +      }
> +    }
> +    this.gotoPage(selectedTourId ? selectedTourId : this._tours[0].id);

Could we do this whole checking tour complete state then `goToPage` job in the "onboarding-overlay-icon" click event only?
2 reasons:
1. Consider the "onboarding-overlay-close-btn" click event comes, then `toggleOverlay` is called. Then the overlay is going to close. At this case, there is no need to go to page.
2. Consider the "onboarding-notification-action-btn" click event comes, then `toggleOverlay` called then a `goToPage` happened inside here. Then outside another `goToPage` is called. We shouldn't make this twice `goToPage` happen.
Thanks

::: browser/extensions/onboarding/content/onboarding.js:484
(Diff revision 2)
>      if (this._tourItems && this._tourItems.length > 0 && this.isTourCompleted(tourId)) {
>        let targetItem = this._tourItems.find(item => item.id == tourId);
>        targetItem.classList.add("onboarding-complete");
> +      return true;
>      }
> +    return false;

Returning true or false based on if we did the css class is fine for me. Just please don't use `markTourCompletionState` to check the tour complete state. There is a case that a tour is complete but false returned because `_tourItems` is empty. And it is ambiguous to tell `markTourCompletionState` would do the job of judging complete state. We have a `isTourCompleted` is exactly doing that job.
Attachment #8888170 - Flags: review?(fliu)
Comment on attachment 8888170 [details]
Bug 1381765 - Should show the first uncomplete tour by default;

https://reviewboard.mozilla.org/r/159090/#review164496

> Please use `isTourCompleted` because our purpose is to check the complete state.

updated

> Could we do this whole checking tour complete state then `goToPage` job in the "onboarding-overlay-icon" click event only?
> 2 reasons:
> 1. Consider the "onboarding-overlay-close-btn" click event comes, then `toggleOverlay` is called. Then the overlay is going to close. At this case, there is no need to go to page.
> 2. Consider the "onboarding-notification-action-btn" click event comes, then `toggleOverlay` called then a `goToPage` happened inside here. Then outside another `goToPage` is called. We shouldn't make this twice `goToPage` happen.
> Thanks

updated

> Returning true or false based on if we did the css class is fine for me. Just please don't use `markTourCompletionState` to check the tour complete state. There is a case that a tour is complete but false returned because `_tourItems` is empty. And it is ambiguous to tell `markTourCompletionState` would do the job of judging complete state. We have a `isTourCompleted` is exactly doing that job.

updated with isTourCompleted
Comment on attachment 8888170 [details]
Bug 1381765 - Should show the first uncomplete tour by default;

https://reviewboard.mozilla.org/r/159090/#review164584

::: browser/extensions/onboarding/content/onboarding.js:386
(Diff revision 3)
>        case "onboarding-overlay":
> +        let selectedTourId;
>          this.toggleOverlay();
> +        for (let tour of this._tours) {
> +          if (!this.isTourCompleted(tour.id) && !selectedTourId) {
> +            selectedTourId = tour.id;

This could have some redundant loop even after find the 1st uncompleted tour.
Maybe we could move `let selectedTourId` down after `toggleOverlay` and do `let selectedTourId = this._tours.find(tour => !this.isTourCompleted(tour.id)) || this._tours[0].id;

::: browser/extensions/onboarding/content/onboarding.js:389
(Diff revision 3)
> +        for (let tour of this._tours) {
> +          if (!this.isTourCompleted(tour.id) && !selectedTourId) {
> +            selectedTourId = tour.id;
> +          }
> +        }
> +        this.gotoPage(selectedTourId ? selectedTourId : this._tours[0].id);

Could simplify this to `this.goToPage(selectedTourId)` with the update above.

::: browser/extensions/onboarding/test/browser/browser_onboarding_select_default_tour.js:23
(Diff revision 3)
> +
> +  for (let url of URLs) {
> +    let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
> +    await BrowserTestUtils.loadURI(tab.linkedBrowser, url);
> +    await promiseOnboardingOverlayLoaded(tab.linkedBrowser);
> +    await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-overlay-icon", {}, tab.linkedBrowser);

Just remind that the bug 1377439 is going to land, please rebase on it and to use `#onboarding-overlay-button`
Attachment #8888170 - Flags: review?(fliu)
Comment on attachment 8888170 [details]
Bug 1381765 - Should show the first uncomplete tour by default;

https://reviewboard.mozilla.org/r/159090/#review164584

> Just remind that the bug 1377439 is going to land, please rebase on it and to use `#onboarding-overlay-button`

Don't have to do `#onboarding-overlay-button` right now just remeber to update this before landing this bug.
Comment on attachment 8888170 [details]
Bug 1381765 - Should show the first uncomplete tour by default;

https://reviewboard.mozilla.org/r/159090/#review164802

r+ with Fischer's changes
Attachment #8888170 - Flags: review?(dtownsend) → review+
Comment on attachment 8888170 [details]
Bug 1381765 - Should show the first uncomplete tour by default;

https://reviewboard.mozilla.org/r/159090/#review164978

::: browser/extensions/onboarding/content/onboarding.js:386
(Diff revision 3)
>        case "onboarding-overlay":
> +        let selectedTourId;
>          this.toggleOverlay();
> +        for (let tour of this._tours) {
> +          if (!this.isTourCompleted(tour.id) && !selectedTourId) {
> +            selectedTourId = tour.id;

Nice suggestion! Though the _tours.find will return tours and gotoPage only take tour.id, so it need slight modification:

```
let selectedTour = this._tours.find(tour => !this.isTourCompleted(tour.id)) || this._tours[0];
this.gotoPage(selectedTour.id);
```
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
thanks for reminding
Keywords: checkin-needed
Fischer, please help set r+ in mozreview, or the mozreview flag will not been set
Flags: needinfo?(fliu)
Keywords: checkin-needed
(In reply to Fred Lin [:gasolin] from comment #18)
> Fischer, please help set r+ in mozreview, or the mozreview flag will not been set
Sorry for postponing this , I am looking into the Bug 1382079, Bug 1383301, and Bug 1381679.

Currently our onboarding tests are experiencing some intermittent issues on checking the overlay loading and the window leaks on the tourset tests. Maybe what causing this is not yet clear. However, the tests on the treeherder and other bug patches(bug 1159003, bug 1379762) are a bit messy. 

Just a bit worried the things could go more complicated if this bug introduced the browser_onboarding_select_default_tour.js test. Since the update on the onboarding.js to go to the 1st uncompleted tour is pretty small. Probably we could land that part first then land the test in a follow-up bug.

fred, would you mind split up the patch? Thanks.

p.s I could definitely do splitting up job if you'd like.
Flags: needinfo?(fliu) → needinfo?(gasolin)
Comment on attachment 8888170 [details]
Bug 1381765 - Should show the first uncomplete tour by default;

https://reviewboard.mozilla.org/r/159090/#review165508

::: browser/extensions/onboarding/test/browser/browser_onboarding_select_default_tour.js:52
(Diff revision 6)
> +
> +  for (let url of URLs) {
> +    let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser);
> +    await BrowserTestUtils.loadURI(tab.linkedBrowser, url);
> +    await promiseOnboardingOverlayLoaded(tab.linkedBrowser);
> +    await BrowserTestUtils.synthesizeMouseAtCenter("#onboarding-overlay-icon", {}, tab.linkedBrowser);

This test cannot be landed too as the bug 1381765 comment 10. This id reabsing issue is not resolved.
Attachment #8888170 - Flags: review?(fliu)
no problem, I'll split the test part and file a issue to land it separately
Flags: needinfo?(gasolin)
Blocks: 1383505
Comment on attachment 8888170 [details]
Bug 1381765 - Should show the first uncomplete tour by default;

https://reviewboard.mozilla.org/r/159090/#review165510
Attachment #8888170 - Flags: review?(fliu) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/162b4c3253c4
Should show the first uncomplete tour by default;r=Fischer,mossop
Keywords: checkin-needed
Blocks: 1354046
https://hg.mozilla.org/mozilla-central/rev/162b4c3253c4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
I have confirmed this fix.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: