Closed Bug 1392791 Opened 2 years ago Closed 2 years ago

The last Notification does not show. Last element in tour overlay is marked as complete without any interaction

Categories

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

57 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 --- verified

People

(Reporter: jwilliams, Assigned: gasolin)

Details

(Whiteboard: [photon-onboarding])

Attachments

(1 file)

STR:
1. Start the tour as a new user
2. Open overlay and complete all options except Default Browser
3. Close the overlay and open a new tab

Expected:
Default Browser Notification should show. Clicking the fox head should show Default Browser as unchecked.

Actual:
Default Browser Notification does not show. Default Browser is marked as completed in the overlay.


See video below:
https://jwilliams-softvision.tinytake.com/sf/MTg5NTA1Ml82MDYyOTE1
After further testing I have discovered that this happens also with sync. Changing the title to the last element.
Summary: Default Browser Notification does not show. Default Browser Marked as complete without any interaction → The last Notification does not show. Last element in tour overlay is marked as complete without any interaction
can reproduce it, investigate now
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: P3 → P1
QA Contact: jwilliams
The root cause is when we toggle the overlay, we will call `gotoPage(this.selectedTour.id)` whenever the overlay is show or hide. `selectedTour` will pick the next tour id and `gotoPage` will try to focus on it, which marked the next auto-completed tour as complete.
Comment on attachment 8900557 [details]
Bug 1392791 - not call gotoPage while hide overlay;

https://reviewboard.mozilla.org/r/171956/#review177356

::: browser/extensions/onboarding/content/onboarding.js:466
(Diff revision 1)
>        // If the clicking target is directly on the outer-most overlay,
>        // that means clicking outside the tour content area.
>        // Let's toggle the overlay.
>        case "onboarding-overlay":
>          this.toggleOverlay();
> +        if (this._overlay.classList.contains("onboarding-opened")) {

We should one day separate `toggleOverlay()` to `showOverlay()` and `hideOverlay()` because the code there is difficult to tell which line is need for which.....

If we're not going to do this refactoring for now, at least, only `onboarding-overlay-button` is for opening the overlay and the remaining are for closing. so please remove the `if()` and do required thing in separate cases rather than put them all together.
Attachment #8900557 - Flags: review?(rexboy)
separated function to showOverlay and hideOverlay, will add test later
Comment on attachment 8900557 [details]
Bug 1392791 - not call gotoPage while hide overlay;

https://reviewboard.mozilla.org/r/171956/#review178526

::: browser/extensions/onboarding/content/onboarding.js:459
(Diff revision 3)
>      if (classList.contains("onboarding-tour-item-container")) {
>        ({ id, classList } = target.firstChild);
>      }
>  
>      switch (id) {
>        case "onboarding-overlay-button":

`this.showOverlay()` and `this.gotoPage()` should be just in this case.

::: browser/extensions/onboarding/content/onboarding.js:464
(Diff revision 3)
>        case "onboarding-overlay-button":
>        case "onboarding-overlay-close-btn":
>        // If the clicking target is directly on the outer-most overlay,
>        // that means clicking outside the tour content area.
>        // Let's toggle the overlay.
>        case "onboarding-overlay":

... and `this.hideOverlay()` for just these two cases. So we don't need the `if()` anymore.

I guess there'd be a conflict for `toggleModal()` but that should be easy to fix.
Attachment #8900557 - Flags: review?(rexboy) → review+
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cfd465206f79
not call gotoPage while hide overlay;r=rexboy
https://hg.mozilla.org/mozilla-central/rev/cfd465206f79
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
I have verified that this is fixed on today's nightly.
Status: RESOLVED → VERIFIED
I can confirm this issue is fixed on Fx 57.0b8 as well.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.