Should show the first uncomplete tour by default

VERIFIED FIXED in Firefox 56

Status

()

Firefox
New Tab Page
P1
normal
VERIFIED FIXED
a month ago
14 days ago

People

(Reporter: gasolin, Assigned: gasolin)

Tracking

(Blocks: 1 bug)

unspecified
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

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

Updated

a month ago
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: -- → P1
Whiteboard: [photon-onboarding]
(Assignee)

Comment 1

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

Comment 2

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

Updated

a month ago
QA Contact: jwilliams
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 5

a month ago
I moved the complete state check out of _loadTours, so we could fullfill the request in comment 2

also added tests

Comment 6

a month ago
mozreview-review
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)
(Assignee)

Comment 7

a month ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 9

a month ago
mozreview-review
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 10

a month ago
mozreview-review-reply
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 11

29 days ago
mozreview-review
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+
(Assignee)

Comment 12

29 days ago
mozreview-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);
```
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

28 days ago
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
(Assignee)

Comment 17

27 days ago
thanks for reminding
Keywords: checkin-needed
(Assignee)

Comment 18

27 days ago
Fischer, please help set r+ in mozreview, or the mozreview flag will not been set
Flags: needinfo?(fliu)
Keywords: checkin-needed

Comment 19

27 days ago
(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 20

27 days ago
mozreview-review
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)
(Assignee)

Comment 21

27 days ago
no problem, I'll split the test part and file a issue to land it separately
Flags: needinfo?(gasolin)
(Assignee)

Updated

27 days ago
Blocks: 1383505
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

27 days ago
mozreview-review
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+

Updated

27 days ago
Keywords: checkin-needed

Comment 25

26 days ago
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

Updated

26 days ago
Duplicate of this bug: 1365531

Updated

26 days ago
Blocks: 1354046

Comment 27

26 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/162b4c3253c4
Status: ASSIGNED → RESOLVED
Last Resolved: 26 days 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.