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

VERIFIED FIXED in Firefox 57

Status

()

Firefox
New Tab Page
P1
normal
VERIFIED FIXED
10 months ago
8 months ago

People

(Reporter: JW_SoftvisionQA, Assigned: gasolin@mozilla.com)

Tracking

57 Branch
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-onboarding])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

10 months ago
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
(Reporter)

Comment 1

10 months ago
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
(Assignee)

Comment 2

10 months ago
can reproduce it, investigate now
Assignee: nobody → gasolin
Status: NEW → ASSIGNED

Updated

10 months ago
Flags: qe-verify+
Priority: P3 → P1
QA Contact: jwilliams
Comment hidden (mozreview-request)
(Assignee)

Comment 4

10 months ago
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 5

10 months ago
mozreview-review
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)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

10 months ago
separated function to showOverlay and hideOverlay, will add test later
Comment hidden (mozreview-request)

Comment 9

10 months ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

10 months ago
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cfd465206f79
not call gotoPage while hide overlay;r=rexboy

Comment 13

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cfd465206f79
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Reporter)

Comment 14

10 months ago
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.
status-firefox57: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.