Closed
Bug 1392475
Opened 7 years ago
Closed 7 years ago
[Onboarding] turn fox logo to watermark
Categories
(Firefox :: New Tab Page, enhancement, P1)
Tracking
()
VERIFIED
FIXED
Firefox 57
People
(Reporter: gasolin, Assigned: rexboy)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [photon-onboarding][photon-onboarding-newui])
Attachments
(5 files)
based on https://docs.google.com/document/d/1XVowXtnAzzzyLcwdFiP6cx3l9_CNBadP9_7v17XJ40s/edit
- The overlay icon should change to the watermark version after click “skip tour” button (2d)
- change to watermark when all tours complete
- change to watermark when notifications are done
- change to watermark when user click the `skip tour` button
- hover on watermark should turn to normal icon
- hover on watermark should not show the speech bubble
- hover on watermark should not show the blue dot
- This watermarked version is still clickable and the tour can still be accessed through it
- properly handle update user case (make sure update speech bubble is shown correctly)
- add test cases
Since its a flow change, it's harder to estimate the required time now, we suggest do this after fixing other things
Updated•7 years ago
|
Flags: qe-verify+
Priority: -- → P2
QA Contact: jwilliams
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
Reporter | ||
Comment 1•7 years ago
|
||
I'm not sure if we already have any watermark firefox icon inside the firefox repo, please provide it if it's not exist yet
Flags: needinfo?(mverdi)
Comment 2•7 years ago
|
||
Stephen or Brian, can you provide the gray Firefox icon specified here https://mozilla.invisionapp.com/share/XND574NDV#/screens/249240594
Flags: needinfo?(shorlander)
Flags: needinfo?(mverdi)
Flags: needinfo?(bbell)
Assignee | ||
Comment 4•7 years ago
|
||
This change causes a listener being loaded permanently to determine whether it should load Onboarding module, even if user is not stopping at about:newtab or about:home.
There's possibility for some performance drop because the check is performed on every page loaded in the browser. but maybe it's fine for just a listener.
Assignee | ||
Comment 5•7 years ago
|
||
(The listener exists before this change, except for those who completed or hided the tour.)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → rexboy
Assignee | ||
Comment 6•7 years ago
|
||
I guess we'll need watermark for at least unofficial builds, for example, a grayscaled icon of nightly so we can use it in unofficial builds.
Does that make sense?
Flags: needinfo?(mverdi)
Assignee | ||
Comment 7•7 years ago
|
||
The asset provided in comment 3 doesn't seem to be right:
1. The color is too light to be seen in the background.
2. Background is not transparent.
Flags: needinfo?(bbell)
Sorry about the trouble with the assets. Rooky mistake. These should be okay.
Flags: needinfo?(bbell)
Comment 9•7 years ago
|
||
(In reply to KM Lee [:rexboy] from comment #6)
> I guess we'll need watermark for at least unofficial builds, for example, a
> grayscaled icon of nightly so we can use it in unofficial builds.
> Does that make sense?
The grayscale logo is the same shape across builds. There isn't a special Nightly version - it looks the same as the Firefox version.
Flags: needinfo?(mverdi)
Reporter | ||
Comment 10•7 years ago
|
||
since this bug requires some flow change (we need check some notification related status before showing the overlay icon), we may able to spin off the main flow change to a separate bug, and protected that with tests.
Reporter | ||
Updated•7 years ago
|
Whiteboard: [photon-onboarding] → [photon-onboarding][photon-onboarding-newui]
Comment 11•7 years ago
|
||
As soon as bug 1392474 complete, we should maker sure that trigger a "skip the tour" button will turn fox logo to watermark.
Tracked for 57, this bug (among others) will track the "Onboarding spec change" work planned for early Beta57
tracking-firefox57:
--- → +
Reporter | ||
Comment 13•7 years ago
|
||
Here's the current plan for the watermark change, discussed by me, Fischer, Rex, Ricky
https://docs.google.com/a/mozilla.com/presentation/d/1rt5viys6kI0urMcwK71XzKhsGlOEKne3RIUJIQeq-WA/edit?usp=sharing
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review181694
looks pretty good for the 1st round review
::: browser/extensions/onboarding/OnboardingTourType.jsm
(Diff revision 2)
> // User has never seen an onboarding tour, present the user with the new user tour.
> Services.prefs.setStringPref(PREF_TOUR_TYPE, "new");
> } else if (Services.prefs.getIntPref(PREF_SEEN_TOURSET_VERSION) < TOURSET_VERSION) {
> // show the update user tour when tour set version is larger than the seen tourset version
> Services.prefs.setStringPref(PREF_TOUR_TYPE, "update");
> - Services.prefs.setBoolPref("browser.onboarding.hidden", false);
can remove the pref in http://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js or it's better to do the migration from hidden=true -> state="watermark" so we'll also respect nightly user's pref.
::: browser/extensions/onboarding/content/onboarding.css:96
(Diff revision 2)
> }
>
> +#onboarding-overlay-button.watermark::after {
> + display: none;
> +}
> +
we still need to show the firefox icon and the speechbubble when hover on the watermark
::: browser/extensions/onboarding/content/onboarding.js:387
(Diff revision 2)
> this.uiInitialized = false;
> this._resizeTimerId =
> this._window.requestIdleCallback(() => this._resizeUI());
> }
>
> + _checkWatermarkByTour() {
can we move `_checkWatermarkByTour` method to near where it's been called, so it will be easier to trace?
::: browser/extensions/onboarding/content/onboarding.js:435
(Diff revision 2)
> body.appendChild(this._overlay);
>
> this._loadJS(TOUR_AGENT_JS_URI);
>
> this._initPrefObserver();
> + this.onIconStateChange(Services.prefs.getStringPref("browser.onboarding.state", "icon"));
we should document the default state and available states in README.md to help QA to reproduce these complicated situations.
Attachment #8904922 -
Flags: review?(gasolin)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review182166
Per discussion I'm going to add the migration code of onboarding.hidden pref
Comment 19•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review181694
> we still need to show the firefox icon and the speechbubble when hover on the watermark
According to the UX[1], it says "This watermarked version is still clickable and the tour can still be accessed through it. Hovering over the watermark overlay icon will display a full color version without a speech bubble or dot". Didn't hear change about this part but if it did change, the speechbubble should have been considered.
[1] https://docs.google.com/document/d/1XVowXtnAzzzyLcwdFiP6cx3l9_CNBadP9_7v17XJ40s/edit
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review182178
Just like the pffline discussion yesterday, the bug 1392474 is in Central now. We could rebase on it then update.
::: browser/app/profile/firefox.js:1739
(Diff revision 3)
>
> // Preferences for Photon onboarding system extension
> pref("browser.onboarding.enabled", true);
> // Mark this as an upgraded profile so we don't offer the initial new user onboarding tour.
> pref("browser.onboarding.tourset-version", 2);
> -pref("browser.onboarding.hidden", false);
> +pref("browser.onboarding.state", "icon");
"icon" is fine for me. But maybe "default" is more explicit about the "default" state (in the watermark stare, we still have colored icon on hovering). Up to you to chose the wording.
::: browser/extensions/onboarding/content/onboarding.js:26
(Diff revision 3)
> .GetStringFromName("brandShortName");
> const PROMPT_COUNT_PREF = "browser.onboarding.notification.prompt-count";
> const ONBOARDING_DIALOG_ID = "onboarding-overlay-dialog";
> const ONBOARDING_MIN_WIDTH_PX = 960;
> const SPEECH_BUBBLE_MIN_WIDTH_PX = 1150;
> +const ONBOARDING_ICON_STATES = ["icon", "watermark"];
Didn't see this being used in other place??
::: browser/extensions/onboarding/content/onboarding.js:385
(Diff revision 3)
> this.uiInitialized = false;
> this._resizeTimerId =
> this._window.requestIdleCallback(() => this._resizeUI());
> }
>
> + _checkWatermarkByTour() {
nit: `_checkWatermarkByTours`
::: browser/extensions/onboarding/content/onboarding.js:1027
(Diff revision 3)
> closeBtn.setAttribute("title",
> this._bundle.GetStringFromName("onboarding.notification-close-button-tooltip"));
> return footer;
> }
>
> hide() {
Reminder: bug 1392474 touched this method. Please remember to rebase.
::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_all.js:6
(Diff revision 3)
> - ok(!removal, `Should remove ${selector} onboarding element`);
> - }
> - });
> -}
> -
> add_task(async function test_hide_onboarding_tours() {
Reminder: bug 1392474 has renamed the filename and the test function name as well as modified the test codes. Please remeber to rebase.
::: browser/extensions/onboarding/test/browser/browser_onboarding_hide_all.js:37
(Diff revision 3)
> + await assertWatermarkIconDisplayed(tab.linkedBrowser);
> await BrowserTestUtils.removeTab(tab);
> }
> });
>
> add_task(async function test_refresh_onboarding_tours_after_hide() {
Reminder: bug 1392474 removed this test case.
::: browser/extensions/onboarding/test/browser/browser_onboarding_notification_4.js:19
(Diff revision 3)
> let targetTourId = null;
> await closeTourNotificationsOneByOne();
>
> - let expectedPrefUpdate = promisePrefUpdated("browser.onboarding.notification.finished", true);
> + let expectedPrefUpdates = [
> + promisePrefUpdated("browser.onboarding.notification.finished", true),
> + promisePrefUpdated("browser.onboarding.state", "watermark")
Thanks for taking care the notification tests. There is another test case in browser_onboarding_notification.js [1] in need (so many test cases for this specs changes...)
[1] http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/browser/extensions/onboarding/test/browser/browser_onboarding_notification.js#20
::: browser/extensions/onboarding/test/browser/browser_onboarding_tourset.js:85
(Diff revision 3)
> });
>
> await BrowserTestUtils.removeTab(tab);
> });
> +
> +add_task(async function test_set_watermark_after_all_tour_completed() {
I was thinking the place where this test case belongs to. This test file is holding test cases for loading the rigth tourset under different conditions. There is other test cases like "test_click_action_button_to_set_tour_completed"/"test_set_right_tour_completed_style_on_overlay" in browser_onboarding_tours.js, which is closer to our test case here. Maybe we could move this test case there.
[1] http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/browser/extensions/onboarding/test/browser/browser_onboarding_tours.js#63
::: browser/extensions/onboarding/test/browser/browser_onboarding_tourset.js:95
(Diff revision 3)
> + ]});
> +
> + let tab = await openTab(ABOUT_NEWTAB_URL);
> + await promiseOnboardingOverlayLoaded(tab.linkedBrowser);
> + TOUR_IDs.forEach(id => Preferences.set(`browser.onboarding.tour.${id}.completed`, true));
> + let expectedPrefUpdate = promisePrefUpdated("browser.onboarding.state", "watermark");
Better to move this `let expectedPrefUpdate = ...` before `TOUR_IDs.forEach(...` in case of the intermittent possibility.
::: browser/extensions/onboarding/test/browser/head.js:4
(Diff revision 3)
> /* Any copyright is dedicated to the Public Domain.
> * http://creativecommons.org/publicdomain/zero/1.0/ */
>
> +/* globals gBrowser */
Do we need this? If I wasn't wrong, without this line would fail at local `./mach lint` but still pass on TRY server. (Still we could keep this, just a question)
Attachment #8904922 -
Flags: review?(fliu)
Assignee | ||
Updated•7 years ago
|
Attachment #8904922 -
Flags: review?(gasolin)
Reporter | ||
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review182610
::: browser/extensions/onboarding/bootstrap.js:29
(Diff revision 3)
> ["browser.onboarding.notification.finished", PREF_BOOL],
> ["browser.onboarding.notification.prompt-count", PREF_INT],
> ["browser.onboarding.notification.last-time-of-changing-tour-sec", PREF_INT],
> ["browser.onboarding.notification.tour-ids-queue", PREF_STRING],
> ];
>
we may better check the allowed state for `browser.onboarding.state`, currently only the `watermark`
Comment hidden (mozreview-request) |
Comment 23•7 years ago
|
||
We need to make sure the logo doesn't disappear at small screen sizes. It should be a permanent fixture and the current breakpoints should keep it from overlapping other elements.
Comment 24•7 years ago
|
||
(In reply to Aaron Benson from comment #23)
> We need to make sure the logo doesn't disappear at small screen sizes. It
> should be a permanent fixture and the current breakpoints should keep it
> from overlapping other elements.
But it the user clicks on the watermark, they would see the broken tour overlay layout...
Reporter | ||
Comment 25•7 years ago
|
||
Aaron, if we don't hide the logo in small screen sizes, the user is able to click the logo and open the onboarding overlay with wrong layout.
I think the behavior from Comment 23 is not documented yet, please help us figure out the right behavior if we want to show the logo in any resolution.
Flags: needinfo?(abenson)
Reporter | ||
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review183092
::: browser/extensions/onboarding/OnboardingTourType.jsm:29
(Diff revision 4)
> check() {
> const PREF_TOUR_TYPE = "browser.onboarding.tour-type";
> const PREF_SEEN_TOURSET_VERSION = "browser.onboarding.seen-tourset-version";
> const TOURSET_VERSION = Services.prefs.getIntPref("browser.onboarding.tourset-version");
>
> + // Migrate browser.onboarding.hidden to browser.onboarding.state for earlier versions.
we can put the migration script in `browser/components/nsBrowserGlue.js` to just run once and keep code base clean.
Set `UI_VERSION = 53` and put the script under `currentUIVersion < 53`.
::: browser/extensions/onboarding/README.md:59
(Diff revision 4)
> For example, if we update the tourset in v60 and decide to show all update users the tour, we set `browser.onboarding.tourset-version` as `3`.
> +
> +## Icon states
> +
> +Onboarding module has two states for its overlay icon: `default` and `watermark`.
> +By default it shows `default` state.
`By default, ...`
::: browser/extensions/onboarding/README.md:60
(Diff revision 4)
> +
> +## Icon states
> +
> +Onboarding module has two states for its overlay icon: `default` and `watermark`.
> +By default it shows `default` state.
> +when either tours or notifications are all completed, the icon changes to watermark state.
nit: `W`hen
::: browser/extensions/onboarding/README.md:60
(Diff revision 4)
> +
> +## Icon states
> +
> +Onboarding module has two states for its overlay icon: `default` and `watermark`.
> +By default it shows `default` state.
> +when either tours or notifications are all completed, the icon changes to watermark state.
"changes to watermark state" -> "changes to the `watermark` state"
::: browser/extensions/onboarding/README.md:62
(Diff revision 4)
> +
> +Onboarding module has two states for its overlay icon: `default` and `watermark`.
> +By default it shows `default` state.
> +when either tours or notifications are all completed, the icon changes to watermark state.
> +The icon state is stored in `browser.onboarding.state`.
> +Upon version update of the tourset, icon state changes back to `default`.
When `tourset-version` is updated, or when we detect the `tour-type` is changed to `update`, icon state will be changed back to the `default` state.
::: browser/extensions/onboarding/content/onboarding.css:105
(Diff revision 4)
> +
> +#onboarding-overlay-button.watermark:not(:hover) > #onboarding-overlay-button-watermark-icon {
> + display: block;
> +}
> +
> +#onboarding-overlay-button.watermark:not(:hover) > #onboarding-overlay-button-icon {
can we merge these 3 decalrations for one `display: none` styles?
::: browser/extensions/onboarding/content/onboarding.js:676
(Diff revision 4)
> }
> this._tourItems = this._tourPages =
> this._overlayIcon = this._overlay = this._notificationBar = null;
> }
>
> + onIconStateChange(state) {
Do we want to have a _private method convention like `_onIconStateChange`? (it's optional though)
::: browser/extensions/onboarding/content/onboarding.js:1091
(Diff revision 4)
> let tooltip = this._bundle.formatStringFromName(tooltipStringId, [BRAND_SHORT_NAME], 1);
> button.setAttribute("aria-label", tooltip);
> button.id = "onboarding-overlay-button";
> button.setAttribute("aria-haspopup", true);
> button.setAttribute("aria-controls", `${ONBOARDING_DIALOG_ID}`);
> - let img = this._window.document.createElement("img");
> + button.innerHTML = `<img src="chrome://branding/content/icon64.png" id="onboarding-overlay-button-icon" role="presentation"/>
instead of innerHTML, please still use createElement to create both images and append to the button for consistency.
::: browser/extensions/onboarding/test/browser/browser_onboarding_notification_4.js:19
(Diff revision 4)
> let targetTourId = null;
> await closeTourNotificationsOneByOne();
>
> - let expectedPrefUpdate = promisePrefUpdated("browser.onboarding.notification.finished", true);
> + let expectedPrefUpdates = [
> + promisePrefUpdated("browser.onboarding.notification.finished", true),
> + promisePrefUpdated("browser.onboarding.state", "watermark")
we can define "watermark" and "default" as constant in `head.js`, then we can use them across the tests
Attachment #8904922 -
Flags: review?(gasolin)
Comment hidden (mozreview-request) |
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review183168
In fact this is almost ok to set a r+ for me. However, it looks like the patch may have another update so wold like to wait for the next update. Hope this is not too nit for you.
::: browser/extensions/onboarding/OnboardingTourType.jsm:31
(Diff revision 4)
> const PREF_SEEN_TOURSET_VERSION = "browser.onboarding.seen-tourset-version";
> const TOURSET_VERSION = Services.prefs.getIntPref("browser.onboarding.tourset-version");
>
> + // Migrate browser.onboarding.hidden to browser.onboarding.state for earlier versions.
> + if (Services.prefs.prefHasUserValue("browser.onboarding.hidden")) {
> + let state = Services.prefs.getBoolPref("browser.onboarding.hidden") ? "watermark" : "default";
Do we really need this migration block? For the cases as below:
Case 1: New user starts from 57 so no way "browser.onboarding.hidden" would exist
Case 2: Update user comes from 55 directly to 57 so no way "browser.onboarding.hidden" would exist
Case 3: Update user comes from 55 to 56 to 57. The user wouldn't see any tour in 56 so no way "browser.onboarding.hidden" would be set to True
Case 4: Update user comes from 56 (in 56 the user is an new user) but didn't hide tours so no way "browser.onboarding.hidden" would be set to True
Case 5: Update user comes from 56 (in 56 the user is an new user) and hid tours so "browser.onboarding.hidden" would be set to True. However, in this case we reset/clear all prefs in the below `else if (Services.prefs.getIntPref(PREF_SEEN_TOURSET_VERSION) < TOURSET_VERSION)` condition.
::: browser/extensions/onboarding/OnboardingTourType.jsm:39
(Diff revision 4)
> + }
> +
> if (!Services.prefs.prefHasUserValue(PREF_SEEN_TOURSET_VERSION)) {
> // User has never seen an onboarding tour, present the user with the new user tour.
> Services.prefs.setStringPref(PREF_TOUR_TYPE, "new");
> } else if (Services.prefs.getIntPref(PREF_SEEN_TOURSET_VERSION) < TOURSET_VERSION) {
If we wanted to *migrate* the "hidden" pref, we probably should just do
`Services.prefs.clearUserPref("browser.onboarding.hidden");` inside this condition block and leave a comment saying like "we want to clear this legacy useless pref"
::: browser/extensions/onboarding/test/browser/browser_onboarding_skip_tour.js:11
(Diff revision 4)
> add_task(async function test_skip_onboarding_tours() {
> resetOnboardingDefaultState();
>
> let tourIds = TOUR_IDs;
> let expectedPrefUpdates = [
> - promisePrefUpdated("browser.onboarding.notification.finished", true)
> + promisePrefUpdated("browser.onboarding.notification.finished", true),
Shouldn't we expect "browser.onboarding.state" got updated here too?
::: browser/extensions/onboarding/test/browser/head.js:283
(Diff revision 4)
> }
> +
> +function assertWatermarkIconDisplayed(browser) {
> + return ContentTask.spawn(browser, {}, function() {
> + let overlayButton = content.document.getElementById("onboarding-overlay-button");
> + ok(overlayButton.classList.contains("watermark"));
Please add some assertion comment like "Should display the watermark onboarding icon" so peopel could know what's wrong in the failure case.
Attachment #8904922 -
Flags: review?(fliu)
Comment 29•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review183092
> instead of innerHTML, please still use createElement to create both images and append to the button for consistency.
I'm fine with the `innerHTML` approach. Much easier for reading and cleaner. But also ok for `createElement` if we really wanted that. Up to you two's discussion.
Comment 30•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review183168
> If we wanted to *migrate* the "hidden" pref, we probably should just do
> `Services.prefs.clearUserPref("browser.onboarding.hidden");` inside this condition block and leave a comment saying like "we want to clear this legacy useless pref"
Or maybe just call `Services.prefs.clearUserPref("browser.onboarding.hidden")` while entering `OnboardingTourType.check`
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review183190
::: browser/components/nsBrowserGlue.js:2094
(Diff revision 5)
> Services.prefs.setBoolPref("devtools.netmonitor.persistlog", true);
> }
> }
>
> + if (currentUIVersion < 53) {
> + // Migrate browser.onboarding.hidden to browser.onboarding.state for earlier versions.
This is problematic. The UI_VERSION 53 was adde by bug 1395419 [1]. This part is unnecessary and brings conde confilict.
[1] http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/browser/components/nsBrowserGlue.js#2070
Comment 32•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review183190
> This is problematic. The UI_VERSION 53 was adde by bug 1395419 [1]. This part is unnecessary and brings conde confilict.
>
> [1] http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/browser/components/nsBrowserGlue.js#2070
Per the offline discussin, we could do this "hidden" pref migration for nightly users. p.s please still remember to rebase on the latest Central.
Comment 33•7 years ago
|
||
(In reply to Fred Lin [:gasolin] from comment #25)
> Aaron, if we don't hide the logo in small screen sizes, the user is able to
> click the logo and open the onboarding overlay with wrong layout.
>
> I think the behavior from Comment 23 is not documented yet, please help us
> figure out the right behavior if we want to show the logo in any resolution.
I see. I didn't realize the tour didn't have a compact mode. Seems like a relatively easy problem to solve .. we could drop the big illustration on the right-hand side (for example).
Michael, do we have designs at all for a smaller-breakpoint onboarding modal?
Flags: needinfo?(abenson) → needinfo?(mverdi)
Reporter | ||
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review183172
The patch looks in good shape now, please rebase and we can help doing the on-device test
::: browser/components/nsBrowserGlue.js:2094
(Diff revision 5)
> Services.prefs.setBoolPref("devtools.netmonitor.persistlog", true);
> }
> }
>
> + if (currentUIVersion < 53) {
> + // Migrate browser.onboarding.hidden to browser.onboarding.state for earlier versions.
I think `for earlier versions` is redundant here
Attachment #8904922 -
Flags: review?(gasolin)
Comment hidden (mozreview-request) |
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review183640
Attachment #8904922 -
Flags: review?(fliu) → review+
Reporter | ||
Comment 37•7 years ago
|
||
mozreview-review |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review183680
::: browser/extensions/onboarding/test/browser/head.js:34
(Diff revision 6)
>
> function resetOnboardingDefaultState() {
> // All the prefs should be reset to the default states
> // and no need to revert back so we don't use `SpecialPowers.pushPrefEnv` here.
> Preferences.set("browser.onboarding.enabled", true);
> + Preferences.set("browser.onboarding.state", "default");
Preferences.set("browser.onboarding.state", ICON_STATE_DEFAULT);
Attachment #8904922 -
Flags: review?(gasolin) → review+
Comment hidden (mozreview-request) |
Comment 39•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.
hg error in cmd: hg rebase -s 2b063b380f25 -d b674d9f6d1d6: rebasing 419399:2b063b380f25 "Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished. r=Fischer,gasolin" (tip)
merging browser/app/profile/firefox.js
merging browser/extensions/onboarding/content/onboarding.css
merging browser/extensions/onboarding/test/browser/browser_onboarding_tours.js
warning: conflicts while merging browser/extensions/onboarding/test/browser/browser_onboarding_tours.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Reporter | ||
Comment 40•7 years ago
|
||
mozreview-review |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review184172
::: browser/extensions/onboarding/content/onboarding.css:98
(Diff revision 7)
> #onboarding-overlay-button:dir(rtl)::after {
> box-shadow: 2px 0 5px 0 rgba(74, 74, 79, 0.25);
> }
>
> +#onboarding-overlay-button-watermark-icon,
> +#onboarding-overlay-button.watermark:not(:hover)::after,
we forget to follow our rule: add `onboarding-` as class prefix... So please use `onboarding-watermark` instead of `watermark`
Comment hidden (mozreview-request) |
Assignee | ||
Comment 42•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8904922 [details]
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
https://reviewboard.mozilla.org/r/176718/#review184172
> we forget to follow our rule: add `onboarding-` as class prefix... So please use `onboarding-watermark` instead of `watermark`
Updated & rebased. Waiting for try.
Comment 43•7 years ago
|
||
Pushed by kmlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d0949c18bf5
[Onboarding] Turn fox logo to watermark if all tours or notifications are finished. r=Fischer,gasolin
Comment 45•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•7 years ago
|
Priority: P2 → P1
Comment 46•7 years ago
|
||
Fred, the watermark icon in nightly looks ... not great. Was it possibly compressed .. or? We should be using the icon Bryan supplied in this bug.
Flags: needinfo?(gasolin)
Comment 47•7 years ago
|
||
(In reply to Aaron Benson from comment #46)
> Fred, the watermark icon in nightly looks ... not great. Was it possibly
> compressed .. or? We should be using the icon Bryan supplied in this bug.
The icon provided in comment 8 is png file.
@Bryan,
About the icon provided in comment 8, do we have the svg version?
Thanks
Flags: needinfo?(gasolin) → needinfo?(bbell)
Comment 51•7 years ago
|
||
I have verified that this issue has been fixed on today's nightly.
Status: RESOLVED → VERIFIED
Comment 52•7 years ago
|
||
I can confirm the intended behavior is respected on beta. I verified using Fx 57.0b7 on Windows 10 x64, Ubuntu 14.04 LTS and macOS X 10.12.6.
Flags: qe-verify+
Updated•2 years ago
|
Flags: needinfo?(mverdi)
You need to log in
before you can comment on or make changes to this bug.
Description
•