Closed Bug 1400326 Opened 7 years ago Closed 7 years ago

Preparing existing about:home for 57 if we decide not to ship activity stream on it

Categories

(Firefox :: New Tab Page, defect)

Product:
Firefox
Component:
New Tab Page
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- verified
firefox58 --- verified

People

(Reporter: k88hudson, Assigned: ursula)

References

Details

Attachments

(3 files)

example onboarding (30% opacity) covering about:home controls
64.61 KB, image/png
Details
Bug 1400326 - Preparing existing about:home for 57 if we decide not to ship activity stream on it
59 bytes, text/x-review-board-request
k88hudson
: review+
Mardak
: review+
mconley
: feedback+
ritu
: approval-mozilla-beta+
Details
video --setpref browser.newtabpage.activity-stream.aboutHome.enabled=false
53.62 KB, video/webm
Details 
Note that this is an option only if we decide that activity stream + onboarding + snippets should not be shipped on about:home for 57.


If we do decide to stick with the existing about:home, we should:

- restyle the search input to use the correct icons/styles matching activity stream
- restyle the background (and possibly the helper bar at the bottom) to match photon colours/styles
- add telemetry via ping-centre so we can track stuff we're interested in
verdi, what needs to happen for existing about:home if activity stream is not about:home in 57?

I believe the "manual migration" was depending on activity stream about:home.

Also, onboarding notification at the bottom of the page covers up all the about:home controls. See attachment.
Flags: needinfo?(mverdi)
In addition to what Kate said, I think we’d have to:
* Remove that bottom button bar, including the restore session button
* Remove snippets from their place under the search field and use the new implementation of snippets
* Add the manual data import section
Flags: needinfo?(mverdi)
Depends on: 1401289
It was just decided that we will ship Activity Stream on about:home in Firefox 57 even though there is a slight hero element regression (33ms) compared to Firefox 56 about:home and a potential larger perceived regression given that more content is shown on Activity Stream where it can feel longer to load compared to before.

The risk of making code changes to old about:home for 57 outweigh the potential regression, which based on early feedback of Beta 57 of people saying it overall feels much faster might not actually be a perceived regression.
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: affected → wontfix
Resolution: --- → WONTFIX
After some discussion about collecting telemetry, we'd like to implement this behind a pref in order to have a more accurate comparison for a shield study.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee: nobody → usarracini
Comment on attachment 8916124 [details]
Bug 1400326 - Preparing existing about:home for 57 if we decide not to ship activity stream on it

https://reviewboard.mozilla.org/r/187348/#review192408

::: browser/base/content/abouthome/aboutHome.css:38
(Diff revision 3)
>  
>  .spacer {
>    -moz-box-flex: 1;
>  }
>  
> +#brandLogo {

Remove this duplication

::: browser/base/content/abouthome/aboutHome.css:433
(Diff revision 3)
> +
> +/* Overrides for snippets
> +   These changes make it so that snippets do not affect the height of the
> +   main element, so that elements don't move around on page load. */
> +#topSection {
> +  position: relative;

You could combine these with the general rules up top

::: browser/base/content/test/about/browser_aboutHome_input.js:62
(Diff revision 3)
>  
>      is(result.pane, "paneSync", "openPreferences should be called with paneSync");
>      is(result.params.urlParams.entrypoint, "abouthome",
>        "openPreferences should be called with abouthome entrypoint");
>    });
> -});
> +}).skip();

Can you add a comment about why this is getting skipped?
Attachment #8916124 - Flags: review?(khudson) → review-
Comment on attachment 8916124 [details]
Bug 1400326 - Preparing existing about:home for 57 if we decide not to ship activity stream on it

https://reviewboard.mozilla.org/r/187348/#review192416

::: browser/base/content/abouthome/aboutHome.css:40
(Diff revision 5)
>    -moz-box-flex: 1;
>  }
>  
>  #topSection {
>    text-align: center;
> +  margin-top: -90px;

Who should be signing off on the final look? I'm guessing this is to better center the content on the page in a way that it doesn't shift much when snippets decides to appear?

::: browser/base/content/abouthome/aboutHome.css:169
(Diff revision 5)
>       the snippets load. */
>    min-height: calc(15/12 * 3em);
>  }
>  
>  #launcher {
> -  display: -moz-box;
> +  display: none;

Any reason not to just delete the html? Tests? I suppose this should be fine for now if it'll just all be deleted anyway.

::: browser/base/content/abouthome/aboutHome.css:384
(Diff revision 5)
>    }
>  }
> +
> +/* Overrides for onboarding
> +   These overrides hide the Firefox logo (since about:home has a
> +   large logo already) and make the helper icon larger */

Was this recommended by onboarding?

::: browser/base/content/abouthome/aboutHome.js
(Diff revision 5)
>  
>  function setupSearch() {
> -  // Set submit button label for when CSS background are disabled (e.g.
> -  // high contrast mode).
> -  document.getElementById("searchSubmit").value =
> -    document.body.getAttribute("dir") == "ltr" ? "\u25B6" : "\u25C0";

Not sure if we should be removing this. As the comment says, it's for high contrast mode when background images disappear.

::: browser/base/jar.mn
(Diff revision 5)
>          content/browser/abouthome/addons.png          (content/abouthome/addons.png)
>          content/browser/abouthome/sync.png            (content/abouthome/sync.png)
>          content/browser/abouthome/settings.png        (content/abouthome/settings.png)
>          content/browser/abouthome/restore.png         (content/abouthome/restore.png)
>          content/browser/abouthome/restore-large.png   (content/abouthome/restore-large.png)
> -        content/browser/abouthome/mozilla.svg         (content/abouthome/mozilla.svg)

Something something about removing files requiring a clobber. ? E.g., bug 1399686 ?

::: browser/locales/en-US/chrome/browser/aboutHome.dtd
(Diff revision 5)
>  <!ENTITY abouthome.syncButton.label      "&syncBrand.shortName.label;">
> -
> -<!-- LOCALIZATION NOTE (abouthome.aboutMozilla.label): The (invisible) label for
> -     the mozilla wordmark in the top-right corner that links to Mozilla's main
> -     about page. -->
> -<!ENTITY abouthome.aboutMozilla.label    "About Mozilla">

Unless we need to remove this now, probably best not to touch especially if there's potential for uplift.
Comment on attachment 8916124 [details]
Bug 1400326 - Preparing existing about:home for 57 if we decide not to ship activity stream on it

https://reviewboard.mozilla.org/r/187348/#review192420
Comment on attachment 8916124 [details]
Bug 1400326 - Preparing existing about:home for 57 if we decide not to ship activity stream on it

https://reviewboard.mozilla.org/r/187348/#review192416

> Who should be signing off on the final look? I'm guessing this is to better center the content on the page in a way that it doesn't shift much when snippets decides to appear?

We've been discussing on slack with Aaron, we'll run this particular issue by him, but yeah, the reasoning for this is so that the page doesn't shift. There is a comment near the snippets css
Comment on attachment 8916124 [details]
Bug 1400326 - Preparing existing about:home for 57 if we decide not to ship activity stream on it

https://reviewboard.mozilla.org/r/187348/#review192416

> Was this recommended by onboarding?

This was recommended by Aaron to deal with the triple logo situation. It's only applied to the old version of about:home
Comment on attachment 8916124 [details]
Bug 1400326 - Preparing existing about:home for 57 if we decide not to ship activity stream on it

https://reviewboard.mozilla.org/r/187348/#review192416

> Not sure if we should be removing this. As the comment says, it's for high contrast mode when background images disappear.

Yeah, this just needs a color: transparent on the search button css
Comment on attachment 8916124 [details]
Bug 1400326 - Preparing existing about:home for 57 if we decide not to ship activity stream on it

https://reviewboard.mozilla.org/r/187348/#review192416

> Any reason not to just delete the html? Tests? I suppose this should be fine for now if it'll just all be deleted anyway.

I started doing that, but it started to be a pretty large diff, since I had to remove stuff that was looking for the html. So we decided it was less risky to just keep it as small of a diff as possible. We should file a ticket though to properly remove all of about:home stuff for when we're ready
Comment on attachment 8916124 [details]
Bug 1400326 - Preparing existing about:home for 57 if we decide not to ship activity stream on it

https://reviewboard.mozilla.org/r/187348/#review192440
Attachment #8916124 - Flags: review?(khudson) → review+
Comment on attachment 8916124 [details]
Bug 1400326 - Preparing existing about:home for 57 if we decide not to ship activity stream on it

https://reviewboard.mozilla.org/r/187348/#review192442

Let's get this in so others can play around with it and make a decision.
Attachment #8916124 - Flags: review+
Attachment #8916124 - Flags: feedback?(jaws)
Comment on attachment 8916124 [details]
Bug 1400326 - Preparing existing about:home for 57 if we decide not to ship activity stream on it

Stealing feedback? flag from jaws - this lgtm.
Attachment #8916124 - Flags: feedback?(jaws) → feedback+
I looked through the patch and had many of the same questions that Mardak asked in his review. I'm not fond of us changing nodes to display:none when we could just be removing them, I also would like to not make major changes to our fallback (about:home) without many hours of time to test that this didn't break anything.
Pushed by usarracini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/455a2c387a21
Preparing existing about:home for 57 if we decide not to ship activity stream on it r=k88hudson,Mardak
https://hg.mozilla.org/mozilla-central/rev/455a2c387a21
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Is that something that we would like to uplift in 57?
Flags: needinfo?(usarracini)
:sylvestre This is something we definitely want to uplift to 57.  

Note that it is preffed off by default, and will be used for testing how engagement and retention is affected by the Activity Stream version of about:home in subsequent Shield studies.

We want to make sure we spend some additional QA effort with this feature manually preffed on.
Comment on attachment 8916124 [details]
Bug 1400326 - Preparing existing about:home for 57 if we decide not to ship activity stream on it

Approval Request Comment
[Feature/Bug causing the regression]: Activity Stream on about:home (See bug 1399961)
[User impact if declined]: Inability to pref off Activity Stream on about:home
[Is this code covered by automated tests?]: Yes, about:home has automated tests 
[Has the fix been verified in Nightly?]: Yes, 20171010100200
[Needs manual test from QE? If yes, steps to reproduce]: Yes. STR: open about:config, set "browser.newtabpage.activity-stream.aboutHome.enabled" to false. Open a new window and observe that Activity Stream is not enabled on about:home
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Medium risk
[Why is the change risky/not risky?]: It mostly just changes CSS of existing about:home to make it match Photon designs
[String changes made/needed]: None
Flags: needinfo?(usarracini)
Attachment #8916124 - Flags: approval-mozilla-beta?
Comment on attachment 8916124 [details]
Bug 1400326 - Preparing existing about:home for 57 if we decide not to ship activity stream on it

This seems like a must, beta57+
Attachment #8916124 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I can confirm that 58.0a1 (2017-10-12) and 57.0b8 build3 (20171013042429) are verified fixed, using Windows 10 x64, Ubuntu 16.04 x86 and macOS 10.12.1.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
status-firefox58: fixed → verified
Flags: qe-verify+
See Also: → 1407957
Can anyone explain why the buttons were hidden? I disabled activity stream and this has been bothering me for some time.
If the buttons and onboarding overlapped, wouldn't it be better to place one above the other?
(In reply to Oriol Brufau [:Oriol] from comment #31)
> Can anyone explain why the buttons were hidden? I disabled activity stream
> and this has been bothering me for some time.
> If the buttons and onboarding overlapped, wouldn't it be better to place one
> above the other?

We removed the buttons on about:home because all of these, including restore session, are available in the menu or toolbar beginning with Firefox 57.
(In reply to Verdi [:verdi] from comment #32)
> We removed the buttons on about:home because all of these, including restore
> session, are available in the menu or toolbar beginning with Firefox 57.

But redundancy can be good if it requires less clicks. And weren't all the buttons also available somewhere in the menu or toolbar in Autralis?
Blocks: 1433133
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: