Closed Bug 1392475 Opened 7 years ago Closed 7 years ago

[Onboarding] turn fox logo to watermark

Categories

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

Product:
Firefox
Component:
New Tab Page
Version:
57 Branch
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 + verified

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)

watermark firefox
26.75 KB, image/png
Details
newtab-firefox.zip
8.31 KB, application/zip
Details
Screenshot with Asset in comment 3
45.61 KB, image/png
Details
newtab-firefox.zip
9.66 KB, application/zip
Details
Bug 1392475 - [Onboarding] Turn fox logo to watermark if all tours or notifications are finished.
59 bytes, text/x-review-board-request
Fischer
: review+
gasolin
: review+
Details 
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
Flags: qe-verify+
Priority: -- → P2
QA Contact: jwilliams
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
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)
Attached image watermark firefox — 
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)
Attached file newtab-firefox.zip — 
Parts Needed
Flags: needinfo?(shorlander)
Flags: needinfo?(bbell)
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.
(The listener exists before this change, except for those who completed or hided the tour.)
Assignee: nobody → rexboy
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)
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)
Attached file newtab-firefox.zip — 
Sorry about the trouble with the assets. Rooky mistake. These should be okay.
Flags: needinfo?(bbell)
(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)
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.
Whiteboard: [photon-onboarding] → [photon-onboarding][photon-onboarding-newui]
Depends on: 1395483
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: --- → +
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 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 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 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 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)
Attachment #8904922 - Flags: review?(gasolin)
Depends on: 1395480
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`
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.
(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...
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)
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 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 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 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 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 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.
(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)
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 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+
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+
Blocks: 1399051
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)
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 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.
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
https://hg.mozilla.org/mozilla-central/rev/7d0949c18bf5
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Priority: P2 → P1
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)
(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)
Yes, I put that in a new bug 1399693
Flags: needinfo?(bbell)
Depends on: 1399693
I have verified that this issue has been fixed on today's nightly.
Status: RESOLVED → VERIFIED
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.
status-firefox57: fixed → verified
Flags: qe-verify+
Flags: needinfo?(mverdi)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: