Should add the screenshot tour in the onBoarding overlay

UX call it "screenshots" in bug 1381351 and bug 1382376, so let's align with them and use "screenshots" as ID

I'm not sure this is a good idea since we cannot screenshot the about:newtab page and clicking the take a shot button will pop new tab.

as today's meeting decision, please compose the tour without CTA button. At the mean time I suggest add screenshots id in gotoPage to turn it as an autocomplete tour, and we can remove that and add the CTA button back when we figure out the solution.

Hi Verdi, Per the meeting conclusion, the Screenshots tour action button should open the screenshots page. And the screenshots page should be responsible for highlighting the Screenshots button, see bug 1393668. The old Screenshots tour action button title is "Show Screenshots in menu", which no longer fits our case here. Alternatively we use "Show Screenshots" as the tour action button title, see attachment 8901160 [details]: Screenshots_tour_page.png for now. Please let us know what is the final new title for the Screenshots tour action button title? Will update accordingly. Thanks

Comment on attachment 8901162 [details] Bug 1371538 - Should add the Screenshots tour in the onBoarding overlay, https://reviewboard.mozilla.org/r/172634/#review177924 ::: browser/extensions/onboarding/content/onboarding.css:474 (Diff revision 1) > +#onboarding-tour-screenshots:hover { > + background-image: url("img/icons_screenshots-colored.svg"); > +} > + > +a#onboarding-tour-screenshots-button:hover, > +a#onboarding-tour-screenshots-button:visited { In fact, `#onboarding-tour-screenshots-button:hover` and `#onboarding-tour-screenshots-button:visited` without `a` is enough. However, in order to highlight this is a style rule for <a> element so added `a`. Otherwise maybe it would be confusing why this Screenshots tour buttion needs some extra css rule. ::: browser/extensions/onboarding/content/onboarding.js:319 (Diff revision 1) > + </section> > + <section class="onboarding-tour-content"> > + <img src="resource://onboarding/img/figure_screenshots.svg" role="presentation"/> > + </section> > + <aside class="onboarding-tour-button-container"> > + <a id="onboarding-tour-screenshots-button" class="onboarding-tour-action-button" data-l10n-id="onboarding.tour-screenshots.button" Why go for <a>: 1) for a11y, since it is really an link. 2) we can open the screenshots page in an new tab only with one line of `target="_blank"`. Otherwise, we need to send async msg to the chrome side to ask the chrome side to the job. ::: browser/extensions/onboarding/content/onboarding.js:478 (Diff revision 1) > + > /** > * Find a tour that should be selected. It is either a first tour that was not > * yet complete or the first one in the tab list. > */ > - get selectedTour() { > + get _firstUncompleteTour() { Changed the getter name because the name of `selectedTour` has flaw. Let's consider the followings: 1) The overlay opens and `goTo` the 1st uncompleted Customize tour. Then user selects another Sync tour. At this moment, the slected tour should be Sync but this getter would still return Cusomize tour. 2) The original purpose of this code was to get the 1st uncompleted tour, which is added by bug 1381765. 3) This is more like an internal usage getter so prefixed with "_" ::: browser/extensions/onboarding/content/onboarding.js:668 (Diff revision 1) > // Lazy loading until first toggle. > this._loadTours(this._tours); > } > > this.hideNotification(); > - this._overlay.classList.toggle("onboarding-opened"); > + this.toggleModal(this._overlay.classList.toggle("onboarding-opened")); A btw improvement: `classList.toggle` is enough to tell the existence of "onboarding-opened"[1]. [1] https://developer.mozilla.org/en-US/docs/Web/API/DOMTokenList/toggle ::: browser/extensions/onboarding/content/onboarding.js:690 (Diff revision 1) > child => child.id !== "onboarding-overlay" && > child.setAttribute("aria-hidden", true)); > - // When dialog is opened with the keyboard, focus on the selected or > - // first tour item. > + // When dialog is opened with the keyboard, focus on the 1st uncomplete tour > + // because it will be the selected tour > if (this._overlayIcon.dataset.keyboardFocus) { > - doc.getElementById(this.selectedTour.id).focus(); > + doc.getElementById(this._firstUncompleteTour.id).focus(); TBH, better to use `selectedTourId` here but that means we need to call `toggleModal` after `gotoPage`. That is a bit bothersome and the line of `this.toggleModal(this._overlay.classList.toggle("onboarding-opened"));` is consice. So chose to utilize comment. Please let me know your thought. ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:126 (Diff revision 1) > + > +onboarding.tour-screenshots=Screenshots > +onboarding.tour-screenshots.title=Take better screenshots. > +# LOCALIZATION NOTE(onboarding.tour-screenshots.description): %S is brandShortName. > +onboarding.tour-screenshots.description=Take, save and share screenshots — without leaving %S. Capture a region or an entire page as you browse. Then save to the web for easy access and sharing. > +onboarding.tour-screenshots.button=Show Screenshots ni? UX for this button's final string. Will update when checked-in or later upon having the final string. Please don't let this trivial thing stop us proceeding, thank you.

(In reply to Fischer [:Fischer] from comment #9) > Created attachment 8901162 [details] > Bug 1371538 - Should add the Screenshots tour in the onBoarding overlay, > > Review commit: https://reviewboard.mozilla.org/r/172634/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/172634/ Hi Rex, Please see the Screenshots tour in action, attachment 8901158 [details]: Screenshots_Tour.gif TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48895f66536c6f592973dbde78f74983baad0337 Thanks

(In reply to Fred Lin [:gasolin] from comment #5) > as today's meeting decision, please compose the tour without CTA button. > > At the mean time I suggest add screenshots id in gotoPage to turn it as an > autocomplete tour, and we can remove that and add the CTA button back when > we figure out the solution. Update the status: Now we want to have the Screenshots tour action button open the screenshots page. And the screenshots page should be responsible for highlighting the Screenshots button, see bug 1393668.

(In reply to Fischer [:Fischer] from comment #8) > Please let us know what is the final new title for the Screenshots tour > action button title? Will update accordingly. > The button label should be: Open Screenshots Website

Comment on attachment 8901162 [details] Bug 1371538 - Should add the Screenshots tour in the onBoarding overlay, https://reviewboard.mozilla.org/r/172634/#review178464 r=me after the tab issue of `<a>` confirming with a11y team. ::: browser/extensions/onboarding/content/onboarding.js:319 (Diff revision 1) > + </section> > + <section class="onboarding-tour-content"> > + <img src="resource://onboarding/img/figure_screenshots.svg" role="presentation"/> > + </section> > + <aside class="onboarding-tour-button-container"> > + <a id="onboarding-tour-screenshots-button" class="onboarding-tour-action-button" data-l10n-id="onboarding.tour-screenshots.button" Can you add some comment like "Screenshot tour opens the screenshot page directly. The screenshots page should be responsible for highlighting the Screenshots button" either in this function or in the css? ::: browser/extensions/onboarding/content/onboarding.js:536 (Diff revision 1) > * @return {DOMNode} newly focused element if any > */ > wrapMoveFocus(current, back) { > let elms = [...this._dialog.querySelectorAll( > `button, input[type="checkbox"], input[type="email"], [tabindex="0"]`)]; > + // The Screenshots tour <a> button is not an navigation target (even with tabindex) This is fine for me but seems this is a Mac-only specification of Firefox which can be overcome by pressing control+F7. Maybe Yura knows more information about it. If that's allowed by A11y team we may not need these logic for handling this `<a>` exception. ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:126 (Diff revision 1) > + > +onboarding.tour-screenshots=Screenshots > +onboarding.tour-screenshots.title=Take better screenshots. > +# LOCALIZATION NOTE(onboarding.tour-screenshots.description): %S is brandShortName. > +onboarding.tour-screenshots.description=Take, save and share screenshots — without leaving %S. Capture a region or an entire page as you browse. Then save to the web for easy access and sharing. > +onboarding.tour-screenshots.button=Show Screenshots Okay, just remember to get l10n team review the final string before checking it in.

Yura: For some reason we have to use <a> rather than <button> under Screenshot tour's action button (Although pretending it's a button by some CSS). But we found that <a> doesn't work unless user triggers control+f7 for the "Full keyboard access". So in the patcＨ some exception for handling that case is added. My question is: how do you usually handle the issue under Mac that <a> is not tab-able, do you usually put any special logics for them? Or we can simply ignore it? Thank you!

(In reply to KM Lee [:rexboy] from comment #15) > Yura: For some reason we have to use <a> rather than <button> under > Screenshot tour's action button (Although pretending it's a button by some > CSS). But we found that <a> doesn't work unless user triggers control+f7 for > the "Full keyboard access". So in the patcＨ some exception for handling that > case is added. My question is: how do you usually handle the issue under Mac > that <a> is not tab-able, do you usually put any special logics for them? Or > we can simply ignore it? > > Thank you! Ah yes, this is unfortunate but we at this time still support the platform (OSX) setting. Even safari does not support it any more. I think it's expected by our keyboard users that if they have the setting not set, then not all elements will be focusable. There is no way to fix this in content at the moment except for catching keyboard events and managing focus ourselves, so it's not worth it. I would keep it as is (an anchor) and assume that it is accessible if it is accessible with the OSX preference set to All Controls.

(In reply to Yura Zenevich [:yzen] from comment #16) > (In reply to KM Lee [:rexboy] from comment #15) > > Yura: For some reason we have to use <a> rather than <button> under > > Screenshot tour's action button (Although pretending it's a button by some > > CSS). But we found that <a> doesn't work unless user triggers control+f7 for > > the "Full keyboard access". So in the patcＨ some exception for handling that > > case is added. My question is: how do you usually handle the issue under Mac > > that <a> is not tab-able, do you usually put any special logics for them? Or > > we can simply ignore it? > > > > Thank you! > > Ah yes, this is unfortunate but we at this time still support the platform > (OSX) setting. Even safari does not support it any more. I think it's > expected by our keyboard users that if they have the setting not set, then > not all elements will be focusable. There is no way to fix this in content > at the moment except for catching keyboard events and managing focus > ourselves, so it's not worth it. I would keep it as is (an anchor) and > assume that it is accessible if it is accessible with the OSX preference set > to All Controls. OK, I will remove the handling of this <a> element.

Comment on attachment 8901162 [details] Bug 1371538 - Should add the Screenshots tour in the onBoarding overlay, https://reviewboard.mozilla.org/r/172634/#review178464 > Can you add some comment like "Screenshot tour opens the screenshot page directly. The screenshots page should be responsible for highlighting the Screenshots button" either in this function or in the css? Thanks updated > This is fine for me but seems this is a Mac-only specification of Firefox which can be overcome by pressing control+F7. Maybe Yura knows more information about it. If that's allowed by A11y team we may not need these logic for handling this `<a>` exception. Removed, thanks

(In reply to Fischer [:Fischer] from comment #19) > Comment on attachment 8901162 [details] > Bug 1371538 - Should add the Screenshots tour in the onBoarding overlay, > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/172634/diff/1-2/ Hi Flod, That would be great you could have a look at new strings added into the onboarding.properties, thanks

Comment on attachment 8901162 [details] Bug 1371538 - Should add the Screenshots tour in the onBoarding overlay, https://reviewboard.mozilla.org/r/172634/#review178926 One question, and a localization note to add for sure. ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:122 (Diff revision 2) > onboarding.tour-performance.description=It’s a whole new %1$S, built for faster page loading, smoother scrolling, and more responsive tab switching. These performance upgrades come paired with a modern, intuitive design. Start browsing and experience it for yourself: the best %1$S yet. > onboarding.notification.onboarding-tour-performance.title=Browse with the best of ‘em. > # LOCALIZATION NOTE(onboarding.notification.onboarding-tour-performance.message): %S is brandShortName. > onboarding.notification.onboarding-tour-performance.message=Prepare yourself for the fastest, smoothest, most reliable %S yet. > + > +onboarding.tour-screenshots=Screenshots Is this "Screenshots" as a general plural noun (i.e. "Screen captures"), or "Screenshots" as the name of the Firefox Screenshots feature? "Firefox Screenshots" is a feature name, and it's not localized within Firefox, so we need a note to explain which one it is. ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:125 (Diff revision 2) > onboarding.notification.onboarding-tour-performance.message=Prepare yourself for the fastest, smoothest, most reliable %S yet. > + > +onboarding.tour-screenshots=Screenshots > +onboarding.tour-screenshots.title=Take better screenshots. > +# LOCALIZATION NOTE(onboarding.tour-screenshots.description): %S is brandShortName. > +onboarding.tour-screenshots.description=Take, save and share screenshots — without leaving %S. Capture a region or an entire page as you browse. Then save to the web for easy access and sharing. I feel the need to point out that Screenshots doesn't currently support full page screenshots anymore, hopefully it will be fixed by the time we're in Beta or Release. ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:126 (Diff revision 2) > + > +onboarding.tour-screenshots=Screenshots > +onboarding.tour-screenshots.title=Take better screenshots. > +# LOCALIZATION NOTE(onboarding.tour-screenshots.description): %S is brandShortName. > +onboarding.tour-screenshots.description=Take, save and share screenshots — without leaving %S. Capture a region or an entire page as you browse. Then save to the web for easy access and sharing. > +onboarding.tour-screenshots.button=Open Screenshots Website This one definitely needs a note, like # LOCALIZATION NOTE(onboarding.tour-screenshots.button): Screenshots is the name # of the Firefox feature and should not be localized.

Comment on attachment 8901162 [details] Bug 1371538 - Should add the Screenshots tour in the onBoarding overlay, https://reviewboard.mozilla.org/r/172634/#review178926 > Is this "Screenshots" as a general plural noun (i.e. "Screen captures"), or "Screenshots" as the name of the Firefox Screenshots feature? > > "Firefox Screenshots" is a feature name, and it's not localized within Firefox, so we need a note to explain which one it is. Yes, "Screenshots" is the name of the Firefox Screenshots feature. One note is added. > This one definitely needs a note, like > > # LOCALIZATION NOTE(onboarding.tour-screenshots.button): Screenshots is the name > # of the Firefox feature and should not be localized. Thanks. One note is added.

(In reply to Francesco Lodolo [:flod] from comment #21) > Comment on attachment 8901162 [details] > Bug 1371538 - Should add the Screenshots tour in the onBoarding overlay, > > https://reviewboard.mozilla.org/r/172634/#review178926 > ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:125 (Diff revision 2) > > +onboarding.tour-screenshots.description=Take, save and share screenshots — without leaving %S. Capture a region or an entire page as you browse. Then save to the web for easy access and sharing. > > I feel the need to point out that Screenshots doesn't currently support full > page screenshots anymore, hopefully it will be fixed by the time we're in Beta or Release. > Hi Verdi, The localization team, Francesco, has noticed some thing in the Screenshots tour description. Would you please have a look and let us know your thought? Thank you.

(In reply to Francesco Lodolo [:flod] from comment #21) > Comment on attachment 8901162 [details] > Bug 1371538 - Should add the Screenshots tour in the onBoarding overlay, > > https://reviewboard.mozilla.org/r/172634/#review178926 > ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:125 > (Diff revision 2) > > onboarding.notification.onboarding-tour-performance.message=Prepare yourself for the fastest, smoothest, most reliable %S yet. > > + > > +onboarding.tour-screenshots=Screenshots > > +onboarding.tour-screenshots.title=Take better screenshots. > > +# LOCALIZATION NOTE(onboarding.tour-screenshots.description): %S is brandShortName. > > +onboarding.tour-screenshots.description=Take, save and share screenshots — without leaving %S. Capture a region or an entire page as you browse. Then save to the web for easy access and sharing. > > I feel the need to point out that Screenshots doesn't currently support full > page screenshots anymore, hopefully it will be fixed by the time we're in > Beta or Release. > Thanks, Francesco, for noticing this. Needinfo UX verdi to see his thought.

Comment on attachment 8901162 [details] Bug 1371538 - Should add the Screenshots tour in the onBoarding overlay, https://reviewboard.mozilla.org/r/172634/#review178974 Tiny fixes on the comments, but it's good for me otherwise (pending question on the feature description) ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:122 (Diff revisions 2 - 3) > onboarding.tour-performance.description=It’s a whole new %1$S, built for faster page loading, smoother scrolling, and more responsive tab switching. These performance upgrades come paired with a modern, intuitive design. Start browsing and experience it for yourself: the best %1$S yet. > onboarding.notification.onboarding-tour-performance.title=Browse with the best of ‘em. > # LOCALIZATION NOTE(onboarding.notification.onboarding-tour-performance.message): %S is brandShortName. > onboarding.notification.onboarding-tour-performance.message=Prepare yourself for the fastest, smoothest, most reliable %S yet. > > +# LOCALIZATION NOTE (onboarding.tour-screenshots): The "Screenshots" is the name of the Firefox Screenshots feature and should not be localized. Drop "The" at the beginning ::: browser/extensions/onboarding/locales/en-US/onboarding.properties:127 (Diff revisions 2 - 3) > +# LOCALIZATION NOTE (onboarding.tour-screenshots): The "Screenshots" is the name of the Firefox Screenshots feature and should not be localized. > onboarding.tour-screenshots=Screenshots > onboarding.tour-screenshots.title=Take better screenshots. > # LOCALIZATION NOTE(onboarding.tour-screenshots.description): %S is brandShortName. > onboarding.tour-screenshots.description=Take, save and share screenshots — without leaving %S. Capture a region or an entire page as you browse. Then save to the web for easy access and sharing. > +# LOCALIZATION NOTE (onboarding.tour-screenshots.button): The "Screenshots" is the name of the Firefox Screenshots feature and should not be localized. Same here, drop "The"

(In reply to Francesco Lodolo [:flod] from comment #21) > I feel the need to point out that Screenshots doesn't currently support full > page screenshots anymore, hopefully it will be fixed by the time we're in > Beta or Release. Let's ask John Guen.

57 *should* include full page shooting. It's landed back in nightly as of today. That having been said, there are is an outstanding perf bug that could (very low risk, but not no risk) cause us to pull it. https://github.com/mozilla-services/screenshots/issues/3182

(In reply to [:jgruen] from comment #28) > 57 *should* include full page shooting. It's landed back in nightly as of > today. > > That having been said, there are is an outstanding perf bug that could (very > low risk, but not no risk) cause us to pull it. > > https://github.com/mozilla-services/screenshots/issues/3182 Thanks, ok, we will keep the current Screenshots description strings. BTW, will set the target screenshots website url to https://screenshots.firefox.com/#tour per the bug 1393668 comment 11.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/15b957e02bbf Should add the Screenshots tour in the onBoarding overlay, r=flod,rexboy

I have verified that this is fixed on today's nightly.

I can confirm this is fixed on beta as well (Fx 57.0b7).