Closed
Bug 1392791
Opened 7 years ago
Closed 7 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)
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
Reporter | ||
Comment 1•7 years 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•7 years ago
|
||
can reproduce it, investigate now
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Updated•7 years ago
|
Flags: qe-verify+
Priority: P3 → P1
QA Contact: jwilliams
Comment hidden (mozreview-request) |
Assignee | ||
Comment 4•7 years 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•7 years 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•7 years ago
|
||
separated function to showOverlay and hideOverlay, will add test later
Comment hidden (mozreview-request) |
Comment 9•7 years 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•7 years ago
|
||
Pushed by flin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cfd465206f79 not call gotoPage while hide overlay;r=rexboy
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cfd465206f79
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Reporter | ||
Comment 14•7 years ago
|
||
I have verified that this is fixed on today's nightly.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•